454073 - Fix regressions from bug 413260 related to |aimScreenName|

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Serge Gautherie (:sgautherie)]

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

[[e-mail]]

I mentioned it in bug 451370.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-SM) <msgHdrViewOverlay.js>

*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.

Comment 3

[Serge Gautherie (:sgautherie)]

(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.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-SM) <msgHdrViewOverlay.js> [Checkin: Comment 6]

Av1-SM, unbitrotted after bug 309057 checkin.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 337554 [details] [diff] [review]
(Av1a-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 6]

sr=dmose; thanks for the patch

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 337554 [details] [diff] [review]
(Av1a-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 6]

http://hg.mozilla.org/comm-central/rev/65f0fe195fc7

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-TB) 2 <ab*.js> [Checkin: Comment 9]

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.)

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 337576 [details] [diff] [review]
(Bv1-TB) 2 <ab*.js>
[Checkin: Comment 9]

sr=dmose

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 337576 [details] [diff] [review]
(Bv1-TB) 2 <ab*.js>
[Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/d67d548170ce

Comment 10

[Serge Gautherie (:sgautherie)]

(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.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-SM) <msgHdrViewOverlay.js>

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.

Comment 12

[[e-mail]]

If the parameter is allowed to be null then you can annotate the interface with [optional] (does not need a uuid revision).

Comment 13

[Serge Gautherie (:sgautherie)]

(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]| !?

Comment 14

[Serge Gautherie (:sgautherie)]

(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|

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbCardProperty.cpp?mark=229-236#228

Argh:
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsBaseHashtable.h?mark=115-124#114

Comment 16

[Serge Gautherie (:sgautherie)]

Neil, Joshua, did I understand correctly the |[optional]| suggestion ?

Comment 17

[[e-mail]]

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.

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a-SM) <msgHdrViewOverlay.js>

[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)

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1b-SM) <msgHdrViewOverlay.js>

Cv1a-SM, un-regressed.

Comment 20

[Serge Gautherie (:sgautherie)]

(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

[Mark Banner (:standard8)]

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.

Comment 22

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2-SM) <msgHdrViewOverlay.js> [Checkin: Comment 26]

[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.

Comment 23

[Mark Banner (:standard8)]

(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.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2-SM-Bw) <msgHdrViewOverlay.js>

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

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).

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 341864 [details] [diff] [review]
(Cv2-SM) <msgHdrViewOverlay.js>
[Checkin: Comment 26]

http://hg.mozilla.org/comm-central/rev/6c52afbf9f23

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> Out of curiosity, [...]

This discussion is bug 453627 ;-)