make tracker return true/false when adding a changed ID to indicate if it was a valid add or not; change bookmarks & history trackers to match; fix some problems in bookmarks tracker

services/sync/modules/engines/bookmarks.js

@@ -672,43 +672,36 @@ BookmarksTracker.prototype = {
   },
 
   onItemAdded: function BMT_onEndUpdateBatch(itemId, folder, index) {
-    this._all[itemId] = this._bms.getItemGUID(itemId);
     this._log.trace("onItemAdded: " + itemId);
-    this.addChangedID(this._all[itemId]);
-    this._upScore();
+    this._all[itemId] = this._bms.getItemGUID(itemId);
+    if (this.addChangedID(this._all[itemId]))
+      this._upScore();
   },
 
   onItemRemoved: function BMT_onItemRemoved(itemId, folder, index) {
     this._log.trace("onItemRemoved: " + itemId);
-    this.addChangedID(this._all[itemId]);
+    if (this.addChangedID(this._all[itemId]))
+      this._upScore();
     delete this._all[itemId];
-    this._upScore();
   },
 
-  onItemChanged: function BMT_onItemChanged(itemId, property, isAnnotationProperty, value) {
-    this._log.trace("onItemChanged: " + itemId + ", property: " + property +
-                    ", isAnno: " + isAnnotationProperty + ", value: " + value);
-
-    // NOTE: we get onItemChanged too much, when adding an item, changing its
-    //       GUID, before removal... it makes removals break, because trying
-    //       to update our cache before removals makes it so we think the
-    //       temporary guid was removed, instead of the real one.
-    //       Therefore, we ignore all guid changes.  When *Weave* changes the
-    //       GUID, we take care to update the tracker's cache.  If anyone else
-    //       changes the GUID, that will case breakage.
-    let guid = this._bms.getItemGUID(itemId);
-    if (guid != this._all[itemId])
-      this._log.trace("GUID change, ignoring");
-    else
-      this.addChangedID(this._all[itemId]); // some other change
-
-    this._upScore();
+  onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value) {
+    this._log.trace("onItemChanged: " + itemId +
+                    ", \"" + property + (isAnno? " (anno)" : "") + "\"" +
+                    " = \"" + value + "\"");
+    // 1) notifications for already-deleted items are ignored
+    // 2) note that engine/store are responsible for manually updating the
+    //    tracker's placesId->weaveId cache
+    if ((itemId in this._all) &&
+        (this._bms.getItemGUID(itemId) != this._all[itemId]) &&
+        this.addChangedID(this._all[itemId]))
+      this._upScore();
   },
 
   onItemMoved: function BMT_onItemMoved(itemId, oldParent, oldIndex, newParent, newIndex) {
     this._log.trace("onItemMoved: " + itemId);
-    this.addChangedID(this._all[itemId]);
-    this._upScore();
+    if (this.addChangedID(this._all[itemId]))
+      this._upScore();
   },
 
   onBeginUpdateBatch: function BMT_onBeginUpdateBatch() {},

services/sync/modules/engines/history.js

@@ -433,25 +433,23 @@ HistoryTracker.prototype = {
    * Clearing the whole history is worth 50 points (see below)
    */
   _upScore: function BMT__upScore() {
-    if (!this.enabled)
-      return;
     this._score += 1;
   },
 
   onVisit: function HT_onVisit(uri, vid, time, session, referrer, trans) {
     this._log.trace("onVisit: " + uri.spec);
-    this.addChangedID(this._store._getGUID(uri));
-    this._upScore();
+    if (this.addChangedID(this._store._getGUID(uri)))
+      this._upScore();
   },
   onPageExpired: function HT_onPageExpired(uri, time, entry) {
     this._log.trace("onPageExpired: " + uri.spec);
-    this.addChangedID(this._store._getGUID(uri)); // XXX eek ?
-    this._upScore();
+    if (this.addChangedID(this._store._getGUID(uri))) // XXX eek ?
+      this._upScore();
   },
   onDeleteURI: function HT_onDeleteURI(uri) {
     this._log.trace("onDeleteURI: " + uri.spec);
-    this.addChangedID(this._all[uri]);
-    this._upScore();
+    if (this.addChangedID(this._all[uri]))
+      this._upScore();
   },
   onClearHistory: function HT_onClearHistory() {
     this._log.trace("onClearHistory");

services/sync/modules/trackers.js

@@ -170,29 +170,31 @@ Tracker.prototype = {
   addChangedID: function T_addChangedID(id) {
     if (!id) {
       this._log.warn("Attempted to add undefined ID to tracker");
-      return;
+      return false;
     }
     if (id in this._ignored)
-      return;
-    this._log.debug("Adding changed ID " + id);
+      return false;
     if (!this.changedIDs[id]) {
+      this._log.debug("Adding changed ID " + id);
       this.changedIDs[id] = true;
       this.saveChangedIDs();
     }
+    return true;
   },
 
   removeChangedID: function T_removeChangedID(id) {
     if (!id) {
       this._log.warn("Attempted to remove undefined ID to tracker");
-      return;
+      return false;
     }
     if (id in this._ignored)
-      return;
-    this._log.debug("Removing changed ID " + id);
+      return false;
     if (this.changedIDs[id]) {
+      this._log.debug("Removing changed ID " + id);
       delete this.changedIDs[id];
       this.saveChangedIDs();
     }
+    return true;
   },
 
   clearChangedIDs: function T_clearChangedIDs() {