Bug 1415877 - Merge transaction handling into one function for onDrop/paste. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 08 Nov 2017 14:48:45 +0000
changeset 698277 06c1bfcd9103bdb09381b8a4365cc442d8396f9e
parent 698276 8d0539088d3cef9744a2f77dd8af5a88e36f5afb
child 740326 fe77b696619a782fdab3e7af3dc1f622ca9f35af
push id89234
push userbmo:standard8@mozilla.com
push dateWed, 15 Nov 2017 12:20:47 +0000
reviewersmak
bugs1415877
milestone59.0a1
Bug 1415877 - Merge transaction handling into one function for onDrop/paste. r?mak MozReview-Commit-ID: AMhuLyc98WO
browser/components/places/content/controller.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1309,41 +1309,21 @@ PlacesController.prototype = {
       items = PlacesUtils.unwrapNodes(data, type);
     } catch (ex) {
       // No supported data exists or nodes unwrap failed, just bail out.
       return;
     }
 
     let itemsToSelect = [];
     if (PlacesUIUtils.useAsyncTransactions) {
-      let transactions = [];
-      if (ip.isTag) {
-        let urls = items.filter(item => "uri" in item).map(item => Services.io.newURI(item.uri));
-        transactions.push(PlacesTransactions.Tag({ urls, tag: ip.tagName }));
-      } else {
-        let insertionIndex = await ip.getIndex();
-        let doCopy = action == "copy";
-        let newTransactions = await getTransactionsForTransferItems(
-          items, insertionIndex, ip.guid, doCopy);
-        if (newTransactions.length) {
-          transactions = [...transactions, ...newTransactions];
-        }
-      }
-      // Note: this._view may be a view or the tree element.
-      let resultForBatching = getResultForBatching(this._view);
+      let doCopy = action == "copy";
+      let guidsToSelect = await handleTransferItems(items, ip, doCopy, this._view);
 
-      await PlacesUIUtils.batchUpdatesForNode(resultForBatching,
-        transactions.length, async () => {
-          await PlacesTransactions.batch(async () => {
-            for (let transaction of transactions) {
-              let guid = await transaction.transact();
-              itemsToSelect.push(await PlacesUtils.promiseItemId(guid));
-            }
-          });
-        });
+      let guidsToIdMap = await PlacesUtils.promiseManyItemIds(guidsToSelect);
+      itemsToSelect = Array.from(guidsToIdMap.values());
     } else {
       let transactions = [];
       let insertionIndex = await ip.getIndex();
       for (let index = insertionIndex, i = 0; i < items.length; ++i) {
         if (ip.isTag) {
           // Pasting into a tag container means tagging the item, regardless of
           // the requested action.
           let tagTxn = new PlacesTagURITransaction(NetUtil.newURI(items[i].uri),
@@ -1615,17 +1595,16 @@ var PlacesControllerDragHelper = {
    *                                batching to take place.
    */
   async onDrop(insertionPoint, dt, view) {
     let doCopy = ["copy", "link"].includes(dt.dropEffect);
 
     let transactions = [];
     let dropCount = dt.mozItemCount;
     let parentGuid = insertionPoint.guid;
-    let tagName = insertionPoint.tagName;
 
     // Following flavors may contain duplicated data.
     let duplicable = new Map();
     duplicable.set(PlacesUtils.TYPE_UNICODE, new Set());
     duplicable.set(PlacesUtils.TYPE_X_MOZ_URL, new Set());
 
     // Collect all data from the DataTransfer before processing it, as the
     // DataTransfer is only valid during the synchronous handling of the `drop`
@@ -1662,39 +1641,17 @@ var PlacesControllerDragHelper = {
             title: data.label,
             type: PlacesUtils.TYPE_X_MOZ_URL
           });
         } else {
           throw new Error("bogus data was passed as a tab");
         }
       }
 
-      // 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 {
-        let insertionIndex = await insertionPoint.getIndex();
-        let newTransactions = await getTransactionsForTransferItems(
-          nodes, insertionIndex, parentGuid, doCopy);
-        if (newTransactions.length) {
-          transactions = [...transactions, ...newTransactions];
-        }
-      }
-
-      // 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;
-      }
-      let resultForBatching = getResultForBatching(view);
-      await PlacesUIUtils.batchUpdatesForNode(resultForBatching,
-        transactions.length, async () => {
-          await PlacesTransactions.batch(transactions);
-        });
+      await handleTransferItems(nodes, insertionPoint, doCopy, view);
     } else {
       for (let {flavor, data} of dtItems) {
         let nodes;
         if (flavor != TAB_DROP_TYPE) {
           nodes = PlacesUtils.unwrapNodes(data, flavor);
         } else if (data instanceof XULElement && data.localName == "tab" &&
                  data.ownerGlobal.isChromeWindow) {
           let uri = data.linkedBrowser.currentURI;
@@ -1859,16 +1816,70 @@ function getResultForBatching(viewOrElem
   if (viewOrElement && viewOrElement.result) {
     return viewOrElement.result;
   }
 
   return null;
 }
 
 /**
+ * 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.
+ * @return {Array} Returns an empty array when the insertion point is a tag, else
+ *                 returns an array of copied or moved guids.
+ */
+async function handleTransferItems(items, insertionPoint, doCopy, view) {
+  let transactions;
+  if (insertionPoint.isTag) {
+    let urls = items.filter(item => "uri" in item).map(item => item.uri);
+    transactions = [PlacesTransactions.Tag({ urls, tag: insertionPoint.tagName })];
+  } else {
+    let insertionIndex = await insertionPoint.getIndex();
+    transactions = await getTransactionsForTransferItems(
+      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 [];
+  }
+
+  let guidsToSelect = [];
+  let resultForBatching = getResultForBatching(view);
+
+  // If we're inserting into a tag, we don't get the guid, so we'll just
+  // pass the transactions direct to the batch function.
+  let batchingItem = transactions;
+  if (!insertionPoint.isTag) {
+    // If we're not a tag, then we need to get the ids of the items to select.
+    batchingItem = async () => {
+      for (let transaction of transactions) {
+        let guid = await transaction.transact();
+        if (guid) {
+          guidsToSelect.push(guid);
+        }
+      }
+    };
+  }
+
+  await PlacesUIUtils.batchUpdatesForNode(resultForBatching,
+    transactions.length, async () => {
+      await PlacesTransactions.batch(batchingItem);
+    });
+
+  return guidsToSelect;
+}
+
+/**
  * 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