Bug 1122702 JavaScript Error: "invalid value for aNodeOrItemId" from PlacesUIUtils.jsm r=Neil a=IanN a=comm-aurora a=comm-beta on CLOSED TREE
authorPhilip Chee <philip.chee@gmail.com>
Mon, 19 Jan 2015 23:51:12 +0800
changeset 21637 ce25f6b1662d34d3bbee7b9763a4d39b914b0d91
parent 21636 db00efe0aa4c39459ebfb102f935b0ae06698027
child 21638 fdcb700082b60c50fa5b386a62e507422b23647b
push id1305
push usermbanner@mozilla.com
push dateMon, 23 Feb 2015 19:48:12 +0000
treeherdercomm-beta@3ae4f13858fd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersNeil, IanN, comm-aurora, comm-beta
bugs1122702
Bug 1122702 JavaScript Error: "invalid value for aNodeOrItemId" from PlacesUIUtils.jsm r=Neil a=IanN a=comm-aurora a=comm-beta on CLOSED TREE
suite/common/bookmarks/browser-places.js
suite/common/places/controller.js
suite/modules/PlacesUIUtils.jsm
--- a/suite/common/bookmarks/browser-places.js
+++ b/suite/common/bookmarks/browser-places.js
@@ -433,17 +433,18 @@ var PlacesCommandHook = {
    * Opens the bookmarks manager.
    * @param   aLeftPaneRoot
    *          The query to select in the organizer window - options
    *          are: AllBookmarks, BookmarksMenu, BookmarksToolbar,
    *          UnfiledBookmarks and Tags.
    */
   showBookmarksManager: function PCH_showBookmarksManager(aLeftPaneRoot) {
     var manager = Services.wm.getMostRecentWindow("bookmarks:manager");
-    if (!manager) {
+    // Due to bug 528706, getMostRecentWindow can return closed windows.
+    if (!manager || manager.closed) {
       // No currently open places window, so open one with the specified mode.
       openDialog("chrome://communicator/content/bookmarks/bookmarksManager.xul",
                  "", "all,dialog=no", aLeftPaneRoot);
     }
     else {
       manager.PlacesOrganizer.selectLeftPaneQuery(aLeftPaneRoot);
       manager.focus();
     }
--- a/suite/common/places/controller.js
+++ b/suite/common/places/controller.js
@@ -130,30 +130,30 @@ PlacesController.prototype = {
   isCommandEnabled: function PC_isCommandEnabled(aCommand) {
     switch (aCommand) {
     case "cmd_undo":
       return PlacesUtils.transactionManager.numberOfUndoItems > 0;
     case "cmd_redo":
       return PlacesUtils.transactionManager.numberOfRedoItems > 0;
     case "cmd_cut":
     case "placesCmd_cut":
-      var nodes = this._view.selectedNodes;
-      // If selection includes history nodes there's no reason to allow cut.
-      for (var i = 0; i < nodes.length; i++) {
-        if (nodes[i].itemId == -1)
+    case "placesCmd_moveBookmarks":
+      for (let node of this._view.selectedNodes) {
+        // If selection includes history nodes or tags-as-bookmark,
+        // disallow cutting.
+        if (node.itemId == -1 ||
+            (node.parent && PlacesUtils.nodeIsTagQuery(node.parent))) {
           return false;
+        }
       }
-      // Otherwise fallback to cmd_delete check.
+      // Otherwise fall through the cmd_delete check.
     case "cmd_delete":
     case "placesCmd_delete":
-      return this._hasRemovableSelection(false);
     case "placesCmd_deleteDataHost":
-      return this._hasRemovableSelection(false);
-    case "placesCmd_moveBookmarks":
-      return this._hasRemovableSelection(true);
+      return this._hasRemovableSelection();
     case "cmd_copy":
     case "placesCmd_copy":
       return this._view.hasSelection;
     case "cmd_paste":
     case "placesCmd_paste":
       return this._canInsert(true) && this._isClipboardDataPasteable();
     case "cmd_selectAll":
       if (this._view.selType != "single") {
@@ -284,23 +284,21 @@ PlacesController.prototype = {
   onEvent: function PC_onEvent(eventName) { },
 
   /**
    * Determine whether or not the selection can be removed, either by the
    * delete or cut operations based on whether or not any of its contents
    * are non-removable. We don't need to worry about recursion here since it
    * is a policy decision that a removable item not be placed inside a non-
    * removable item.
-   * @param aIsMoveCommand
-   *        True if the command for which this method is called only moves the
-   *        selected items to another container, false otherwise.
+   *
    * @returns true if all nodes in the selection can be removed,
    *          false otherwise.
    */
-  _hasRemovableSelection: function PC__hasRemovableSelection(aIsMoveCommand) {
+  _hasRemovableSelection() {
     var ranges = this._view.removableSelectionRanges;
     if (!ranges.length)
       return false;
 
     var root = this._view.result.root;
 
     for (var j = 0; j < ranges.length; j++) {
       var nodes = ranges[j];
@@ -1016,17 +1014,17 @@ PlacesController.prototype = {
 
   /**
    * Removes the selection
    * @param   aTxnName
    *          A name for the transaction if this is being performed
    *          as part of another operation.
    */
   remove: function PC_remove(aTxnName) {
-    if (!this._hasRemovableSelection(false))
+    if (!this._hasRemovableSelection())
       return;
 
     NS_ASSERT(aTxnName !== undefined, "Must supply Transaction Name");
 
     var root = this._view.result.root;
 
     if (PlacesUtils.nodeIsFolder(root))
       this._removeRowsFromBookmarks(aTxnName);
--- a/suite/modules/PlacesUIUtils.jsm
+++ b/suite/modules/PlacesUIUtils.jsm
@@ -790,16 +790,20 @@ var PlacesUIUtils = {
     // livemark.
     if (aNode.itemId == -1) {
       // Rather than executing a db query, checking the existence of the feedURI
       // annotation, detect livemark children by the fact that they are the only
       // direct non-bookmark children of bookmark folders.
       return !PlacesUtils.nodeIsFolder(parentNode);
     }
 
+    // Generally it's always possible to remove children of a query.
+    if (PlacesUtils.nodeIsQuery(parentNode))
+      return true;
+
     // Otherwise it has to be a child of an editable folder.
     return !this.isContentsReadOnly(parentNode);
   },
 
   /**
    * DO NOT USE THIS API IN ADDONS. IT IS VERY LIKELY TO CHANGE WHEN THE SWITCH
    * TO GUIDS IS COMPLETE (BUG 1071511).
    *