Fix Bug 1444518 - Ignore bookmarks added and removed during an import…

… or restore

system-addon/lib/HighlightsFeed.jsm

@@ -27,6 +27,8 @@ const HIGHLIGHTS_MAX_LENGTH = 9;
 const MANY_EXTRA_LENGTH = HIGHLIGHTS_MAX_LENGTH * 5 + TOP_SITES_DEFAULT_ROWS * TOP_SITES_MAX_SITES_PER_ROW;
 const SECTION_ID = "highlights";
 const SYNC_BOOKMARKS_FINISHED_EVENT = "weave:engine:sync:applied";
+const BOOKMARKS_RESTORE_SUCCESS_EVENT = "bookmarks-restore-success";
+const BOOKMARKS_RESTORE_FAILED_EVENT = "bookmarks-restore-failed";
 
 this.HighlightsFeed = class HighlightsFeed {
   constructor() {
@@ -43,6 +45,8 @@ this.HighlightsFeed = class HighlightsFeed {
 
   init() {
     Services.obs.addObserver(this, SYNC_BOOKMARKS_FINISHED_EVENT);
+    Services.obs.addObserver(this, BOOKMARKS_RESTORE_SUCCESS_EVENT);
+    Services.obs.addObserver(this, BOOKMARKS_RESTORE_FAILED_EVENT);
     SectionsManager.onceInitialized(this.postInit.bind(this));
   }
 
@@ -55,11 +59,18 @@ this.HighlightsFeed = class HighlightsFeed {
     SectionsManager.disableSection(SECTION_ID);
     PageThumbs.removeExpirationFilter(this);
     Services.obs.removeObserver(this, SYNC_BOOKMARKS_FINISHED_EVENT);
+    Services.obs.removeObserver(this, BOOKMARKS_RESTORE_SUCCESS_EVENT);
+    Services.obs.removeObserver(this, BOOKMARKS_RESTORE_FAILED_EVENT);
   }
 
   observe(subject, topic, data) {
-    // When we receive a notification that a sync has happened for bookmarks, refresh highlights
-    if (topic === SYNC_BOOKMARKS_FINISHED_EVENT && data === "bookmarks") {
+    // When we receive a notification that a sync has happened for bookmarks,
+    // or Places finished importing or restoring bookmarks, refresh highlights
+    const manyBookmarksChanged =
+      (topic === SYNC_BOOKMARKS_FINISHED_EVENT && data === "bookmarks") ||
+      topic === BOOKMARKS_RESTORE_SUCCESS_EVENT ||
+      topic === BOOKMARKS_RESTORE_FAILED_EVENT;
+    if (manyBookmarksChanged) {
       this.fetchHighlights({broadcast: true});
     }
   }
@@ -232,4 +243,4 @@ this.HighlightsFeed = class HighlightsFeed {
   }
 };
 
-const EXPORTED_SYMBOLS = ["HighlightsFeed", "SECTION_ID", "MANY_EXTRA_LENGTH", "SYNC_BOOKMARKS_FINISHED_EVENT"];
+const EXPORTED_SYMBOLS = ["HighlightsFeed", "SECTION_ID", "MANY_EXTRA_LENGTH", "SYNC_BOOKMARKS_FINISHED_EVENT", "BOOKMARKS_RESTORE_SUCCESS_EVENT", "BOOKMARKS_RESTORE_FAILED_EVENT"];

system-addon/lib/PlacesFeed.jsm

@@ -142,6 +142,7 @@ class BookmarksObserver extends Observer {
    */
   onItemRemoved(id, folderId, index, type, uri, guid, parentGuid, source) { // eslint-disable-line max-params
     if (type === PlacesUtils.bookmarks.TYPE_BOOKMARK &&
+        source !== PlacesUtils.bookmarks.SOURCES.IMPORT_REPLACE &&
         source !== PlacesUtils.bookmarks.SOURCES.SYNC) {
       this.dispatch({
         type: at.PLACES_BOOKMARK_REMOVED,

system-addon/test/unit/lib/HighlightsFeed.test.js

@@ -14,6 +14,8 @@ describe("Highlights Feed", () => {
   let SECTION_ID;
   let MANY_EXTRA_LENGTH;
   let SYNC_BOOKMARKS_FINISHED_EVENT;
+  let BOOKMARKS_RESTORE_SUCCESS_EVENT;
+  let BOOKMARKS_RESTORE_FAILED_EVENT;
   let feed;
   let globals;
   let sandbox;
@@ -57,7 +59,7 @@ describe("Highlights Feed", () => {
 
     globals.set("NewTabUtils", fakeNewTabUtils);
     globals.set("PageThumbs", fakePageThumbs);
-    ({HighlightsFeed, SECTION_ID, MANY_EXTRA_LENGTH, SYNC_BOOKMARKS_FINISHED_EVENT} = injector({
+    ({HighlightsFeed, SECTION_ID, MANY_EXTRA_LENGTH, SYNC_BOOKMARKS_FINISHED_EVENT, BOOKMARKS_RESTORE_SUCCESS_EVENT, BOOKMARKS_RESTORE_FAILED_EVENT} = injector({
       "lib/FilterAdult.jsm": {filterAdult: filterAdultStub},
       "lib/ShortURL.jsm": {shortURL: shortURLStub},
       "lib/SectionsManager.jsm": {SectionsManager: sectionsManagerStub},
@@ -96,6 +98,8 @@ describe("Highlights Feed", () => {
     it("should add the sync observer", () => {
       feed.onAction({type: at.INIT});
       assert.calledWith(global.Services.obs.addObserver, feed, SYNC_BOOKMARKS_FINISHED_EVENT);
+      assert.calledWith(global.Services.obs.addObserver, feed, BOOKMARKS_RESTORE_SUCCESS_EVENT);
+      assert.calledWith(global.Services.obs.addObserver, feed, BOOKMARKS_RESTORE_FAILED_EVENT);
     });
     it("should call SectionsManager.onceInitialized on INIT", () => {
       feed.onAction({type: at.INIT});
@@ -120,11 +124,19 @@ describe("Highlights Feed", () => {
       feed.observe(null, SYNC_BOOKMARKS_FINISHED_EVENT, "bookmarks");
       assert.calledWith(feed.fetchHighlights, {broadcast: true});
     });
+    it("should fetch highlights after a successful import", () => {
+      feed.observe(null, BOOKMARKS_RESTORE_SUCCESS_EVENT, "html");
+      assert.calledWith(feed.fetchHighlights, {broadcast: true});
+    });
+    it("should fetch highlights after a failed import", () => {
+      feed.observe(null, BOOKMARKS_RESTORE_FAILED_EVENT, "json");
+      assert.calledWith(feed.fetchHighlights, {broadcast: true});
+    });
     it("should not fetch higlights when we are doing a sync for something that is not bookmarks", () => {
       feed.observe(null, SYNC_BOOKMARKS_FINISHED_EVENT, "tabs");
       assert.notCalled(feed.fetchHighlights);
     });
-    it("should not fetch higlights when we are not doing a sync at all", () => {
+    it("should not fetch higlights for other events", () => {
       feed.observe(null, "someotherevent", "bookmarks");
       assert.notCalled(feed.fetchHighlights);
     });
@@ -391,9 +403,11 @@ describe("Highlights Feed", () => {
       feed.onAction({type: at.UNINIT});
       assert.calledOnce(fakePageThumbs.removeExpirationFilter);
     });
-    it("should remove the sync observer", () => {
+    it("should remove the sync and Places observers", () => {
       feed.onAction({type: at.UNINIT});
       assert.calledWith(global.Services.obs.removeObserver, feed, SYNC_BOOKMARKS_FINISHED_EVENT);
+      assert.calledWith(global.Services.obs.removeObserver, feed, BOOKMARKS_RESTORE_SUCCESS_EVENT);
+      assert.calledWith(global.Services.obs.removeObserver, feed, BOOKMARKS_RESTORE_FAILED_EVENT);
     });
   });
   describe("#onAction", () => {

system-addon/test/unit/lib/PlacesFeed.test.js

@@ -390,6 +390,12 @@ describe("PlacesFeed", () => {
 
         assert.notCalled(dispatch);
       });
+      it("should not dispatch a PLACES_BOOKMARK_REMOVED action - has IMPORT_REPLACE source", async () => {
+        const args = [null, null, null, TYPE_BOOKMARK, {spec: "foo.com"}, "123foo", "", SOURCES.IMPORT_REPLACE];
+        await observer.onItemRemoved(...args);
+
+        assert.notCalled(dispatch);
+      });
       it("should dispatch a PLACES_BOOKMARK_REMOVED action with the right URL and bookmarkGuid", () => {
         observer.onItemRemoved(null, null, null, TYPE_BOOKMARK, {spec: "foo.com"}, "123foo", "", SOURCES.DEFAULT);
         assert.calledWith(dispatch, {type: at.PLACES_BOOKMARK_REMOVED, data: {bookmarkGuid: "123foo", url: "foo.com"}});