Closed
Bug 1490626
Opened 6 years ago
Closed 6 years ago
Add new notifications we need for address book API
Categories
(Thunderbird :: Add-Ons: Extensions API, enhancement)
Thunderbird
Add-Ons: Extensions API
Tracking
(thunderbird_esr6064+ fixed, thunderbird64 fixed)
RESOLVED
FIXED
Thunderbird 64.0
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
darktrojan
:
review+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9008364 -
Flags: review?(mkmelin+mozilla)
Comment 2•6 years ago
|
||
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.
Attachment #9008364 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 3•6 years ago
|
||
New notification names and using nsAutoCString instead of nsCString for local variables.
Attachment #9008364 -
Attachment is obsolete: true
Attachment #9008982 -
Flags: review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/60d7adb0d3bd
Add new notifications we need for WebExt address book API; r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Comment 5•6 years ago
|
||
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•6 years ago
|
||
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.
Attachment #9008982 -
Flags: approval-comm-esr60?
Comment 7•6 years ago
|
||
> 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•6 years ago
|
||
(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•6 years ago
|
||
OK. We'll look at it, and backport it as necessary.
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff
Good for 60.4.
Attachment #9008982 -
Flags: approval-comm-esr60? → approval-comm-esr60+
Comment 12•6 years ago
|
||
TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/73ed0515afc2a98dd34890ada1a80324c42daf0a
status-thunderbird64:
--- → fixed
status-thunderbird_esr60:
--- → fixed
tracking-thunderbird_esr60:
--- → 64+
Comment 13•6 years ago
|
||
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
status-thunderbird_esr60:
fixed → ---
tracking-thunderbird_esr60:
64+ → ---
Comment 14•6 years ago
|
||
Relanded for TB 60.4.0 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/7afc65db094f2c60dda0e65753d3ddf8b87717a4
status-thunderbird_esr60:
--- → fixed
tracking-thunderbird_esr60:
--- → 64+
You need to log in
before you can comment on or make changes to this bug.
Description
•