Bug 1451352 - Rework copying bookmarks (via paste or dnd) to use insertTree to improve performance. draft
authorMark Banner <standard8@mozilla.com>
Wed, 04 Apr 2018 08:05:08 +0100
changeset 777277 df98be4dd33581a11f7a7e8712bb8d9ad17f236c
parent 777163 ff0efa4132f0efd78af0910762aec7dcc1a8de66
push id105140
push userbmo:standard8@mozilla.com
push dateWed, 04 Apr 2018 14:45:17 +0000
bugs1451352
milestone61.0a1
Bug 1451352 - Rework copying bookmarks (via paste or dnd) to use insertTree to improve performance. MozReview-Commit-ID: 5oC0ASheHf7
browser/components/places/PlacesUIUtils.jsm
browser/components/places/tests/browser/browser_paste_bookmarks.js
toolkit/components/places/PlacesTransactions.jsm
toolkit/components/places/tests/unit/test_async_transactions.js
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -1248,55 +1248,51 @@ async function getTransactionsForMove(it
  * @param {Array} items A list of unwrapped nodes to get transactions for.
  * @param {Integer} insertionIndex The requested index for insertion.
  * @param {String} insertionParentGuid The guid of the parent folder to insert
  *                                     or move the items to.
  * @return {Array} Returns an array of created PlacesTransactions.
  */
 async function getTransactionsForCopy(items, insertionIndex,
                                       insertionParentGuid) {
-  let transactions = [];
   let index = insertionIndex;
+  let children = [];
 
   for (let item of items) {
-    let transaction;
     let guid = item.itemGuid;
 
     if (PlacesUIUtils.PLACES_FLAVORS.includes(item.type) &&
         // For anything that is comming from within this session, we do a
         // direct copy, otherwise we fallback and form a new item below.
         "instanceId" in item && item.instanceId == PlacesUtils.instanceId &&
         // If the Item doesn't have a guid, this could be a virtual tag query or
         // other item, so fallback to inserting a new bookmark with the URI.
         guid &&
         // For virtual root items, we fallback to creating a new bookmark, as
         // we want a shortcut to be created, not a full tree copy.
         !PlacesUtils.bookmarks.isVirtualRootItem(guid) &&
         !PlacesUtils.isVirtualLeftPaneItem(guid)) {
-      transaction = PlacesTransactions.Copy({
-        excludingAnnotation: "Places/SmartBookmark",
-        guid,
-        newIndex: index,
-        newParentGuid: insertionParentGuid,
-      });
+      // XXX Can't promiseBookmarksTree has the wrong style of data for insertTree,
+      // is it time for fetchTree?
+      // children.push(await PlacesUtils.promiseBookmarksTree(guid));
     } else if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
-      transaction = PlacesTransactions.NewSeparator({
+      children.push({
         index,
-        parentGuid: insertionParentGuid,
+        type: PlacesUtils.bookmarks.TYPE_SEPARATOR
       });
     } else {
       let title = item.type != PlacesUtils.TYPE_UNICODE ? item.title : item.uri;
-      transaction = PlacesTransactions.NewBookmark({
+      children.push({
         index,
-        parentGuid: insertionParentGuid,
         title,
         url: item.uri,
       });
     }
 
-    transactions.push(transaction);
-
     if (index != -1) {
       index++;
     }
   }
-  return transactions;
+  return [PlacesTransactions.InsertTree({
+    guid: insertionParentGuid,
+    children
+  })];
 }
--- a/browser/components/places/tests/browser/browser_paste_bookmarks.js
+++ b/browser/components/places/tests/browser/browser_paste_bookmarks.js
@@ -207,17 +207,17 @@ add_task(async function paste_from_diffe
   let data = {
     "title": "test",
     "id": 32,
     "instanceId": "FAKEFAKEFAKE",
     "itemGuid": "ZBf_TYkrYGvW",
     "parent": 452,
     "dateAdded": 1464866275853000,
     "lastModified": 1507638113352000,
-    "type": "text/x-moz-place",
+    "type": PlacesUtils.TYPE_X_MOZ_PLACE,
     "uri": TEST_URL1
   };
   data = JSON.stringify(data);
 
   xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
   xferable.setTransferData(PlacesUtils.TYPE_X_MOZ_PLACE, PlacesUtils.toISupportsString(data),
                            data.length * 2);
 
@@ -235,8 +235,127 @@ add_task(async function paste_from_diffe
                "Should be one bookmark in the unfiled folder.");
   Assert.equal(tree.children[0].title, "test",
                "Should have the correct title");
   Assert.equal(tree.children[0].uri, TEST_URL1,
                "Should have the correct URL");
 
   await PlacesUtils.bookmarks.remove(tree.children[0].guid);
 });
+
+add_task(async function paste_separator_from_different_instance() {
+  let xferable = Cc["@mozilla.org/widget/transferable;1"]
+                   .createInstance(Ci.nsITransferable);
+  xferable.init(null);
+
+  // Fake data on the clipboard to pretend this is from a different instance
+  // of Firefox.
+  let data = {
+    "title": "test",
+    "id": 32,
+    "instanceId": "FAKEFAKEFAKE",
+    "itemGuid": "ZBf_TYkrYGvW",
+    "parent": 452,
+    "dateAdded": 1464866275853000,
+    "lastModified": 1507638113352000,
+    "type": PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR
+  };
+  data = JSON.stringify(data);
+
+  xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
+  xferable.setTransferData(PlacesUtils.TYPE_X_MOZ_PLACE, PlacesUtils.toISupportsString(data),
+                           data.length * 2);
+
+  Services.clipboard.setData(xferable, null, Ci.nsIClipboard.kGlobalClipboard);
+
+  info("Selecting UnfiledBookmarks in the left pane");
+  PlacesOrganizer.selectLeftPaneBuiltIn("UnfiledBookmarks");
+
+  info("Pasting clipboard");
+  await ContentTree.view.controller.paste();
+
+  let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
+
+  Assert.equal(tree.children.length, 1,
+               "Should be one bookmark in the unfiled folder.");
+  Assert.equal(tree.children[0].type, PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
+               "Should have the correct type");
+
+  await PlacesUtils.bookmarks.remove(tree.children[0].guid);
+});
+
+// XXX fixme
+add_task(async function paste_copy_check_indexes() {
+  info("Selecting BookmarksToolbar in the left pane");
+  PlacesOrganizer.selectLeftPaneBuiltIn("BookmarksToolbar");
+
+  let copyChildren = [];
+  let targetChildren = [];
+  for (let i = 0; i < 10; i++) {
+    copyChildren.push({
+      url: `${TEST_URL}${i}`,
+      title: `Copy ${i}`
+    });
+    targetChildren.push({
+      url: `${TEST_URL1}${i}`,
+      title: `Target ${i}`
+    });
+  }
+
+  let copyBookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    children: copyChildren
+  });
+
+  let targetBookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: targetChildren
+  });
+
+  ContentTree.view.selectItems([
+    copyBookmarks[0].guid,
+    copyBookmarks[3].guid,
+    copyBookmarks[6].guid,
+    copyBookmarks[9].guid,
+  ]);
+
+  await promiseClipboard(() => {
+    info("Cutting multiple selection");
+    ContentTree.view.controller.copy();
+  }, PlacesUtils.TYPE_X_MOZ_PLACE);
+
+  info("Selecting UnfiledBookmarks in the left pane");
+  PlacesOrganizer.selectLeftPaneBuiltIn("UnfiledBookmarks");
+
+  ContentTree.view.selectItems([targetBookmarks[4].guid]);
+
+  info("Pasting clipboard");
+  await ContentTree.view.controller.paste();
+
+  let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
+
+  const expectedBookmarkOrder = [
+    targetBookmarks[0].guid,
+    targetBookmarks[1].guid,
+    targetBookmarks[2].guid,
+    targetBookmarks[3].guid,
+    copyBookmarks[0].guid,
+    copyBookmarks[3].guid,
+    copyBookmarks[6].guid,
+    copyBookmarks[9].guid,
+    targetBookmarks[4].guid,
+    targetBookmarks[5].guid,
+    targetBookmarks[6].guid,
+    targetBookmarks[7].guid,
+    targetBookmarks[8].guid,
+    targetBookmarks[9].guid,
+  ];
+
+  Assert.equal(tree.children.length, expectedBookmarkOrder.length,
+               "Should be the expected amount of bookmarks in the unfiled folder.");
+
+  for (let i = 0; i < expectedBookmarkOrder.length; ++i) {
+    Assert.equal(tree.children[i].guid, expectedBookmarkOrder[i],
+                 `Should be the expected item at index ${i}`);
+  }
+
+  await PlacesUtils.bookmarks.eraseEverything();
+});
--- a/toolkit/components/places/PlacesTransactions.jsm
+++ b/toolkit/components/places/PlacesTransactions.jsm
@@ -178,16 +178,22 @@ const TRANSACTIONS_QUEUE_TIMEOUT_MS = 24
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.defineModuleGetter(this, "PlacesUtils",
                                "resource://gre/modules/PlacesUtils.jsm");
 
 Cu.importGlobalProperties(["URL"]);
 
+const BOOKMARK_TYPES = [
+  PlacesUtils.bookmarks.TYPE_BOOKMARK,
+  PlacesUtils.bookmarks.TYPE_FOLDER,
+  PlacesUtils.bookmarks.TYPE_SEPARATOR,
+];
+
 function setTimeout(callback, ms) {
   let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   timer.initWithCallback(callback, ms, timer.TYPE_ONE_SHOT);
 }
 
 class TransactionsHistoryArray extends Array {
   constructor() {
     super();
@@ -744,22 +750,23 @@ DefineTransaction.annotationObjectValida
     }
   }
   throw new Error("Invalid annotation object");
 };
 
 DefineTransaction.childObjectValidate = function(obj) {
   if (obj &&
       checkProperty(obj, "title", false, v => typeof(v) == "string") &&
-      !("type" in obj && obj.type != PlacesUtils.bookmarks.TYPE_BOOKMARK)) {
-    obj.url = DefineTransaction.urlValidate(obj.url);
-    let validKeys = ["title", "url"];
-    if (Object.keys(obj).every(k => validKeys.includes(k))) {
-      return obj;
+      checkProperty(obj, "type", false, v => BOOKMARK_TYPES.includes(v))) {
+    if ("type" in obj && obj.type == PlacesUtils.bookmarks.TYPE_FOLDER) {
+      obj.children = DefineTransaction.validatePropertyValue("children", obj, true);
     }
+    // Note: we don't validate all items here - we let the bookmarks API validate
+    // them for us to save duplication.
+    return obj;
   }
   throw new Error("Invalid child object");
 };
 
 DefineTransaction.urlValidate = function(url) {
   if (url instanceof Ci.nsIURI)
     return new URL(url.spec);
   return new URL(url);
@@ -1744,8 +1751,63 @@ PT.Copy.prototype = {
     };
     this.redo = async function() {
       await createItemsFromBookmarksTree(newItemInfo, true);
     };
 
     return newItemGuid;
   }
 };
+
+/**
+ * Transaction for inserting a tree of items.
+ *
+ * Required Input Properties: guid, children
+ */
+PT.InsertTree = DefineTransaction(["guid", "children"]);
+PT.InsertTree.prototype = {
+  async execute({guid, children}) {
+    let bms = await PlacesUtils.bookmarks.insertTree({
+      guid, children
+    });
+
+    this.undo = async function() {
+      for (let bm of bms) {
+        try {
+          await PlacesUtils.bookmarks.remove(bm.guid);
+        } catch (ex) {
+          if (!ex.message.includes("No bookmarks found for the provided GUID.")) {
+            throw ex;
+          }
+        }
+      }
+    };
+    this.redo = async function() {
+      // For re-inserting, we have to remap the bookmarks tree so that children
+      // are in the correct place.
+      if (bms.length > 1) {
+        let knownBms = new Map();
+        // We'll filter out non-top level items from the array.
+        bms = bms.filter(bm => {
+          knownBms.set(bm.guid, bm);
+          // If the item has a known parent, then we re-parent it within the
+          // tree.
+          if ("parentGuid" in bm && knownBms.has(bm.parentGuid)) {
+            let parent = knownBms.get(bm.parentGuid);
+            if ("children" in parent) {
+              parent.children.push(bm);
+            } else {
+              parent.children = [bm];
+            }
+            return false;
+          }
+          return true;
+        });
+      }
+
+      bms = await PlacesUtils.bookmarks.insertTree({
+        guid, children: bms
+      });
+    };
+
+    return bms.map(bm => bm.guid);
+  }
+};
--- a/toolkit/components/places/tests/unit/test_async_transactions.js
+++ b/toolkit/components/places/tests/unit/test_async_transactions.js
@@ -537,18 +537,18 @@ add_task(async function test_new_bookmar
                          parentGuid: bm_info.parentGuid,
                          index:      bmStartIndex });
     observer.reset();
   };
 
   await ensureDo();
   await PT.undo();
   ensureUndo();
-  await PT.redo(true);
-  await ensureDo();
+  await PT.redo();
+  await ensureDo(true);
   await PT.undo();
   ensureUndo();
 
   await PT.clearTransactionsHistory();
   ensureUndoState();
 });
 
 add_task(async function test_merge_create_folder_and_item() {
@@ -1918,8 +1918,130 @@ add_task(async function test_renameTag()
 
   await PT.undo();
   ensureTagsForURI(url, []);
 
   await PT.clearTransactionsHistory();
   ensureUndoState();
   await PlacesUtils.bookmarks.eraseEverything();
 });
+
+add_task(async function test_insertTree_singleBookmark() {
+  observer.reset();
+
+  let bmData = [{
+    url: "http://example.com",
+    title: "135246",
+  }];
+
+  let guids = await PT.InsertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: bmData
+  }).transact();
+
+  Assert.equal(guids.length, bmData.length,
+    "Should have inserted the correct amount of items");
+
+  ensureItemsAdded({guid: guids[0], parentGuid: PlacesUtils.bookmarks.unfiledGuid, ...bmData[0]});
+
+  let originalInfos = [];
+  for (let guid of guids) {
+    originalInfos.push(await PlacesUtils.promiseBookmarksTree(guid));
+  }
+  await PT.undo();
+  await ensureItemsRemoved(...guids);
+  await ensureNonExistent(...guids);
+  await PT.redo();
+  await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
+  ensureItemsAdded({guid: guids[0], parentGuid: PlacesUtils.bookmarks.unfiledGuid, ...bmData[0]});
+
+  await PlacesUtils.bookmarks.eraseEverything();
+});
+
+add_task(async function test_insertTree_multipleItems() {
+  observer.reset();
+
+  let bmData = [{
+    url: "http://example.com",
+    title: "135246",
+  }, {
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    title: "142536",
+    children: [{
+      url: "http://example.com/2",
+      title: "531246",
+    }],
+  }, {
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    title: "13572468",
+    children: [{
+      url: "http://example.com/3",
+      title: "15263748",
+    }, {
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      title: "12563478",
+      children: [{
+        url: "http://example.com/4",
+        title: "75312468",
+      }],
+    }],
+  }];
+
+  let guids = await PT.InsertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: bmData
+  }).transact();
+
+  Assert.equal(guids.length, 7,
+    "Should have inserted the correct amount of items");
+
+  const expectedAddedData = [{
+    guid: guids[0],
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: bmData[0].url,
+    title: bmData[0].title,
+  }, {
+    guid: guids[1],
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    title: bmData[1].title,
+    type: bmData[1].type,
+  }, {
+    guid: guids[2],
+    parentGuid: guids[1],
+    url: bmData[1].children[0].url,
+    title: bmData[1].children[0].title,
+  }, {
+    guid: guids[3],
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    title: bmData[2].title,
+    type: bmData[2].type,
+  }, {
+    guid: guids[4],
+    parentGuid: guids[3],
+    url: bmData[2].children[0].url,
+    title: bmData[2].children[0].title,
+  }, {
+    guid: guids[5],
+    parentGuid: guids[3],
+    title: bmData[2].children[1].title,
+    type: bmData[2].children[1].type,
+  }, {
+    guid: guids[6],
+    parentGuid: guids[5],
+    url: bmData[2].children[1].children[0].url,
+    title: bmData[2].children[1].children[0].title,
+  }];
+
+  ensureItemsAdded(...expectedAddedData);
+
+  let originalInfos = [];
+  for (let guid of guids) {
+    originalInfos.push(await PlacesUtils.promiseBookmarksTree(guid));
+  }
+  await PT.undo();
+  await ensureItemsRemoved(...guids);
+  await ensureNonExistent(...guids);
+  await PT.redo();
+  await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
+  ensureItemsAdded(...expectedAddedData);
+
+  await PlacesUtils.bookmarks.eraseEverything();
+});