Bug 1467807 - Stop supporting passing ids to (places)tree.selectItems and selectLeftPaneContainerByHierarchy. r=mak

MozReview-Commit-ID: 8E1VUKU2Ya2 Differential Revision: https://phabricator.services.mozilla.com/D1594
2018-06-08 16:09:37 +00:00 · 2018-06-08 16:09:37 +00:00 · 332df0f776
--- a/browser/components/places/content/editBookmark.js
+++ b/browser/components/places/content/editBookmark.js
@ -858,14 +858,16 @@ var gEditItemOverlay = {

    // XXXmano: add a separate "New Folder" string at some point...
    let title = this._element("newFolderButton").label;
-    await PlacesTransactions.NewFolder({ parentGuid: ip.guid, title,
-                                         index: await ip.getIndex() })
-                            .transact().catch(Cu.reportError);
+    let guid = await PlacesTransactions.NewFolder({
+      parentGuid: ip.guid,
+      title,
+      index: await ip.getIndex()
+    }).transact().catch(Cu.reportError);

    this._folderTree.focus();
-    this._folderTree.selectItems([ip.itemId]);
+    this._folderTree.selectItems([ip.guid]);
    PlacesUtils.asContainer(this._folderTree.selectedNode).containerOpen = true;
-    this._folderTree.selectItems([this._lastNewItem]);
+    this._folderTree.selectItems([guid]);
    this._folderTree.startEditing(this._folderTree.view.selection.currentIndex,
                                  this._folderTree.columns.getFirstColumn());
  },
@ -1034,10 +1036,7 @@ var gEditItemOverlay = {
    });
  },

-  onItemAdded(aItemId, aParentId, aIndex, aItemType, aURI) {
-    this._lastNewItem = aItemId;
-  },
-
+  onItemAdded() {},
  onItemRemoved() { },
  onBeginUpdateBatch() { },
  onEndUpdateBatch() { },
--- a/browser/components/places/content/places.js
+++ b/browser/components/places/content/places.js
@ -105,24 +105,19 @@ var PlacesOrganizer = {
      this._places.view.selection.selectEventsSuppressed = true;
    try {
      for (let container of hierarchy) {
-        switch (typeof container) {
-          case "number":
+        if (typeof container != "string") {
+          throw new Error("Invalid container type found: " + container);
+        }
+
+        try {
+          this.selectLeftPaneBuiltIn(container);
+        } catch (ex) {
+          if (container.substr(0, 6) == "place:") {
+            this._places.selectPlaceURI(container);
+          } else {
+            // Must be a guid.
            this._places.selectItems([container], false);
-            break;
-          case "string":
-            try {
-              this.selectLeftPaneBuiltIn(container);
-            } catch (ex) {
-              if (container.substr(0, 6) == "place:") {
-                this._places.selectPlaceURI(container);
-              } else {
-                // May be a guid.
-                this._places.selectItems([container], false);
-              }
-            }
-            break;
-          default:
-            throw new Error("Invalid container type found: " + container);
+          }
        }
        PlacesUtils.asContainer(this._places.selectedNode).containerOpen = true;
      }
@ -377,7 +372,7 @@ var PlacesOrganizer = {
  openFlatContainer: function PO_openFlatContainerFlatContainer(aContainer) {
    if (aContainer.itemId != -1) {
      PlacesUtils.asContainer(this._places.selectedNode).containerOpen = true;
-      this._places.selectItems([aContainer.itemId], false);
+      this._places.selectItems([aContainer.bookmarkGuid], false);
    } else if (PlacesUtils.nodeIsQuery(aContainer)) {
      this._places.selectPlaceURI(aContainer.uri);
    }
--- a/browser/components/places/content/tree.xml
+++ b/browser/components/places/content/tree.xml
@ -549,11 +549,9 @@
      <!-- This method will select the first node in the tree that matches
           each given item guid. It will open any folder nodes that it needs
           to in order to show the selected items.
-           Note: An array of ids or guids (or a mixture) may be passed as aIDs.
-           Passing IDs should be considered deprecated.
      -->
      <method name="selectItems">
-        <parameter name="aIDs"/>
+        <parameter name="aGuids"/>
        <parameter name="aOpenContainers"/>
        <body><![CDATA[
          // Never open containers in flat lists.
@ -565,7 +563,7 @@
          if (aOpenContainers === undefined)
            aOpenContainers = true;

-          var ids = aIDs; // don't manipulate the caller's array
+          var guids = aGuids; // don't manipulate the caller's array

          // Array of nodes found by findNodes which are to be selected
          var nodes = [];
@ -581,8 +579,8 @@

          /**
           * Recursively search through a node's children for items
-           * with the given IDs. When a matching item is found, remove its ID
-           * from the IDs array, and add the found node to the nodes dictionary.
+           * with the given GUIDs. When a matching item is found, remove its GUID
+           * from the GUIDs array, and add the found node to the nodes dictionary.
           *
           * NOTE: This method will leave open any node that had matching items
           * in its subtree.
@ -592,29 +590,22 @@
            // See if node matches an ID we wanted; add to results.
            // For simple folder queries, check both itemId and the concrete
            // item id.
-            var index = ids.indexOf(node.itemId);
-            if (index == -1 &&
-                node.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT)
-              index = ids.indexOf(PlacesUtils.asQuery(node).folderItemId);
-
+            var index = guids.indexOf(node.bookmarkGuid);
            if (index == -1) {
-              index = ids.indexOf(node.bookmarkGuid);
-              if (index == -1) {
-                let concreteGuid = PlacesUtils.getConcreteItemGuid(node);
-                if (concreteGuid != node.bookmarkGuid) {
-                  index = ids.indexOf(concreteGuid);
-                }
+              let concreteGuid = PlacesUtils.getConcreteItemGuid(node);
+              if (concreteGuid != node.bookmarkGuid) {
+                index = guids.indexOf(concreteGuid);
              }
            }

            if (index != -1) {
              nodes.push(node);
              foundOne = true;
-              ids.splice(index, 1);
+              guids.splice(index, 1);
            }

            var concreteGuid = PlacesUtils.getConcreteItemGuid(node);
-            if (ids.length == 0 || !PlacesUtils.nodeIsContainer(node) ||
+            if (guids.length == 0 || !PlacesUtils.nodeIsContainer(node) ||
                checkedGuidsSet.has(concreteGuid))
              return foundOne;

@ -634,7 +625,7 @@
            // this node if we don't find any additional results here.
            var previousOpenness = node.containerOpen;
            node.containerOpen = true;
-            for (var child = 0; child < node.childCount && ids.length > 0; child++) {
+            for (var child = 0; child < node.childCount && guids.length > 0; child++) {
              var childNode = node.getChild(child);
              var found = findNodes(childNode);
              if (!foundOne)
--- a/browser/components/places/tests/browser/browser_library_left_pane_select_hierarchy.js
+++ b/browser/components/places/tests/browser/browser_library_left_pane_select_hierarchy.js
@ -21,10 +21,7 @@ add_task(async function() {
    }],
  });

-  let folder1Id = await PlacesUtils.promiseItemId(items[0].guid);
-  let folder2Id = await PlacesUtils.promiseItemId(items[1].guid);
-
-  hierarchy.push(folder1Id, folder2Id);
+  hierarchy.push(items[0].guid, items[1].guid);

  let library = await promiseLibrary();