Bug 1439697 - Re-organise the copy/paste & drag/drop algorithm to make it easier to understand. r?mak draft
authorMark Banner <standard8@mozilla.com>
Tue, 27 Mar 2018 22:49:01 +0100
changeset 774794 e59dfb1d295f263369a3530ff7374476e3a43823
parent 774444 6aa3b57955fed5e137d0306478e1a4b424a6d392
child 774795 aaabb0d5c6e3bf876bae046adf06704308fa123a
push id104505
push userbmo:standard8@mozilla.com
push dateThu, 29 Mar 2018 15:40:27 +0000
reviewersmak
bugs1439697
milestone61.0a1
Bug 1439697 - Re-organise the copy/paste & drag/drop algorithm to make it easier to understand. r?mak MozReview-Commit-ID: BrQEU2e4Nr0
browser/components/places/PlacesUIUtils.jsm
browser/components/places/tests/browser/browser_controller_onDrop.js
browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -917,71 +917,16 @@ var PlacesUIUtils = {
     } finally {
       if (itemsBeingChanged > ITEM_CHANGED_BATCH_NOTIFICATION_THRESHOLD) {
         resultNode.onEndUpdateBatch();
       }
     }
   },
 
   /**
-   * Constructs a Places Transaction for the drop or paste of a blob of data
-   * into a container.
-   *
-   * @param   aData
-   *          The unwrapped data blob of dropped or pasted data.
-   * @param   aNewParentGuid
-   *          GUID of the container the data was dropped or pasted into.
-   * @param   aIndex
-   *          The index within the container the item was dropped or pasted at.
-   * @param   aCopy
-   *          The drag action was copy, so don't move folders or links.
-   *
-   * @return  a Places Transaction that can be transacted for performing the
-   *          move/insert command.
-   */
-  getTransactionForData(aData, aNewParentGuid, aIndex, aCopy) {
-    if (!this.SUPPORTED_FLAVORS.includes(aData.type))
-      throw new Error(`Unsupported '${aData.type}' data type`);
-
-    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";
-        return PlacesTransactions.Copy(info);
-      }
-      return PlacesTransactions.Move(info);
-    }
-
-    // Since it's cheap and harmless, we allow the paste of separators and
-    // bookmarks from builds that use legacy transactions (i.e. when itemGuid
-    // was not set on PLACES_FLAVORS data). Containers are a different story,
-    // and thus disallowed.
-    if (aData.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
-      throw new Error("Can't copy a container from a legacy-transactions build");
-
-    if (aData.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
-      return PlacesTransactions.NewSeparator({ parentGuid: aNewParentGuid,
-                                               index: aIndex });
-    }
-
-    let title = aData.type != PlacesUtils.TYPE_UNICODE ? aData.title
-                                                       : aData.uri;
-    return PlacesTransactions.NewBookmark({ url: Services.io.newURI(aData.uri),
-                                            title,
-                                            parentGuid: aNewParentGuid,
-                                            index: aIndex });
-  },
-
-  /**
    * Processes a set of transfer items that have been dropped or pasted.
    * Batching will be applied where necessary.
    *
    * @param {Array} items A list of unwrapped nodes to process.
    * @param {Object} insertionPoint The requested point for insertion.
    * @param {Boolean} doCopy Set to true to copy the items, false will move them
    *                         if possible.
    * @paramt {Object} view The view that should be used for batching.
@@ -994,17 +939,17 @@ var PlacesUIUtils = {
     if (insertionPoint.isTag) {
       let urls = items.filter(item => "uri" in item).map(item => item.uri);
       itemsCount = urls.length;
       transactions = [PlacesTransactions.Tag({ urls, tag: insertionPoint.tagName })];
     } else {
       let insertionIndex = await insertionPoint.getIndex();
       itemsCount = items.length;
       transactions = await getTransactionsForTransferItems(
-        items, insertionIndex, insertionPoint.guid, doCopy);
+        items, insertionIndex, insertionPoint.guid, !doCopy);
     }
 
     // Check if we actually have something to add, if we don't it probably wasn't
     // valid, or it was moving to the same location, so just ignore it.
     if (!transactions.length) {
       return [];
     }
 
@@ -1193,30 +1138,77 @@ function getResultForBatching(viewOrElem
 
   if (viewOrElement && viewOrElement.result) {
     return viewOrElement.result;
   }
 
   return null;
 }
 
+
 /**
  * Processes a set of transfer items and returns transactions to insert or
  * move them.
  *
  * @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.
- * @param {Boolean} doCopy Set to true to copy the items, false will move them
- *                         if possible.
+ * @param {Boolean} doMove Set to true to MOVE the items if possible, false will
+ *                         copy them.
  * @return {Array} Returns an array of created PlacesTransactions.
  */
 async function getTransactionsForTransferItems(items, insertionIndex,
-                                               insertionParentGuid, doCopy) {
+                                               insertionParentGuid, doMove) {
+  let canMove = true;
+  for (let item of items) {
+    if (!PlacesUIUtils.SUPPORTED_FLAVORS.includes(item.type)) {
+      throw new Error(`Unsupported '${item.type}' data type`);
+    }
+
+    // Work out if this is data from the same app session we're running in.
+    if ("instanceId" in item && item.instanceId == PlacesUtils.instanceId) {
+      if ("itemGuid" in item && !PlacesUIUtils.PLACES_FLAVORS.includes(item.type)) {
+        throw new Error(`itemGuid unexpectedly set on ${item.type} data`);
+      }
+    } else {
+      if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
+        throw new Error("Can't copy a container from a legacy-transactions build");
+      }
+
+      // We can never move from an external copy.
+      canMove = false;
+    }
+
+    if (doMove && canMove) {
+      canMove = canMoveUnwrappedNode(item);
+    }
+  }
+
+  if (doMove && !canMove) {
+    Cu.reportError("Tried to move an unmovable Places " +
+                   "node, reverting to a copy operation.");
+    doMove = false;
+  }
+
+  return doMove ? getTransactionsForMove(items, insertionIndex, insertionParentGuid) :
+                  getTransactionsForCopy(items, insertionIndex, insertionParentGuid);
+}
+
+/**
+ * Processes a set of transfer items and returns an array of transactions.
+ *
+ * @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 getTransactionsForMove(items, insertionIndex,
+                                      insertionParentGuid) {
   let transactions = [];
   let index = insertionIndex;
 
   for (let item of items) {
     if (index != -1 && item.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.
@@ -1232,27 +1224,69 @@ async function getTransactionsForTransfe
           index--;
         } else if (index == existingBookmark.index) {
           // This isn't moving so we skip it.
           continue;
         }
       }
     }
 
-    // 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 && !canMoveUnwrappedNode(item)) {
-      Cu.reportError("Tried to move an unmovable Places " +
-                     "node, reverting to a copy operation.");
-      doCopy = true;
-    }
-    transactions.push(
-      PlacesUIUtils.getTransactionForData(item,
-                                          insertionParentGuid,
-                                          index,
-                                          doCopy));
+    transactions.push(PlacesTransactions.Move({
+      guid: item.itemGuid,
+      newIndex: index,
+      newParentGuid: insertionParentGuid,
+    }));
 
     if (index != -1 && item.itemGuid) {
       index++;
     }
   }
   return transactions;
 }
+
+/**
+ * Processes a set of transfer items and returns an array of transactions.
+ *
+ * @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;
+
+  for (let item of items) {
+    let transaction;
+
+    if ("itemGuid" in item && "instanceId" in item &&
+        item.instanceId == PlacesUtils.instanceId) {
+      transaction = PlacesTransactions.Copy({
+        excludingAnnotation: "Places/SmartBookmark",
+        guid: item.itemGuid,
+        newIndex: index,
+        newParentGuid: insertionParentGuid,
+      });
+    } else if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
+      transaction = PlacesTransactions.NewSeparator({
+        index,
+        parentGuid: insertionParentGuid,
+      });
+    } else {
+      let title = item.type != PlacesUtils.TYPE_UNICODE ? item.title : item.uri;
+      transaction = PlacesTransactions.NewBookmark({
+        index,
+        parentGuid: insertionParentGuid,
+        title,
+        url: item.uri,
+      });
+    }
+
+    transactions.push(transaction);
+
+    if (index != -1) {
+      index++;
+    }
+  }
+  return transactions;
+}
--- a/browser/components/places/tests/browser/browser_controller_onDrop.js
+++ b/browser/components/places/tests/browser/browser_controller_onDrop.js
@@ -15,17 +15,17 @@ var bookmarkIds;
 add_task(async function setup() {
   registerCleanupFunction(async function() {
     sandbox.restore();
     delete window.sinon;
     await PlacesUtils.bookmarks.eraseEverything();
     await PlacesUtils.history.clear();
   });
 
-  sandbox.stub(PlacesUIUtils, "getTransactionForData");
+  sandbox.stub(PlacesTransactions, "Move");
   sandbox.stub(PlacesTransactions, "batch");
 
   bookmarks = await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.unfiledGuid,
     children: [{
       title: "bm1",
       url: "http://example1.com"
     }, {
@@ -59,17 +59,17 @@ add_task(async function setup() {
 
 async function run_drag_test(startBookmarkIndex, insertionIndex, newParentGuid,
                              expectedInsertionIndex, expectTransactionCreated = true) {
   if (!newParentGuid) {
     newParentGuid = PlacesUtils.bookmarks.unfiledGuid;
   }
 
   // Reset the stubs so that previous test runs don't count against us.
-  PlacesUIUtils.getTransactionForData.reset();
+  PlacesTransactions.Move.reset();
   PlacesTransactions.batch.reset();
 
   let dragBookmark = bookmarks[startBookmarkIndex];
 
   await withSidebarTree("bookmarks", async (tree) => {
     tree.selectItems([PlacesUtils.bookmarks.unfiledGuid]);
     PlacesUtils.asContainer(tree.selectedNode).containerOpen = true;
 
@@ -78,21 +78,27 @@ async function run_drag_test(startBookma
     // insertion point and drag data and call the function direct.
     let ip = new PlacesInsertionPoint({
       parentId: await PlacesUtils.promiseItemId(PlacesUtils.bookmarks.unfiledGuid),
       parentGuid: newParentGuid,
       index: insertionIndex,
       orientation: Ci.nsITreeView.DROP_ON
     });
 
-    let bookmarkWithId = JSON.stringify(Object.assign({
+    let dragData = Object.assign({
       id: bookmarkIds.get(dragBookmark.guid),
       itemGuid: dragBookmark.guid,
       parent: PlacesUtils.unfiledBookmarksFolderId,
-    }, dragBookmark));
+      instanceId: PlacesUtils.instanceId,
+    }, dragBookmark);
+
+    // Force the type.
+    dragData.type = PlacesUtils.TYPE_X_MOZ_PLACE;
+    let bookmarkWithId = JSON.stringify(dragData);
+
 
     let dt = {
       dropEffect: "move",
       mozCursor: "auto",
       mozItemCount: 1,
       types: [ PlacesUtils.TYPE_X_MOZ_PLACE ],
       mozTypesAt(i) {
         return this.types;
@@ -100,34 +106,32 @@ async function run_drag_test(startBookma
       mozGetDataAt(i) {
         return bookmarkWithId;
       }
     };
 
     await PlacesControllerDragHelper.onDrop(ip, dt);
 
     if (!expectTransactionCreated) {
-      Assert.ok(PlacesUIUtils.getTransactionForData.notCalled,
+      Assert.ok(PlacesTransactions.Move.notCalled,
         "Should not have created transaction data");
       return;
     }
 
-    Assert.ok(PlacesUIUtils.getTransactionForData.calledOnce,
+    Assert.ok(PlacesTransactions.Move.calledOnce,
       "Should have called getTransactionForData at least once.");
 
-    let args = PlacesUIUtils.getTransactionForData.args[0];
+    let moveData = PlacesTransactions.Move.args[0][0];
 
-    Assert.deepEqual(args[0], JSON.parse(bookmarkWithId),
+    Assert.deepEqual(moveData.guid, dragBookmark.guid,
       "Should have called getTransactionForData with the correct unwrapped bookmark");
-    Assert.equal(args[1], newParentGuid,
+    Assert.equal(moveData.newParentGuid, newParentGuid,
       "Should have called getTransactionForData with the correct parent guid");
-    Assert.equal(args[2], expectedInsertionIndex,
+    Assert.equal(moveData.newIndex, expectedInsertionIndex,
       "Should have called getTransactionForData with the correct index");
-    Assert.equal(args[3], false,
-      "Should have called getTransactionForData with a move");
   });
 }
 
 add_task(async function test_simple_move_down() {
   // 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.
--- a/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
+++ b/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
@@ -80,17 +80,17 @@ async function run_drag_test(startBookma
       mozGetDataAt(i) {
         return bookmarkWithId;
       }
     };
 
     await PlacesControllerDragHelper.onDrop(ip, dt);
 
     Assert.ok(PlacesTransactions.Tag.calledOnce,
-      "Should have called getTransactionForData at least once.");
+      "Should have called PlacesTransactions.Tag 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,