1482040 - Add UID property to address book interfaces

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Attachment: 1469238-addressbook-guids-1.diff

This is the same patch I posted in bug 1469238. It really should be in its own bug, so here we are.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1482040-addressbook-guids-2.diff

Updated patch to go with the other bug. It's the same as the first, but without the letter G.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9005103 [details] [diff] [review]
1482040-addressbook-guids-2.diff

Review of attachment 9005103 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/public/nsIAbCard.idl
@@ +108,5 @@
>     * implementations SHOULD NOT, in general, modify this property.
>     */
>    attribute AUTF8String localId;
>  
> +  attribute ACString UID;

probably better make it AUTF8String (like localId) above.
And please add documentation too.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1482040-addressbook-guids-3.diff

I've not changed it to AUTF8String because there is no situation where it will need non-ASCII characters. I've documented what the contents of the property should be and stated that others should not change the value once set.

Comment 4

[Magnus Melin [:mkmelin]]

Yes we're not perhaps expecting non-ASCII. But I think anything new should use UTF-8 for everything, so there's one thing less to worry about.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9005159 [details] [diff] [review]
1482040-addressbook-guids-3.diff

Review of attachment 9005159 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/public/nsIAbCard.idl
@@ +110,5 @@
>    attribute AUTF8String localId;
>  
>    /**
> +   * A unique identifier for this card.
> +   * It should be 9 random bytes, base 64 encoded: 12 ASCII characters.

I'll comment on the other bug

@@ +111,5 @@
>  
>    /**
> +   * A unique identifier for this card.
> +   * It should be 9 random bytes, base 64 encoded: 12 ASCII characters.
> +   * This attribute is set by the WebExtensions interface and

Hmm, we might want to use it internally too? At least in the future

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1482040-addressbook-guids-4.diff

UIDs are now set on read if empty, and cannot be set by script.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9008360 [details] [diff] [review]
1482040-addressbook-guids-4.diff

Review of attachment 9008360 [details] [diff] [review]:
-----------------------------------------------------------------

Great, r=mkmelin

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1482040-addressbook-guids-5.diff

Changed to keep the UID in memory as well as in the prefs for address books, so we still have it when making a delete notification.

Also some style changes I learned about shortly after submitting last time.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9008981 [details] [diff] [review]
1482040-addressbook-guids-5.diff

Review of attachment 9008981 [details] [diff] [review]:
-----------------------------------------------------------------

Great that you fixed that!

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +154,5 @@
> +NS_IMETHODIMP nsAbDirProperty::GetUID(nsACString &aUID)
> +{
> +  nsresult rv = NS_OK;
> +  aUID = mUID;
> +  if (aUID.IsEmpty() && !m_IsMailList) {

nit: while the same thing, it's more logical to test mUID.IsEmpty() and then go on to assign as nesseary

But I actually had a question too. Why are mailLists treated differently here and in nsAbDirProperty::SetUID? The test you added says they do have an uid.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1482040-addressbook-guids-6.diff

(In reply to Magnus Melin [:mkmelin] from comment #9)
> nit: while the same thing, it's more logical to test mUID.IsEmpty() and then
> go on to assign as nesseary

You mean like I've done it here?

> But I actually had a question too. Why are mailLists treated differently
> here and in nsAbDirProperty::SetUID? The test you added says they do have an
> uid.

Address books store their UID in the prefs (ldap_2.servers.foo.uid) but mailing lists are stored in the mork database as an nsIAbCard. But mailing lists also implement nsIAbDirectory so the UID is copied from the card object to the directory object and we only keep it in memory.

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #10)
> You mean like I've done it here?

Yes, thx!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bab51e3b5c2
Add UID property to address book interfaces; r=mkmelin

Comment 13

[[e-mail]]

Attachment: Whitespace changes for ESR 60 uplift

Ben requested backport of bug 1490626 which depends on this bug. I needed to verify that the backport will indeed allow us to import the API from bug 1469238 as an Experiment API and use that in Thunderbird 60 after the backport. To save you from having to fix the not quite trivial whitespace changes twice, this is the diff I ended up using for ESR 60.

Comment 14

[Jorg K (CEST = GMT+2)]

Well, for the white-space changes I would have just uplifted
https://hg.mozilla.org/releases/comm-beta/rev/7ebd318d99dac3805458096431ef1f5bf8aa6633
https://hg.mozilla.org/releases/comm-beta/rev/505528fac6296ffb436caf0b8c711df8d8c5f621
See bug 1463266 comment #31. Then the patch almost applies with two minor tweaks.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9021487 [details] [diff] [review]
Whitespace changes for ESR 60 uplift

Sorry, I've fixed the white-space issues in ESR 60 now. Can you rebase this again, this time it will be a three-minute job. Next time, please ask.

Comment 16

[[e-mail]]

(In reply to Jorg K from comment #15)
> Sorry, I've fixed the white-space issues in ESR 60 now. Can you rebase this
> again, this time it will be a three-minute job. Next time, please ask.

I double-checked and the original patch now applies to nsAddrDatabase.cpp correctly but you didn't backport the white-space changes to nsAbDirProperty.* so there's still one white space conflict in each file.

Comment 17

[Jorg K (CEST = GMT+2)]

You mean https://hg.mozilla.org/comm-central/rev/7692c0e19b39. I didn't intend to backport that, so your patch is welcome.

Comment 18

[[e-mail]]

Attachment: Fewer whitespace changes for ESR 60 uplift

Comment 19

[Ben Bucksch (:BenB)]

Comment on attachment 9021588 [details] [diff] [review]
Fewer whitespace changes for ESR 60 uplift

[Approval Request Comment]
User impact if declined:
Owl needs to support 2 different address book APIs, one for TB 60 and one for TB trunk.

Testing completed (on c-c, etc.):
Owl on TB 60 with this patch works.

Risk to taking this patch (and alternatives if risky):
Most of the patch adds new functions, so that should be safe.
nsAddrDatabase.cpp adds a few calls in existing code.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9021588 [details] [diff] [review]
Fewer whitespace changes for ESR 60 uplift

Good for 60.4.

Comment 21

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/7fe1827e6b395377f9c72160e9d191104fc313ed

Comment 22

[Jorg K (CEST = GMT+2)]

As you can see, I've uplifted this now, but a test is not working:

TEST-UNEXPECTED-FAIL | comm/mailnews/addrbook/test/unit/test_uid.js | xpcshell return code: 0
TypeError: MailServices.ab.directories is not iterable at /builds/worker/workspace/build/tests/xpcshell/tests/comm/mailnews/addrbook/test/unit/test_uid.js:19

Line 19 is:   for (let book of MailServices.ab.directories) {

Can someone please provide a fix for this.

Comment 23

[Ben Bucksch (:BenB)]

Neil will look into it.

Comment 24

[[e-mail]]

Attachment: Fix test failure on ESR60

This fixes the test locally for me.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9026410 [details] [diff] [review]
Fix test failure on ESR60

Thanks for looking into it, Neil. I'll get Geoff to review it and I'll get it landed after that.

Layman's question: Would this work:
  for (let book of fixIterator(MailServices.ab.directories, Ci.nsIAbDirectory) {

I'm not sure how that could be applied to the second call site, perhaps
  let bookCards = [...fixIterator(book.childCards,Ci.nsIAbCard)];

Either way, it's just a small test fix.

Comment 26

[Geoff Lankow (:darktrojan)]

Comment on attachment 9026410 [details] [diff] [review]
Fix test failure on ESR60

Review of attachment 9026410 [details] [diff] [review]:
-----------------------------------------------------------------

I'm happy with this. The fixIterator way should work too, but as this is a test on ESR, I'm really not bothered which you use.

Comment 27

[[e-mail]]

I didn't know whether fixIterator was in scope, as the other tests didn't seem to use it.

Comment 28

[Jorg K (CEST = GMT+2)]

Yes, little usage in tests, but there's one here importing it:
https://searchfox.org/comm-central/rev/bcbcd1dcf23821605e10f1cf1b00bf3d14460a73/mailnews/imap/test/unit/test_localToImapFilter.js#14

Comment 29

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/a6e340fe4c07d16d962abab48404aad0d07848b8
Fix failure in test_uid.js by iterating MailServices.ab.directories 'manually'. r=darktrojan

Comment 30

[Ben Bucksch (:BenB)]

Thanks, everybody

Comment 31

[Jorg K (CEST = GMT+2)]

Backed out from the ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/e4d520b5dc17e8a0e3ba4d760556b1ba4aad4842
Backed out changeset a6e340fe4c07 and 7fe1827e6b39 (bug 1482040) for causing address book and auto-complete slowness (bug 1511885). a=backout

Comment 32

[Jorg K (CEST = GMT+2)]

Relanded for TB 60.4.0 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/a02f10ecb5fdcb78ebe00b432c3c808e86de39c4
https://hg.mozilla.org/releases/comm-esr60/rev/707388b59cb577cad0242f36bfa15d902ff65408

Comment 33

[John Bieling (:TbSync)]

Hi Geoff,

I tried to use the new UID but with mailing lists I run into an issue: the UID is not persistent and changes between requests. I am still working with old address book interface and not the new mailextension you are working with. Are you seeing the same thing?

I have a standard address book listener and in onItemPropertyChanged I just log the UID to the console, if Item.isMailList.

This may be related to a general (stupid) bug in Mailing lists cards, that they do not keep any property assigned to them except NickName. Can anyone confirm this?

I am on TB 60.4

Comment 34

[Magnus Melin [:mkmelin]]

Hi John, please file a new bug about that, and cc Geoff. (You can use "Clone This Bug" below)

Comment 35

[Ben Bucksch (:BenB)]

Crash on Mac: bug 1523904