Bug 1410940 - Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other. r=mak
☠☠ backed out by c08ecbfbb509 ☠ ☠
authorMark Banner <standard8@mozilla.com>
Mon, 23 Oct 2017 15:50:52 +0100
changeset 440759 37ceb314b2b3ae5c629a028dd5ac46c222015a56
parent 440758 f7206a54b9aaa37e811b91d8bb07f0901280bac5
child 440760 1c78029ded02384d293d27421ec5cce979f3d391
push id8120
push userryanvm@gmail.com
push dateSat, 04 Nov 2017 17:45:29 +0000
treeherdermozilla-beta@78568f0b1068 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1410940
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1410940 - Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other. r=mak MozReview-Commit-ID: EEFizNPmKpr
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1330,29 +1330,29 @@ PlacesController.prototype = {
           // source, otherwise report an error and fallback to a copy.
           if (!doCopy &&
               !PlacesControllerDragHelper.canMoveUnwrappedNode(item)) {
             Components.utils.reportError("Tried to move an unmovable " +
                            "Places node, reverting to a copy operation.");
             doCopy = true;
           }
 
-          transactionData.push([item, type, parent, insertionIndex, doCopy]);
+          transactionData.push(PlacesUIUtils.getTransactionForData(
+            item, type, parent, insertionIndex, doCopy));
 
           // Adjust index to make sure items are pasted in the correct
           // position.  If index is DEFAULT_INDEX, items are just appended.
           if (insertionIndex != PlacesUtils.bookmarks.DEFAULT_INDEX)
             insertionIndex++;
         }
 
         await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactionData.length, async () => {
           await PlacesTransactions.batch(async () => {
-            for (let item of transactionData) {
-              let guid = await PlacesUIUtils.getTransactionForData(
-                ...item).transact();
+            for (let transaction of transactionData) {
+              let guid = await transaction.transact();
               itemsToSelect.push(await PlacesUtils.promiseItemId(guid));
             }
           });
         });
       }
     } else {
       let transactions = [];
       let insertionIndex = await ip.getIndex();
@@ -1671,79 +1671,103 @@ var PlacesControllerDragHelper = {
         let uri = data.linkedBrowser.currentURI;
         let spec = uri ? uri.spec : "about:blank";
         nodes = [{ uri: spec,
                    title: data.label,
                    type: PlacesUtils.TYPE_X_MOZ_URL}];
       } else
         throw new Error("bogus data was passed as a tab");
 
-      for (let unwrapped of nodes) {
-        let index = await insertionPoint.getIndex();
+      if (PlacesUIUtils.useAsyncTransactions) {
+        // If dragging over a tag container we should tag the item.
+        if (insertionPoint.isTag) {
+          let urls = nodes.filter(item => "uri" in item).map(item => item.uri);
+          transactions.push(PlacesTransactions.Tag({ urls, tag: tagName }));
+        } else {
+          for (let unwrapped of nodes) {
+            let index = await insertionPoint.getIndex();
 
-        if (index != -1 && unwrapped.itemGuid) {
-          // Note: we use the parent from the existing bookmark as the sidebar
-          // gives us an unwrapped.parent that is actually a query and not the real
-          // parent.
-          let existingBookmark = await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid);
+            if (index != -1 && unwrapped.itemGuid) {
+              // Note: we use the parent from the existing bookmark as the sidebar
+              // gives us an unwrapped.parent that is actually a query and not the real
+              // parent.
+              let existingBookmark = await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid);
 
-          // If we're dropping on the same folder, then we may need to adjust
-          // the index to insert at the correct place.
-          if (existingBookmark && parentGuid == existingBookmark.parentGuid) {
-            if (PlacesUIUtils.useAsyncTransactions) {
-              if (index < existingBookmark.index) {
-                // When you drag multiple elts upward: need to increment index or
-                // each successive elt will be inserted at the same index, each
-                // above the previous.
-                index += movedCount++;
-              } else if (index > existingBookmark.index) {
-                // If we're dragging down, we need to go one lower to insert at
-                // the real point as moving the element changes the index of
-                // everything below by 1.
-                index--;
-              } else {
-                // This isn't moving so we skip it.
-                continue;
+              // If we're dropping on the same folder, then we may need to adjust
+              // the index to insert at the correct place.
+              if (existingBookmark && parentGuid == existingBookmark.parentGuid) {
+                if (index < existingBookmark.index) {
+                  // When you drag multiple elts upward: need to increment index or
+                  // each successive elt will be inserted at the same index, each
+                  // above the previous.
+                  index += movedCount++;
+                } else if (index > existingBookmark.index) {
+                  // If we're dragging down, we need to go one lower to insert at
+                  // the real point as moving the element changes the index of
+                  // everything below by 1.
+                  index--;
+                } else {
+                  // This isn't moving so we skip it.
+                  continue;
+                }
               }
-            } else {
+            }
+
+            // If this is not a copy, check for safety that we can move the
+            // source, otherwise report an error and fallback to a copy.
+            if (!doCopy && !PlacesControllerDragHelper.canMoveUnwrappedNode(unwrapped)) {
+              Components.utils.reportError("Tried to move an unmovable Places " +
+                                           "node, reverting to a copy operation.");
+              doCopy = true;
+            }
+            transactions.push(
+              PlacesUIUtils.getTransactionForData(unwrapped,
+                                                  flavor,
+                                                  parentGuid,
+                                                  index,
+                                                  doCopy));
+          }
+        }
+      } else {
+        for (let unwrapped of nodes) {
+          let index = await insertionPoint.getIndex();
+
+          if (index != -1 && unwrapped.itemGuid) {
+            // Note: we use the parent from the existing bookmark as the sidebar
+            // gives us an unwrapped.parent that is actually a query and not the real
+            // parent.
+            let existingBookmark = await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid);
+
+            // If we're dropping on the same folder, then we may need to adjust
+            // the index to insert at the correct place.
+            if (existingBookmark && parentGuid == existingBookmark.parentGuid) {
               // Sync Transactions. Adjust insertion index to prevent reversal
               // of dragged items. When you drag multiple elts upward: need to
               // increment index or each successive elt will be inserted at the
               // same index, each above the previous.
               if (index < existingBookmark.index) { // eslint-disable-line no-lonely-if
                 index += movedCount++;
               }
             }
           }
-        }
 
-        // If dragging over a tag container we should tag the item.
-        if (insertionPoint.isTag) {
-          let uri = NetUtil.newURI(unwrapped.uri);
-          let tagItemId = insertionPoint.itemId;
-          if (PlacesUIUtils.useAsyncTransactions)
-            transactions.push(PlacesTransactions.Tag({ uri, tag: tagName }));
-          else
+          // If dragging over a tag container we should tag the item.
+          // eslint-disable-next-line no-lonely-if
+          if (insertionPoint.isTag) {
+            let uri = NetUtil.newURI(unwrapped.uri);
+            let tagItemId = insertionPoint.itemId;
             transactions.push(new PlacesTagURITransaction(uri, [tagItemId]));
-        } else {
-          // If this is not a copy, check for safety that we can move the
-          // source, otherwise report an error and fallback to a copy.
-          if (!doCopy && !PlacesControllerDragHelper.canMoveUnwrappedNode(unwrapped)) {
-            Components.utils.reportError("Tried to move an unmovable Places " +
-                                         "node, reverting to a copy operation.");
-            doCopy = true;
-          }
-          if (PlacesUIUtils.useAsyncTransactions) {
-            transactions.push(
-              PlacesUIUtils.getTransactionForData(unwrapped,
-                                                  flavor,
-                                                  parentGuid,
-                                                  index,
-                                                  doCopy));
           } else {
+            // If this is not a copy, check for safety that we can move the
+            // source, otherwise report an error and fallback to a copy.
+            if (!doCopy && !PlacesControllerDragHelper.canMoveUnwrappedNode(unwrapped)) {
+              Components.utils.reportError("Tried to move an unmovable Places " +
+                                           "node, reverting to a copy operation.");
+              doCopy = true;
+            }
             transactions.push(PlacesUIUtils.makeTransaction(unwrapped,
                                 flavor, insertionPoint.itemId,
                                 index, doCopy));
           }
         }
       }
     }
     // Check if we actually have something to add, if we don't it probably wasn't
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -22,16 +22,17 @@ support-files =
 [browser_bookmarkProperties_addLivemark.js]
 [browser_bookmarkProperties_bookmarkAllTabs.js]
 [browser_bookmarkProperties_cancel.js]
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 [browser_bookmarksProperties.js]
 [browser_check_correct_controllers.js]
 [browser_click_bookmarks_on_toolbar.js]
+[browser_controller_onDrop_tagFolder.js]
 [browser_controller_onDrop.js]
 [browser_copy_folder_tree.js]
 [browser_copy_query_without_tree.js]
 subsuite = clipboard
 [browser_cutting_bookmarks.js]
 subsuite = clipboard
 [browser_drag_bookmarks_on_toolbar.js]
 [browser_forgetthissite_single.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
@@ -0,0 +1,112 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+/* global sinon */
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
+
+const sandbox = sinon.sandbox.create();
+const TAG_NAME = "testTag";
+
+var bookmarks;
+var bookmarkId;
+
+add_task(async function setup() {
+  registerCleanupFunction(async function() {
+    sandbox.restore();
+    delete window.sinon;
+    await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesTestUtils.clearHistory();
+  });
+
+  sandbox.stub(PlacesTransactions, "batch");
+  sandbox.stub(PlacesTransactions, "Tag");
+
+  bookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: [{
+      title: "bm1",
+      url: "http://example1.com"
+    }, {
+      title: "bm2",
+      url: "http://example2.com",
+      tags: [TAG_NAME]
+    }]
+  });
+  bookmarkId = await PlacesUtils.promiseItemId(bookmarks[0].guid);
+});
+
+async function run_drag_test(startBookmarkIndex, newParentGuid) {
+  if (!PlacesUIUtils.useAsyncTransactions) {
+    Assert.ok(true, "Skipping test as async transactions are turned off");
+    return;
+  }
+
+  if (!newParentGuid) {
+    newParentGuid = PlacesUtils.bookmarks.unfiledGuid;
+  }
+
+  // Reset the stubs so that previous test runs don't count against us.
+  PlacesTransactions.Tag.reset();
+  PlacesTransactions.batch.reset();
+
+  let dragBookmark = bookmarks[startBookmarkIndex];
+
+  await withSidebarTree("bookmarks", async (tree) => {
+    tree.selectItems([PlacesUtils.unfiledBookmarksFolderId]);
+    PlacesUtils.asContainer(tree.selectedNode).containerOpen = true;
+
+    // Simulating a drag-drop with a tree view turns out to be really difficult
+    // as you can't get a node for the source/target. Hence, we fake the
+    // insertion point and drag data and call the function direct.
+    let ip = new InsertionPoint({
+      isTag: true,
+      tagName: TAG_NAME,
+      orientation: Ci.nsITreeView.DROP_ON
+    });
+
+    let bookmarkWithId = JSON.stringify(Object.assign({
+      id: bookmarkId,
+      itemGuid: dragBookmark.guid,
+      parent: PlacesUtils.unfiledBookmarksFolderId,
+      uri: dragBookmark.url
+    }, dragBookmark));
+
+    let dt = {
+      dropEffect: "move",
+      mozCursor: "auto",
+      mozItemCount: 1,
+      types: [ PlacesUtils.TYPE_X_MOZ_PLACE ],
+      mozTypesAt(i) {
+        return this.types;
+      },
+      mozGetDataAt(i) {
+        return bookmarkWithId;
+      }
+    };
+
+    await PlacesControllerDragHelper.onDrop(ip, dt);
+
+    Assert.ok(PlacesTransactions.Tag.calledOnce,
+      "Should have called getTransactionForData at least once.");
+
+    let arg = PlacesTransactions.Tag.args[0][0];
+
+    Assert.equal(arg.urls.length, 1,
+      "Should have called PlacesTransactions.Tag with an array of one url");
+    Assert.equal(arg.urls[0].spec, dragBookmark.url,
+      "Should have called PlacesTransactions.Tag with the correct url");
+    Assert.equal(arg.tag, TAG_NAME,
+      "Should have called PlacesTransactions.Tag with the correct tag name");
+  });
+}
+
+add_task(async function test_simple_drop_and_tag() {
+  // When we move items down the list, we'll get a drag index that is one higher
+  // than where we actually want to insert to - as the item is being moved up,
+  // everything shifts down one. Hence the index to pass to the transaction should
+  // be one less than the supplied index.
+  await run_drag_test(0, PlacesUtils.bookmarks.tagGuid);
+});