Bug 1439697 - Always send the item guid in serialized copy data, and make the copy algorithm clearer as to what items use which methods. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 28 Mar 2018 11:25:40 +0100
changeset 774795 aaabb0d5c6e3bf876bae046adf06704308fa123a
parent 774794 e59dfb1d295f263369a3530ff7374476e3a43823
child 774796 ff5591a262d7416ac7ddba181f962fc7578699a7
push id104505
push userbmo:standard8@mozilla.com
push dateThu, 29 Mar 2018 15:40:27 +0000
reviewersmak
bugs1439697
milestone61.0a1
Bug 1439697 - Always send the item guid in serialized copy data, and make the copy algorithm clearer as to what items use which methods. r?mak MozReview-Commit-ID: 4dwJB2x2qyp
browser/components/places/PlacesUIUtils.jsm
toolkit/components/places/PlacesUtils.jsm
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -1160,21 +1160,17 @@ async function getTransactionsForTransfe
                                                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 (!("instanceId" in item) || item.instanceId != PlacesUtils.instanceId) {
       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;
     }
 
@@ -1253,22 +1249,32 @@ async function getTransactionsForMove(it
  */
 async function getTransactionsForCopy(items, insertionIndex,
                                       insertionParentGuid) {
   let transactions = [];
   let index = insertionIndex;
 
   for (let item of items) {
     let transaction;
+    let guid = item.itemGuid;
 
-    if ("itemGuid" in item && "instanceId" in item &&
-        item.instanceId == PlacesUtils.instanceId) {
+    if (PlacesUIUtils.PLACES_FLAVORS.includes(item.type) &&
+        // For anything that is comming from within this session, we do a
+        // direct copy, otherwise we fallback and form a new item below.
+        "instanceId" in item && item.instanceId == PlacesUtils.instanceId &&
+        // If the Item doesn't have a guid, this could be a virtual tag query or
+        // other item, so fallback to inserting a new bookmark with the URI.
+        guid &&
+        // For virtual root items, we fallback to creating a new bookmark, as
+        // we want a shortcut to be created, not a full tree copy.
+        !PlacesUtils.bookmarks.isVirtualRootItem(guid) &&
+        !PlacesUtils.isVirtualLeftPaneItem(guid)) {
       transaction = PlacesTransactions.Copy({
         excludingAnnotation: "Places/SmartBookmark",
-        guid: item.itemGuid,
+        guid,
         newIndex: index,
         newParentGuid: insertionParentGuid,
       });
     } else if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
       transaction = PlacesTransactions.NewSeparator({
         index,
         parentGuid: insertionParentGuid,
       });
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -102,32 +102,33 @@ async function notifyKeywordChange(url, 
  *        An nsINavHistoryResultNode
  * @param aIsLivemark
  *        Whether the node represents a livemark.
  */
 function serializeNode(aNode, aIsLivemark) {
   let data = {};
 
   data.title = aNode.title;
+  // The id is no longer used for copying within the same instance/session of
+  // Firefox as of at least 61. However, we keep the id for now to maintain
+  // backwards compat of drag and drop with older Firefox versions.
   data.id = aNode.itemId;
+  data.itemGuid = aNode.bookmarkGuid;
   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;
   let grandParentId;
 
   // Some nodes, e.g. the unfiled/menu/toolbar ones can have a virtual guid, so
   // we ignore any that are a folder shortcut. These will be handled below.
   if (guid && !PlacesUtils.bookmarks.isVirtualRootItem(guid) &&
       !PlacesUtils.isVirtualLeftPaneItem(guid)) {
-    // TODO: Really guid should be set on everything, however currently this upsets
-    // the drag 'n' drop / cut/copy/paste operations.
-    data.itemGuid = guid;
     if (aNode.parent) {
       data.parent = aNode.parent.itemId;
       data.parentGuid = aNode.parent.bookmarkGuid;
     }
 
     let grandParent = aNode.parent && aNode.parent.parent;
     if (grandParent) {
       grandParentId = grandParent.itemId;