Bug 1410940 - Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other. r?mak draft
authorMark Banner <standard8@mozilla.com>
Mon, 23 Oct 2017 15:50:52 +0100
changeset 692671 24b8a93f00c7895493fc35979676ee66965d9064
parent 692670 4e6df5159df3e64c3c453c0db0a2e1d126819b71
child 692672 01d69fbad9fe0878fcdfeb307a701e4d9d206b53
push id87562
push userbmo:standard8@mozilla.com
push dateFri, 03 Nov 2017 11:18:20 +0000
reviewersmak
bugs1410940
milestone58.0a1
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], 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);
+});