Bug 1386513 - Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances. r?mak draft
authorMark Banner <standard8@mozilla.com>
Fri, 08 Sep 2017 07:01:29 +0100
changeset 678044 4431ee9e39559daa1d7963e4f07fa095eb80e7f1
parent 677139 4494c218fe4d16f1413d74b37b7fa178111951be
child 735220 b6ec742087dd8b2d107abf189ce1dc9bc15ff12e
push id83807
push userbmo:standard8@mozilla.com
push dateTue, 10 Oct 2017 19:36:09 +0000
reviewersmak
bugs1386513
milestone58.0a1
Bug 1386513 - Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances. r?mak MozReview-Commit-ID: Lv2DT0WQGhZ
browser/components/places/PlacesUIUtils.jsm
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_paste_bookmarks.js
toolkit/components/places/PlacesUtils.jsm
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -577,17 +577,18 @@ this.PlacesUIUtils = {
    *
    * @return  a Places Transaction that can be transacted for performing the
    *          move/insert command.
    */
   getTransactionForData(aData, aType, aNewParentGuid, aIndex, aCopy) {
     if (!this.SUPPORTED_FLAVORS.includes(aData.type))
       throw new Error(`Unsupported '${aData.type}' data type`);
 
-    if ("itemGuid" in aData) {
+    if ("itemGuid" in aData && "instanceId" in aData &&
+        aData.instanceId == PlacesUtils.instanceId) {
       if (!this.PLACES_FLAVORS.includes(aData.type))
         throw new Error(`itemGuid unexpectedly set on ${aData.type} data`);
 
       let info = { guid: aData.itemGuid,
                    newParentGuid: aNewParentGuid,
                    newIndex: aIndex };
       if (aCopy) {
         info.excludingAnnotation = "Places/SmartBookmark";
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -48,16 +48,18 @@ subsuite = clipboard
 [browser_library_middleclick.js]
 [browser_library_move_bookmarks.js]
 [browser_library_open_leak.js]
 [browser_library_openFlatContainer.js]
 [browser_library_panel_leak.js]
 [browser_library_search.js]
 [browser_library_views_liveupdate.js]
 [browser_markPageAsFollowedLink.js]
+[browser_paste_bookmarks.js]
+subsuite = clipboard
 [browser_paste_into_tags.js]
 subsuite = clipboard
 [browser_sidebarpanels_click.js]
 skip-if = true # temporarily disabled for breaking the treeview - bug 658744
 [browser_sort_in_library.js]
 [browser_stayopenmenu.js]
 [browser_toolbar_drop_text.js]
 [browser_toolbar_overflow.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_paste_bookmarks.js
@@ -0,0 +1,103 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_URL = "http://example.com/";
+const TEST_URL1 = "https://example.com/otherbrowser";
+
+var PlacesOrganizer;
+var ContentTree;
+var bookmark;
+var bookmarkId;
+
+add_task(async function setup() {
+  await PlacesUtils.bookmarks.eraseEverything();
+  let organizer = await promiseLibrary();
+
+  registerCleanupFunction(async function() {
+    await promiseLibraryClosed(organizer);
+    await PlacesUtils.bookmarks.eraseEverything();
+  });
+
+  PlacesOrganizer = organizer.PlacesOrganizer;
+  ContentTree = organizer.ContentTree;
+
+  info("Selecting BookmarksToolbar in the left pane");
+  PlacesOrganizer.selectLeftPaneQuery("BookmarksToolbar");
+
+  bookmark = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    url: TEST_URL,
+    title: "0"
+  });
+  bookmarkId = await PlacesUtils.promiseItemId(bookmark.guid);
+
+  ContentTree.view.selectItems([bookmarkId]);
+
+  await promiseClipboard(() => {
+    info("Copying selection");
+    ContentTree.view.controller.cut();
+  }, PlacesUtils.TYPE_X_MOZ_PLACE);
+});
+
+add_task(async function paste() {
+  info("Selecting UnfiledBookmarks in the left pane");
+  PlacesOrganizer.selectLeftPaneQuery("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].title, "0",
+               "Should have the correct title");
+  Assert.equal(tree.children[0].uri, TEST_URL,
+               "Should have the correct URL");
+
+  await PlacesUtils.bookmarks.remove(tree.children[0].guid);
+});
+
+add_task(async function paste_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": "text/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);
+
+  Services.clipboard.setData(xferable, null, Ci.nsIClipboard.kGlobalClipboard);
+
+  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].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);
+});
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -130,16 +130,19 @@ async function notifyKeywordChange(url, 
  *        Whether the node represents a livemark.
  */
 function serializeNode(aNode, aIsLivemark) {
   let data = {};
 
   data.title = aNode.title;
   data.id = aNode.itemId;
   data.livemark = aIsLivemark;
+  // Add an instanceId so we can tell which instance of an FF session the data
+  // is coming from.
+  data.instanceId = PlacesUtils.instanceId;
 
   let guid = aNode.bookmarkGuid;
   if (guid) {
     data.itemGuid = guid;
     if (aNode.parent)
       data.parent = aNode.parent.itemId;
     let grandParent = aNode.parent && aNode.parent.parent;
     if (grandParent)
@@ -2010,16 +2013,21 @@ XPCOMUtils.defineLazyGetter(PlacesUtils,
 
 XPCOMUtils.defineLazyGetter(this, "bundle", function() {
   const PLACES_STRING_BUNDLE_URI = "chrome://places/locale/places.properties";
   return Cc["@mozilla.org/intl/stringbundle;1"].
          getService(Ci.nsIStringBundleService).
          createBundle(PLACES_STRING_BUNDLE_URI);
 });
 
+// This is just used as a reasonably-random value for copy & paste / drag operations.
+XPCOMUtils.defineLazyGetter(PlacesUtils, "instanceId", () => {
+  return PlacesUtils.history.makeGuid();
+});
+
 /**
  * Setup internal databases for closing properly during shutdown.
  *
  * 1. Places initiates shutdown.
  * 2. Before places can move to the step where it closes the low-level connection,
  *   we need to make sure that we have closed `conn`.
  * 3. Before we can close `conn`, we need to make sure that all external clients
  *   have stopped using `conn`.