Bug 1460849 - Allow new items (as well as existing) to be skipped in nsNavHistoryFolderResultNode::OnItemMoved if excludeItems is set. r=mak

MozReview-Commit-ID: CUCAsmG99A8 --HG-- extra : rebase_source : d190f2a5d236dfea25fe247142c69f9754b3cd4f
2018-05-11 13:23:41 +01:00 · 2018-05-11 13:23:41 +01:00 · d13db67be4
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@ -786,7 +786,8 @@ var Bookmarks = Object.freeze({
                                             updatedItem.index, updatedItem.type,
                                             updatedItem.guid, item.parentGuid,
                                             updatedItem.parentGuid,
-                                             updatedItem.source ]);
+                                             updatedItem.source,
+                                             updatedItem.url && updatedItem.url.href ]);
        }

        // Remove non-enumerable properties.
@ -1200,7 +1201,7 @@ var Bookmarks = Object.freeze({
                                           i, child.type,
                                           child.guid, child.parentGuid,
                                           child.parentGuid,
-                                           source ]);
+                                           source, child.url && child.url.href ]);
      }
    })();
  },
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@ -1447,10 +1447,11 @@ class SyncedBookmarksMirror {
    let movedItemRows = await this.db.execute(`
      SELECT b.id, b.guid, b.type, p.id AS newParentId, c.oldParentId,
             p.guid AS newParentGuid, c.oldParentGuid,
-             b.position AS newPosition, c.oldPosition
+             b.position AS newPosition, c.oldPosition, h.url
      FROM itemsMoved c
      JOIN moz_bookmarks b ON b.id = c.itemId
      JOIN moz_bookmarks p ON p.id = b.parent
+      LEFT JOIN moz_places h ON h.id = b.fk
      ORDER BY c.level, newParentId, newPosition`);
    for await (let row of yieldingIterator(movedItemRows)) {
      let info = {
@ -1463,6 +1464,7 @@ class SyncedBookmarksMirror {
        oldParentGuid: row.getResultByName("oldParentGuid"),
        newPosition: row.getResultByName("newPosition"),
        oldPosition: row.getResultByName("oldPosition"),
+        urlHref: row.getResultByName("url"),
      };
      observersToNotify.noteItemMoved(info);
    }
@ -4603,7 +4605,7 @@ class BookmarkObserverRecorder {
      isTagging: false,
      args: [info.id, info.oldParentId, info.oldPosition, info.newParentId,
        info.newPosition, info.type, info.guid, info.oldParentGuid,
-        info.newParentGuid, PlacesUtils.bookmarks.SOURCES.SYNC],
+        info.newParentGuid, PlacesUtils.bookmarks.SOURCES.SYNC, info.urlHref],
    });
  }

--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@ -246,6 +246,8 @@ interface nsINavBookmarkObserver : nsISupports
   * @param aSource
   *        A change source constant from nsINavBookmarksService::SOURCE_*,
   *        passed to the method that notifies the observer.
+   * @param aURI
+   *        The URI for this bookmark.
   */
  void onItemMoved(in long long aItemId,
                   in long long aOldParentId,
@ -256,7 +258,8 @@ interface nsINavBookmarkObserver : nsISupports
                   in ACString aGuid,
                   in ACString aOldParentGuid,
                   in ACString aNewParentGuid,
-                   in unsigned short aSource);
+                   in unsigned short aSource,
+                   in AUTF8String aURI);
 };

 /**
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@ -3032,7 +3032,8 @@ nsNavHistoryQueryResultNode::OnItemMoved(int64_t aFolder,
                                         const nsACString& aGUID,
                                         const nsACString& aOldParentGUID,
                                         const nsACString& aNewParentGUID,
-                                         uint16_t aSource)
+                                         uint16_t aSource,
+                                         const nsACString& aURI)
 {
  // 1. The query cannot be affected by the item's position
  // 2. For the time being, we cannot optimize this not to update
@ -3991,13 +3992,24 @@ nsNavHistoryFolderResultNode::OnItemMoved(int64_t aItemId,
                                          const nsACString& aGUID,
                                          const nsACString& aOldParentGUID,
                                          const nsACString& aNewParentGUID,
-                                          uint16_t aSource)
+                                          uint16_t aSource,
+                                          const nsACString& aURI)
 {
  NS_ASSERTION(aOldParent == mTargetFolderItemId || aNewParent == mTargetFolderItemId,
               "Got a bookmark message that doesn't belong to us");

  RESTART_AND_RETURN_IF_ASYNC_PENDING();

+  bool excludeItems = mOptions->ExcludeItems();
+  if (excludeItems &&
+      (aItemType == nsINavBookmarksService::TYPE_SEPARATOR ||
+       (aItemType == nsINavBookmarksService::TYPE_BOOKMARK &&
+        !StringBeginsWith(aURI, NS_LITERAL_CSTRING("place:"))))) {
+    // This is a bookmark or a separator, so we don't need to handle this if
+    // we're excluding items.
+    return NS_OK;
+  }
+
  uint32_t index;
  nsNavHistoryResultNode* node = FindChildById(aItemId, &index);
  // Bug 1097528.
@ -4010,12 +4022,6 @@ nsNavHistoryFolderResultNode::OnItemMoved(int64_t aItemId,
  if (!node && aOldParent == mTargetFolderItemId)
    return NS_OK;

-  bool excludeItems = mOptions->ExcludeItems();
-  if (node && excludeItems && (node->IsURI() || node->IsSeparator())) {
-    // Don't update items when we aren't displaying them.
-    return NS_OK;
-  }
-
  if (!StartIncrementalUpdate())
    return NS_OK; // entire container was refreshed for us

@ -4670,25 +4676,26 @@ nsNavHistoryResult::OnItemMoved(int64_t aItemId,
                                const nsACString& aGUID,
                                const nsACString& aOldParentGUID,
                                const nsACString& aNewParentGUID,
-                                uint16_t aSource)
+                                uint16_t aSource,
+                                const nsACString& aURI)
 {
  ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(aOldParent,
      OnItemMoved(aItemId, aOldParent, aOldIndex, aNewParent, aNewIndex,
-                  aItemType, aGUID, aOldParentGUID, aNewParentGUID, aSource));
+                  aItemType, aGUID, aOldParentGUID, aNewParentGUID, aSource, aURI));
  if (aNewParent != aOldParent) {
    ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(aNewParent,
        OnItemMoved(aItemId, aOldParent, aOldIndex, aNewParent, aNewIndex,
-                    aItemType, aGUID, aOldParentGUID, aNewParentGUID, aSource));
+                    aItemType, aGUID, aOldParentGUID, aNewParentGUID, aSource, aURI));
  }
  ENUMERATE_ALL_BOOKMARKS_OBSERVERS(OnItemMoved(aItemId, aOldParent, aOldIndex,
                                                aNewParent, aNewIndex,
                                                aItemType, aGUID,
                                                aOldParentGUID,
-                                                aNewParentGUID, aSource));
+                                                aNewParentGUID, aSource, aURI));
  ENUMERATE_HISTORY_OBSERVERS(OnItemMoved(aItemId, aOldParent, aOldIndex,
                                          aNewParent, aNewIndex, aItemType,
                                          aGUID, aOldParentGUID,
-                                          aNewParentGUID, aSource));
+                                          aNewParentGUID, aSource, aURI));
  return NS_OK;
 }

--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
@ -193,7 +193,8 @@ add_task(async function update_move_same_folder() {
  observer.check([ { name: "onItemMoved",
                     arguments: [ bmItemId, bmParentId, bmOldIndex, bmParentId, bm.index,
                                  bm.type, bm.guid, bm.parentGuid, bm.parentGuid,
-                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT,
+                                  "http://move.example.com/" ] }
                 ]);

  // Test that we get the right index for DEFAULT_INDEX input.
@ -206,7 +207,8 @@ add_task(async function update_move_same_folder() {
  observer.check([ { name: "onItemMoved",
                     arguments: [ bmItemId, bmParentId, bmOldIndex, bmParentId, bm.index,
                                  bm.type, bm.guid, bm.parentGuid, bm.parentGuid,
-                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT,
+                                  bm.url ] }
                 ]);
 });

@ -231,7 +233,8 @@ add_task(async function update_move_different_folder() {
                                  bm.index, bm.type, bm.guid,
                                  PlacesUtils.bookmarks.unfiledGuid,
                                  bm.parentGuid,
-                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT,
+                                  "http://move.example.com/" ] }
                 ]);
 });

@ -522,7 +525,8 @@ add_task(async function reorder_notification() {
                                              child.guid,
                                              child.parentGuid,
                                              child.parentGuid,
-                                              Ci.nsINavBookmarksService.SOURCE_DEFAULT
+                                              Ci.nsINavBookmarksService.SOURCE_DEFAULT,
+                                              child.url
                                            ] });
  }
  observer.check(expectedNotifications);
--- a/toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
+++ b/toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
@ -333,6 +333,7 @@ add_task(async function onItemMoved_bookmark() {
          { name: "oldParentGuid", check: v => typeof(v) == "string" && PlacesUtils.isValidGuid(v) },
          { name: "newParentGuid", check: v => typeof(v) == "string" && PlacesUtils.isValidGuid(v) },
          { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) },
+          { name: "url", check: v => typeof(v) == "string" },
        ] },
      { name: "onItemMoved",
        args: [
@ -346,6 +347,7 @@ add_task(async function onItemMoved_bookmark() {
          { name: "oldParentGuid", check: v => typeof(v) == "string" && PlacesUtils.isValidGuid(v) },
          { name: "newParentGuid", check: v => typeof(v) == "string" && PlacesUtils.isValidGuid(v) },
          { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) },
+          { name: "url", check: v => typeof(v) == "string" },
        ] },
  ])]);
  await PlacesUtils.bookmarks.update({
--- a/toolkit/components/places/tests/sync/head_sync.js
+++ b/toolkit/components/places/tests/sync/head_sync.js
@ -253,11 +253,11 @@ BookmarkObserver.prototype = {
  },
  onItemVisited() {},
  onItemMoved(itemId, oldParentId, oldIndex, newParentId, newIndex, type, guid,
-              oldParentGuid, newParentGuid, source) {
+              oldParentGuid, newParentGuid, source, uri) {
    this.notifications.push({
      name: "onItemMoved",
      params: { itemId, oldParentId, oldIndex, newParentId, newIndex, type,
-                guid, oldParentGuid, newParentGuid, source },
+                guid, oldParentGuid, newParentGuid, source, uri },
    });
  },

--- a/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
@ -115,7 +115,8 @@ add_task(async function test_value_structure_conflict() {
              guid: "bookmarkEEEE",
              oldParentGuid: "folderDDDDDD",
              newParentGuid: "folderDDDDDD",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/e" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("bookmarkBBBB"),
@ -125,7 +126,8 @@ add_task(async function test_value_structure_conflict() {
              guid: "bookmarkBBBB",
              oldParentGuid: "folderDDDDDD",
              newParentGuid: "folderDDDDDD",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/b" },
  }, {
    name: "onItemChanged",
    params: { itemId: localItemIds.get("folderDDDDDD"), property: "title",
@ -289,7 +291,8 @@ add_task(async function test_move() {
              guid: "devFolder___",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: PlacesUtils.bookmarks.toolbarGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: null },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("mozFolder___"),
@ -299,7 +302,8 @@ add_task(async function test_move() {
              guid: "mozFolder___",
              oldParentGuid: "devFolder___",
              newParentGuid: PlacesUtils.bookmarks.unfiledGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: null },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("bzBmk_______"),
@ -309,7 +313,8 @@ add_task(async function test_move() {
              guid: "bzBmk_______",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: "devFolder___",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "https://bugzilla.mozilla.org/" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("wmBmk_______"),
@ -319,7 +324,8 @@ add_task(async function test_move() {
              guid: "wmBmk_______",
              oldParentGuid: "devFolder___",
              newParentGuid: "devFolder___",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "https://webmaker.org/" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("nightlyBmk__"),
@ -329,7 +335,8 @@ add_task(async function test_move() {
              guid: "nightlyBmk__",
              oldParentGuid: "mozFolder___",
              newParentGuid: "mozFolder___",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "https://nightly.mozilla.org/" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("mdnBmk______"),
@ -339,7 +346,8 @@ add_task(async function test_move() {
              guid: "mdnBmk______",
              oldParentGuid: "devFolder___",
              newParentGuid: "mozFolder___",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "https://developer.mozilla.org/" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("fxBmk_______"),
@ -349,7 +357,8 @@ add_task(async function test_move() {
              guid: "fxBmk_______",
              oldParentGuid: "mozFolder___",
              newParentGuid: "mozFolder___",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://getfirefox.com/" },
  }]);

  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
@ -513,7 +522,8 @@ add_task(async function test_move_into_parent_sibling() {
              guid: "bookmarkBBBB",
              oldParentGuid: "folderAAAAAA",
              newParentGuid: "folderCCCCCC",
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/b" },
  }]);

  await assertLocalTree(PlacesUtils.bookmarks.menuGuid, {
@ -666,7 +676,8 @@ add_task(async function test_complex_move_with_additions() {
              guid: "bookmarkCCCC",
              oldParentGuid: "folderAAAAAA",
              newParentGuid: PlacesUtils.bookmarks.menuGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/c" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("folderAAAAAA"),
@ -676,7 +687,8 @@ add_task(async function test_complex_move_with_additions() {
              guid: "folderAAAAAA",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: PlacesUtils.bookmarks.toolbarGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: null },
  }]);

  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
--- a/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
@ -145,7 +145,8 @@ add_task(async function test_value_combo() {
              guid: "bzBmk_______",
              oldParentGuid: PlacesUtils.bookmarks.toolbarGuid,
              newParentGuid: PlacesUtils.bookmarks.toolbarGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "https://bugzilla.mozilla.org/" },
  }, {
    name: "onItemChanged",
    params: { itemId: localItemIds.get("mozBmk______"), property: "title",
@ -708,7 +709,8 @@ add_task(async function test_keywords_complex() {
              guid: "bookmarkBBBB",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: PlacesUtils.bookmarks.menuGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/b" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("bookmarkCCCC"),
@ -718,7 +720,8 @@ add_task(async function test_keywords_complex() {
              guid: "bookmarkCCCC",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: PlacesUtils.bookmarks.menuGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/c-remote" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("bookmarkDDDD"),
@ -728,7 +731,8 @@ add_task(async function test_keywords_complex() {
              guid: "bookmarkDDDD",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: PlacesUtils.bookmarks.menuGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/d" },
  }, {
    name: "onItemMoved",
    params: { itemId: localItemIds.get("bookmarkEEEE"),
@ -738,7 +742,8 @@ add_task(async function test_keywords_complex() {
              guid: "bookmarkEEEE",
              oldParentGuid: PlacesUtils.bookmarks.menuGuid,
              newParentGuid: PlacesUtils.bookmarks.menuGuid,
-              source: PlacesUtils.bookmarks.SOURCES.SYNC },
+              source: PlacesUtils.bookmarks.SOURCES.SYNC,
+              uri: "http://example.com/e" },
  }, {
    name: "onItemChanged",
    params: { itemId: localItemIds.get("bookmarkCCCC"), property: "title",