1490626 - Add new notifications we need for address book API

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[Geoff Lankow (:darktrojan)]

There's some places in the address book code where the existing notifications just don't cut it. I'm adding some new ones using the observer service instead of the current mechanism to avoid breaking something. Perhaps in future we can convert all of these to observer service notifications.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1490626-addressbook-notifications-1.diff

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9008364 [details] [diff] [review]
1490626-addressbook-notifications-1.diff

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

Looks good except the naming :) r=mkmelin

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +670,5 @@
> +    nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +    if (observerService) {
> +      nsCString thisUID;
> +      this->GetUID(thisUID);
> +      observerService->NotifyObservers(card, "ab:contactCreated", NS_ConvertUTF8toUTF16(thisUID).get());

A topic naming convention as ab-contact-created, ab-contact-updated and so on seems more common.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1490626-addressbook-notifications-2.diff

New notification names and using nsAutoCString instead of nsCString for local variables.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/60d7adb0d3bd
Add new notifications we need for WebExt address book API; r=mkmelin

Comment 5

[[e-mail]]

Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff

> NS_IMETHODIMP nsAbMDBDirectory::DropCard(nsIAbCard* aCard, bool needToCopyCard)
...
>     mDatabase->CreateNewListCardAndAddToDB(this, m_dbRowID, newCard, false /* notify */);
>+    nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
>+    if (observerService) {
>+      nsAutoCString thisUID;
>+      this->GetUID(thisUID);
>+      observerService->NotifyObservers(newCard, "addrbook-list-member-added", NS_ConvertUTF8toUTF16(thisUID).get());
>+    }
>   }
>   else {
>     mDatabase->CreateNewCardAndAddToDB(newCard, true /* notify */, this);
>   }
No notification for the else case (dragging and dropping a card from one addressbook to another). (I didn't test dragging from one addressbook to a list because I haven't got as far as lists yet.)

Comment 6

[Ben Bucksch (:BenB)]

Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff

We would like to ship our addon for TB 60 that uses the new API. (We had before created our own WebExperiment, which was then obsoleted by this nice new Thunderbird API.) The addon is "Owl for Exchange".

We can import the WebExtension API as WebExperiment into our addon, but we still need these notifications from TB for edits to work. It would therefore be nice for this patch to be backported to TB 60.

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined:
Users of our addon will not be able to edit contacts that come from Exchange via our addon. Edits will be lost and overwritten after restart / re-login.

Testing completed (on c-c, etc.):
Landed on TB trunk a while ago.

Risk to taking this patch (and alternatives if risky):
This adds only new notifications that didn't exist before. Therefore, nothing in Thunderbird 60 should be listening to the notifications. Thus, this is only new code and should be safe to backport.

Comment 7

[Ben Bucksch (:BenB)]

> No notification for the else case (dragging and dropping a card from one addressbook to another).

@Neil: Can you please file a follow-up bug on this, and mention the bug number here?

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #6)
> We would like to ship our addon for TB 60 that uses the new API ...
OK, the code doesn't quite apply to comm-esr60, but there is only a trivial merge conflict. This rely on the ID thing, bug 1482040?

Comment 9

[Ben Bucksch (:BenB)]

OK. We'll look at it, and backport it as necessary.

Comment 10

[Geoff Lankow (:darktrojan)]

Yes, you will need bug 1482040 to make the API and the notifications work. I'm tempted to say just backport the API as well, but I'm not sure that's a good idea.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff

Good for 60.4.

Comment 12

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/73ed0515afc2a98dd34890ada1a80324c42daf0a

Comment 13

[Jorg K (CEST = GMT+2)]

Backed out from the ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/d6e310cff401f6945897aad017e4663dfc9f40cc
Backed out changeset 73ed0515afc2 (bug 1490626) for causing address book and auto-complete slowness (bug 1511885). a=backout

Comment 14

[Jorg K (CEST = GMT+2)]

Relanded for TB 60.4.0 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/7afc65db094f2c60dda0e65753d3ddf8b87717a4