1633998 - Stop using and remove nsIAbListener

Status: RESOLVED FIXED

(MailNews Core :: Address Book, task)

Description

[Geoff Lankow (:darktrojan)]

Bug 1633607 added observer service notifications where nsIAbListener notifications are currently used, but did not stop using nsIAbListener. This is a follow-up bug for the transition.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove uses of nsIAbListener from WebExtensions code. r?mkmelin

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove uses of nsIAbListener from the message header. r?mkmelin

Depends on D87209

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove uses of nsIAbListener from the address book UI. r?mkmelin

Depends on D87210

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove uses of nsIAbListener from the preferences. r?mkmelin

Depends on D87211

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove uses of nsIAbListener from GloDa. r?mkmelin

Depends on D87212

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove uses of nsIAbListener in tests. r?mkmelin

Depends on D87213

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1633998 - Remove nsIAbListener. r?mkmelin

Depends on D87214

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aa28f395b8c7
Remove uses of nsIAbListener from WebExtensions code. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5bc2249d2953
Remove uses of nsIAbListener from the message header. r=mkmelin
https://hg.mozilla.org/comm-central/rev/cc4f22c4497a
Remove uses of nsIAbListener from the address book UI. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a0c102d5720b
Remove uses of nsIAbListener from the preferences. r=mkmelin
https://hg.mozilla.org/comm-central/rev/89dc87773321
Remove uses of nsIAbListener from GloDa. r=mkmelin
https://hg.mozilla.org/comm-central/rev/fa5517054a95
Remove uses of nsIAbListener in tests. r=mkmelin
https://hg.mozilla.org/comm-central/rev/6ab4b0003806
Remove nsIAbListener. r=mkmelin DONTBUILD

Comment 9

[alta88]

@@ -809,44 +799,21 @@ class AddrBookDirectory {
       }
     }
     return false;
   }
   addCard(card) {
     return this.dropCard(card, false);
   }
   modifyCard(card) {
-    let oldProperties = this._loadCardProperties(card.UID);
     let newProperties = new Map();
     for (let { name, value } of fixIterator(card.properties, Ci.nsIProperty)) {
       newProperties.set(name, value);
     }
     this._saveCardProperties(card);
-    for (let [name, oldValue] of oldProperties.entries()) {
-      if (name != "LastModifiedDate" && !newProperties.has(name)) {
-        MailServices.ab.notifyItemPropertyChanged(card, name, oldValue, null);
-      }
-    }
-    for (let [name, newValue] of newProperties.entries()) {
-      if (name == "LastModifiedDate") {
-        continue;
-      }
-      let oldValue = oldProperties.get(name);
-      if (oldValue == null && newValue == "") {
-        continue;
-      }
-      if (oldValue != newValue) {
-        MailServices.ab.notifyItemPropertyChanged(
-          card,
-          name,
-          oldValue,
-          newValue
-        );
-      }
-    }

Any proper add/change/delete mechanism will, for a change, notify of property being changed, oldvalue, newvalue. Is it now impossible for an observer to determine this?
There seems to be a topic "addrbook-contact-changed" without any notification of that topic name, is that intended?

Comment 10

[Geoff Lankow (:darktrojan)]

Is it now impossible for an observer to determine this?

Yes. But we don't have any such observers and if we wanted them we could add something to notify them. Also, the notification I'm removing here has only been functional since I replaced the provider December. Before that, the property name and both values were null, which is equally useless, and we coped like that for decades. So I'm not concerned.

Comment 11

[alta88]

Even in the abstract, a change notifier should always do the right thing; it means an api is featureful, useful, efficient, smart, forward looking. I know it wasn't used internally, maybe if it were some issues of perf would go away.

But in fact, my extension implements, in 78, the missing functionality as described in Bug 460737. Now, instead of updating a cached entry on DisplayName change only, I have to rebuild the entire cache for any ab field change. So I'm requesting you restore the change fields and use the |data| argument as an object, with uid as a member, and change items as additional members.

Comment 12

[alta88]

What's worse, is that now that checking for and notifying change deltas has been removed, the change notification is sent even though no changes have occurred. It's sent on clicking Done in the message header Edit Contact popup panel, it's sent on OK in the Edit Details ab dialog. It's even sent when clicking Edit Details in the popup panel.

Comment 13

[Geoff Lankow (:darktrojan)]

Fine. File a bug and I'll implement a new notification that will tell you the name of any changed property.