Closed
Bug 454073
Opened 16 years ago
Closed 16 years ago
Fix regressions from bug 413260 related to |aimScreenName|
Categories
(MailNews Core :: Address Book, defect)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: sgautherie, Assigned: sgautherie)
References
()
Details
(Keywords: regression)
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
dmosedale
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
standard8
:
review+
dmosedale
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
standard8
:
review+
dmosedale
:
superreview+
|
Details | Diff | Splinter Review |
Bug 309057 comment 69:
[
From Serge Gautherie 2008-09-04 11:22:29 PDT
(In reply to comment #68)
> (From update of attachment 336740 [details] [diff] [review] [details])
> >+ if (card && card.setProperty("_AimScreenName")) {
> I think this was supposed to be .getProperty (I did comment on that bug but I
> guess nothing happened...)
I very much thought so myself too, when I read the patch which changed it ! ;-<
]
This regression comes from bug 413260.
I'm not sure which bug Neil was referring to.
Comment 1•16 years ago
|
||
I mentioned it in bug 451370.
Assignee | ||
Comment 2•16 years ago
|
||
*s/setProperty()/getProperty()/:
I checked bug 413260 for this property: this occurrence was the only error.
*s/aimScreenName/getProperty("_AimScreenName")/:
Missed in bug 413260; this code was removed from TB in bug 331924.
(untested ... see (future) bug 453627)
NB: I have a "conflicting" patch in bug 309057 which I'll land first, then I'll unbitrot this patch.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #337298 -
Flags: superreview?(dmose)
Attachment #337298 -
Flags: review?(bugzilla)
Assignee | ||
Comment 3•16 years ago
|
||
(In reply to comment #1)
> I mentioned it in bug 451370.
Bug 451370 comment 6 ;-)
It seems Joshua never filed that bug he wrote he would.
Updated•16 years ago
|
Attachment #337298 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 4•16 years ago
|
||
Av1-SM, unbitrotted after bug 309057 checkin.
Attachment #337298 -
Attachment is obsolete: true
Attachment #337554 -
Flags: superreview?(dmose)
Attachment #337298 -
Flags: superreview?(dmose)
Comment 5•16 years ago
|
||
Comment on attachment 337554 [details] [diff] [review]
(Av1a-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 6]
sr=dmose; thanks for the patch
Attachment #337554 -
Flags: superreview?(dmose) → superreview+
Assignee | ||
Comment 6•16 years ago
|
||
Comment on attachment 337554 [details] [diff] [review]
(Av1a-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 6]
http://hg.mozilla.org/comm-central/rev/65f0fe195fc7
Attachment #337554 -
Attachment description: (Av1a-SM) <msgHdrViewOverlay.js> → (Av1a-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 6]
Assignee | ||
Comment 7•16 years ago
|
||
2 occurrences missed in bug 413260 ... copied from what SeaMonkey got/has.
I'm not sure why the default value is needed there in <addressbook.js>, but...
(Joshua !?)
(untested ... but obvious, as all the rest was updated that way.)
Attachment #337576 -
Flags: superreview?(dmose)
Attachment #337576 -
Flags: review?(bugzilla)
Assignee | ||
Updated•16 years ago
|
Updated•16 years ago
|
Attachment #337576 -
Flags: review?(bugzilla) → review+
Updated•16 years ago
|
Attachment #337576 -
Flags: superreview?(dmose) → superreview+
Comment 8•16 years ago
|
||
Comment on attachment 337576 [details] [diff] [review]
(Bv1-TB) 2 <ab*.js>
[Checkin: Comment 9]
sr=dmose
Assignee | ||
Comment 9•16 years ago
|
||
Comment on attachment 337576 [details] [diff] [review]
(Bv1-TB) 2 <ab*.js>
[Checkin: Comment 9]
http://hg.mozilla.org/comm-central/rev/d67d548170ce
Attachment #337576 -
Attachment description: (Bv1-TB) 2 <ab*.js> → (Bv1-TB) 2 <ab*.js>
[Checkin: Comment 9]
Assignee | ||
Comment 10•16 years ago
|
||
(In reply to comment #7)
> I'm not sure why the default value is needed there in <addressbook.js>, but...
I was really wondering because I read
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbCard.idl?mark=98-107#97
> (Joshua !?)
IIRC, Joshua remembered and pointed me to
http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCardViewOverlay.js?mark=200-202#193
which SeaMonkey has too
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/resources/content/abCardViewOverlay.js?mark=200-202#192
So, occurrences of (both) abCardViewOverlay.js do not use the default value,
and other files "need" one.
Assignee | ||
Comment 11•16 years ago
|
||
Fix Av1a-SM, after comment 10.
I'm not sure if the (JS) interface allows me to not provide the 2 parameter on the second call ... then can't hurt to have it.
Attachment #338383 -
Flags: superreview?(dmose)
Attachment #338383 -
Flags: review?(bugzilla)
Comment 12•16 years ago
|
||
If the parameter is allowed to be null then you can annotate the interface with [optional] (does not need a uuid revision).
Assignee | ||
Comment 13•16 years ago
|
||
(In reply to comment #12)
> If the parameter is allowed to be null then you can annotate the interface with
Here is how I understand it:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbCard.idl?mark=60,76,98-107#75
the _AimScreenName property is always available,
and stored in
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbCardProperty.h?mark=77-78#76
then
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsInterfaceHashtable.h?mark=64-66#53
always returns a non-null |UserDataType*|
and
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbCardProperty.cpp?mark=229-236#228
never accesses its defaultValue for such a property,
so its |nsIVariant *defaultValue| parameter would accept a |null| (= not JS provided !?) input value.
> [optional] (does not need a uuid revision).
Did I understand correctly ?
So |""| would not be needed after adding |[optional]| !?
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #13)
> http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsInterfaceHashtable.h?mark=64-66#53
> always returns a non-null |UserDataType*|
Actually, this should rather read
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbCardProperty.cpp?mark=229-236#228
always returns |PR_TRUE|
Assignee | ||
Comment 15•16 years ago
|
||
Updated•16 years ago
|
Attachment #338383 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 16•16 years ago
|
||
Neil, Joshua, did I understand correctly the |[optional]| suggestion ?
Comment 17•16 years ago
|
||
So, as it happens, GetProperty crashes if you pass a null default value and the property does not exist. Oops. Once that's fixed, you can make it [optional], assuming you have no problem with a null (rather than, say, "") return value.
Assignee | ||
Comment 18•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20081002 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)
Eventually, I tested this feature :->
This additional patch is needed to fix
{
Exception ``[Exception... "Not enough arguments [nsIAbCard.getProperty]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: chrome://messenger/content/msgHdrViewOverlay.js :: setFromBuddyIcon :: line 989" data: no]'' thrown from function setFromBuddyIcon() in <chrome://messenger/content/msgHdrViewOverlay.js> line 989.
}
which Venkman reports. (otherwise masked by the empty catch)
Attachment #338383 -
Attachment is obsolete: true
Attachment #341503 -
Flags: superreview?(dmose)
Attachment #341503 -
Flags: review?(bugzilla)
Attachment #338383 -
Flags: superreview?(dmose)
Assignee | ||
Comment 19•16 years ago
|
||
Cv1a-SM, un-regressed.
Attachment #341503 -
Attachment is obsolete: true
Attachment #341507 -
Flags: superreview?(dmose)
Attachment #341507 -
Flags: review?(bugzilla)
Attachment #341503 -
Flags: superreview?(dmose)
Attachment #341503 -
Flags: review?(bugzilla)
Assignee | ||
Comment 20•16 years ago
|
||
(In reply to comment #17)
> So, as it happens, GetProperty crashes if you pass a null default value and the
> property does not exist. Oops.
Yes, but it should (have) work fine (as is) for an existing property.
(But crashing if the property doesn't exist would not be fine: I understand.)
> Once that's fixed, you can make it [optional],
Then, I filed bug 458286.
> assuming you have no problem with a null (rather than, say, "") return value.
That shouldn't be an issue, as it will still be possible to pass a wanted default value.
Comment 21•16 years ago
|
||
Comment on attachment 341507 [details] [diff] [review]
(Cv1b-SM) <msgHdrViewOverlay.js>
You should be redoing the indentation as you have added the extra if. Please provide both a -w and a normal patch when asking for review next time.
Attachment #341507 -
Flags: review?(bugzilla) → review-
Assignee | ||
Comment 22•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20081005 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)
Cv1b-SM, with comment 21 suggestion(s),
and more.
Attachment #341507 -
Attachment is obsolete: true
Attachment #341864 -
Flags: superreview?(dmose)
Attachment #341864 -
Flags: review?(bugzilla)
Attachment #341507 -
Flags: superreview?(dmose)
Comment 23•16 years ago
|
||
(In reply to comment #22)
> Created an attachment (id=341864) [details]
> (Cv2-SM) <msgHdrViewOverlay.js>
Please can you provide a diff -w of these changes to make the review easier.
Assignee | ||
Comment 24•16 years ago
|
||
Updated•16 years ago
|
Attachment #341864 -
Flags: review?(bugzilla) → review+
Comment 25•16 years ago
|
||
Comment on attachment 341864 [details] [diff] [review]
(Cv2-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 26]
sr=dmose; thanks for your patience on the review. Out of curiosity, do we think anyone still has profiles that contain these images in them? I assume the only ones that could possibly exist are ones that were used by some version of Netscape (or were migrated from there).
Attachment #341864 -
Flags: superreview?(dmose) → superreview+
Assignee | ||
Updated•16 years ago
|
Attachment #343124 -
Attachment is obsolete: true
Assignee | ||
Comment 26•16 years ago
|
||
Comment on attachment 341864 [details] [diff] [review]
(Cv2-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 26]
http://hg.mozilla.org/comm-central/rev/6c52afbf9f23
Attachment #341864 -
Attachment description: (Cv2-SM) <msgHdrViewOverlay.js> → (Cv2-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 26]
Assignee | ||
Comment 27•16 years ago
|
||
No longer blocks: 446343
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1b2
You need to log in
before you can comment on or make changes to this bug.
Description
•