Bug 1452621 - Cleanup some tag queries related code. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 09 Apr 2018 15:38:45 +0200
changeset 779596 50b547644e39238402985072d400022c83a33a7a
parent 779146 30d72755b1749953d438199456f1524ce84ab5e5
push id105821
push usermak77@bonardo.net
push dateTue, 10 Apr 2018 09:25:09 +0000
reviewersstandard8
bugs1452621
milestone61.0a1
Bug 1452621 - Cleanup some tag queries related code. r=standard8 MozReview-Commit-ID: L8L3i5W1CHe
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser_bookmarkProperties_readOnlyRoot.js
browser/components/places/tests/browser/head.js
toolkit/components/places/PlacesUtils.jsm
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -171,17 +171,17 @@ PlacesController.prototype = {
     case "placesCmd_new:separator":
       return this._canInsert() &&
              !PlacesUtils.asQuery(this._view.result.root).queryOptions.excludeItems &&
              this._view.result.sortingMode ==
                  Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
     case "placesCmd_show:info": {
       let selectedNode = this._view.selectedNode;
       return selectedNode && (PlacesUtils.nodeIsTagQuery(selectedNode) ||
-                              PlacesUtils.getConcreteItemId(selectedNode) != -1);
+                              PlacesUtils.nodeIsBookmark(selectedNode));
     }
     case "placesCmd_reload": {
       // Livemark containers
       let selectedNode = this._view.selectedNode;
       return selectedNode && this.hasCachedLivemarkInfo(selectedNode);
     }
     case "placesCmd_sortBy:name": {
       let selectedNode = this._view.selectedNode;
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_readOnlyRoot.js
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_readOnlyRoot.js
@@ -1,21 +1,21 @@
 "use strict";
 
-add_task(async function() {
-  info("Bug 479348 - Properties on a root should be read-only.");
-
+add_task(async function test_dialog() {
+  info("Bug 479348 - Properties dialog on a root should be read-only.");
   await withSidebarTree("bookmarks", async function(tree) {
     tree.selectItems([PlacesUtils.bookmarks.unfiledGuid]);
-    Assert.ok(tree.controller.isCommandEnabled("placesCmd_show:info"),
-              "'placesCmd_show:info' on current selected node is enabled");
+    Assert.ok(!tree.controller.isCommandEnabled("placesCmd_show:info"),
+              "'placesCmd_show:info' on current selected node is disabled");
 
     await withBookmarksDialog(
       true,
       function openDialog() {
+        // Even if the cmd is disabled, we can execute it regardless.
         tree.controller.doCommand("placesCmd_show:info");
       },
       async function test(dialogWin) {
         // Check that the dialog is read-only.
         Assert.ok(dialogWin.gEditItemOverlay.readOnly, "Dialog is read-only");
         // Check that accept button is disabled
         let acceptButton = dialogWin.document.documentElement.getButton("accept");
         Assert.ok(acceptButton.disabled, "Accept button is disabled");
@@ -24,8 +24,30 @@ add_task(async function() {
         let namepicker = dialogWin.document.getElementById("editBMPanel_namePicker");
         Assert.ok(namepicker.readOnly, "Name field is read-only");
         Assert.equal(namepicker.value,
           PlacesUtils.getString("OtherBookmarksFolderTitle"), "Node title is correct");
       }
     );
   });
 });
+
+add_task(async function test_library() {
+  info("Bug 479348 - Library info pane on a root should be read-only.");
+  let library = await promiseLibrary("UnfiledBookmarks");
+  registerCleanupFunction(async function() {
+    await promiseLibraryClosed(library);
+  });
+  let PlacesOrganizer = library.PlacesOrganizer;
+  let tree = PlacesOrganizer._places;
+  tree.focus();
+  Assert.ok(!tree.controller.isCommandEnabled("placesCmd_show:info"),
+            "'placesCmd_show:info' on current selected node is disabled");
+
+  // Check that the pane is read-only.
+  Assert.ok(library.gEditItemOverlay.readOnly, "Info pane is read-only");
+
+  // Check that name picker is read only
+  let namepicker = library.document.getElementById("editBMPanel_namePicker");
+  Assert.ok(namepicker.readOnly, "Name field is read-only");
+  Assert.equal(namepicker.value,
+    PlacesUtils.getString("OtherBookmarksFolderTitle"), "Node title is correct");
+});
--- a/browser/components/places/tests/browser/head.js
+++ b/browser/components/places/tests/browser/head.js
@@ -204,17 +204,17 @@ var withBookmarksDialog = async function
                                          skipOverlayWait = false) {
   let closed = false;
   let dialogPromise = new Promise(resolve => {
     Services.ww.registerNotification(function winObserver(subject, topic, data) {
       if (topic == "domwindowopened") {
         let win = subject.QueryInterface(Ci.nsIDOMWindow);
         win.addEventListener("load", function() {
           ok(win.location.href.startsWith(dialogUrl),
-             "The bookmark properties dialog is open");
+             "The bookmark properties dialog is open: " + win.location.href);
           // This is needed for the overlay.
           waitForFocus(() => {
             resolve(win);
           }, win);
         }, {once: true});
       } else if (topic == "domwindowclosed") {
         Services.ww.unregisterNotification(winObserver);
         closed = true;
@@ -250,29 +250,24 @@ var withBookmarksDialog = async function
   let closePromise = () => Promise.resolve();
   if (closeFn) {
     closePromise = closeFn(dialogWin);
   }
 
   try {
     await taskFn(dialogWin);
   } finally {
-    if (!closed && !autoCancel) {
-      // Give the dialog a little time to close itself in the manually closing
-      // case.
-      await BrowserTestUtils.waitForCondition(() => closed,
-        "The test should have closed the dialog!");
-    }
-    if (!closed) {
+    if (!closed && autoCancel) {
       info("withBookmarksDialog: canceling the dialog");
-
       doc.documentElement.cancelDialog();
-
       await closePromise;
     }
+    // Give the dialog a little time to close itself.
+    await BrowserTestUtils.waitForCondition(() => closed,
+                                            "The dialog should be closed!");
   }
 };
 
 /**
  * Opens the contextual menu on the element pointed by the given selector.
  *
  * @param selector
  *        Valid selector syntax
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -141,82 +141,54 @@ function serializeNode(aNode, aIsLivemar
   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)) {
     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;
-    }
-
     data.dateAdded = aNode.dateAdded;
     data.lastModified = aNode.lastModified;
 
     let annos = getAnnotationsForItem(data.id);
     if (annos.length > 0)
       data.annos = annos;
   }
 
   if (PlacesUtils.nodeIsURI(aNode)) {
     // Check for url validity.
-    NetUtil.newURI(aNode.uri);
-
-    // Tag root accepts only folder nodes, not URIs.
-    if (data.parent == PlacesUtils.tagsFolderId)
-      throw new Error("Unexpected node type");
-
+    new URL(aNode.uri);
     data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
     data.uri = aNode.uri;
-
     if (aNode.tags)
       data.tags = aNode.tags;
-  } else if (PlacesUtils.nodeIsContainer(aNode)) {
-    // Tag containers accept only uri nodes.
-    if (grandParentId == PlacesUtils.tagsFolderId)
-      throw new Error("Unexpected node type");
-
-    let concreteId = PlacesUtils.getConcreteItemId(aNode);
-    if (concreteId != -1) {
-      // This is a bookmark or a tag container.
-      if (PlacesUtils.nodeIsQuery(aNode) || concreteId != aNode.itemId) {
-        // This is a folder shortcut.
-        data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
-        data.uri = aNode.uri;
-        data.concreteId = concreteId;
-        data.concreteGuid = PlacesUtils.getConcreteItemGuid(aNode);
-      } else {
-        // This is a bookmark folder.
-        data.type = PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER;
-      }
-    } else {
-      // This is a grouped container query, dynamically generated.
+  } else if (PlacesUtils.nodeIsFolder(aNode)) {
+    if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) {
       data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
       data.uri = aNode.uri;
+      data.concreteId = PlacesUtils.getConcreteItemId(aNode);
+      data.concreteGuid = PlacesUtils.getConcreteItemGuid(aNode);
+    } else {
+      data.type = PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER;
     }
+  } else if (PlacesUtils.nodeIsQuery(aNode)) {
+    data.type = PlacesUtils.TYPE_X_MOZ_PLACE;
+    data.uri = aNode.uri;
   } else if (PlacesUtils.nodeIsSeparator(aNode)) {
-    // Tag containers don't accept separators.
-    if (data.parent == PlacesUtils.tagsFolderId ||
-        grandParentId == PlacesUtils.tagsFolderId)
-      throw new Error("Unexpected node type");
-
     data.type = PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR;
   }
 
   return JSON.stringify(data);
 }
 
 // Imposed to limit database size.
 const DB_URL_LENGTH_MAX = 65536;
@@ -839,18 +811,16 @@ var PlacesUtils = {
   /**
    * Gets the concrete item-guid for the given node. For everything but folder
    * shortcuts, this is just node.bookmarkGuid.  For folder shortcuts, this is
    * node.targetFolderGuid (see nsINavHistoryService.idl for the semantics).
    *
    * @param aNode
    *        a result node.
    * @return the concrete item-guid for aNode.
-   * @note unlike getConcreteItemId, this doesn't allow retrieving the guid of a
-   *       ta container.
    */
   getConcreteItemGuid(aNode) {
     if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT)
       return asQuery(aNode).targetFolderGuid;
     return aNode.bookmarkGuid;
   },
 
   /**