451370 - Stop using nsIAbAddressCollecter::GetCardFromAttribute in msgHdrViewOverlay.js

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Mark Banner (:standard8)]

Once bug 450149 is completed there will be a new method on nsIAbDirectory called cardForProperty.

There is one use of nsIAbAddressCollecter::GetCardFromAttribute in msgHdrViewOverlay.js (setFromBuddyIcon), this should be switched over to use the new nsIAbDirectory::cardForProperty.

I would recommend changing it to either cycle round all the address books or just getting the collected address book uri from prefs and getting that directory.

Either way, we need nsIAbAddressCollector::GetCardFromAttribute to die.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 1

Here's a patch which removes it from JS.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 335173 [details] [diff] [review]
Patch, version 1

+       var directory = directories.getNext();
+                                  .QueryInterface(Components.interfaces.nsIAbDirectory);

You don't want the ; on the first line here...

r=me with that fixed.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 335173 [details] [diff] [review]
Patch, version 1

Actually, this is a SM specific file, so Neil should do this...

Comment 4

[[e-mail]]

Comment on attachment 335173 [details] [diff] [review]
Patch, version 1

Wouldn't you be better off fixing nsAbAddressCollecter.cpp [sic]?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> (From update of attachment 335173 [details] [diff] [review])
> Wouldn't you be better off fixing nsAbAddressCollecter.cpp [sic]?

From my perspective nsAbAddressCollecter is for collecting addresses, not searching through address books.

Maybe we should have a central function somewhere (for going through all address books), but IMHO nsAddressCollecter isn't the place, and its something we probably want to re-examine once we're at the stage where we can remove rdf from the address book - as that will define how the new structure will work.

Comment 6

[[e-mail]]

Comment on attachment 335173 [details] [diff] [review]
Patch, version 1

>      if (myScreenName && card && card.setProperty("_AimScreenName")) {
Isn't this supposed to say card.getProperty("_AimScreenName")?

Comment 7

[Joshua Cranmer [:jcranmer]]

Checked in, changeset 2b8c8bbe40ca.

(In reply to comment #6)
> (From update of attachment 335173 [details] [diff] [review])
> >      if (myScreenName && card && card.setProperty("_AimScreenName")) {
> Isn't this supposed to say card.getProperty("_AimScreenName")?

Looks like it's a regression from bug 413260. There's a bit more to the code that needs to be changed, but since the path is only executed if you have a certain hidden pref, I'll file another bug on the issue.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> I'll file another bug on the issue.

I filed bug 454073 ;->