Bug 1452621 - Cleanup some tag queries related code. r=standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 09 Apr 2018 15:38:45 +0200
changeset 412668 5b9d9f133beeb3b2da655a5420f3087e2707d7bd
parent 412667 f079b11f50b7563a87d35931fa45bcbeecb02484
child 412669 827cc04dacce5034dabe08b83351ea72326e45f7
push id101981
push useraiakab@mozilla.com
push dateTue, 10 Apr 2018 22:18:59 +0000
treeherdermozilla-inbound@9ad2b8aabfae [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1452621
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;
   },
 
   /**