Bug 490156 - Can't delete smart bookmark containers, r=adw
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 30 Apr 2009 09:52:49 -0700
changeset 27920 9392b394b383f2ed25a13a3464fba91649a11375
parent 27919 313b7c5a27f5949e6c7b8ebfd32c84e2b63e7b40
child 27921 cb8262ea787fb45fcd644057b42dc61364ade14e
push id6776
push usermak77@bonardo.net
push dateThu, 30 Apr 2009 16:57:09 +0000
treeherdermozilla-central@9392b394b383 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs490156
milestone1.9.2a1pre
Bug 490156 - Can't delete smart bookmark containers, r=adw
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser_library_left_pane_commands.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -903,64 +903,64 @@ PlacesController.prototype = {
     if (!removedFolders)
       removedFolders = [];
 
     for (var i = 0; i < range.length; ++i) {
       var node = range[i];
       if (this._shouldSkipNode(node, removedFolders))
         continue;
 
-      if (PlacesUtils.nodeIsFolder(node)) {
-        // This is a bookmarks folder.  We add it to our array of folders, used
-        // to skip nodes that are children of an already removed folder.
-        removedFolders.push(node);
-      }
-      else if (PlacesUtils.nodeIsTagQuery(node.parent)) {
+      if (PlacesUtils.nodeIsTagQuery(node.parent)) {
         // This is a uri node inside a tag container.  It needs a special
         // untag transaction.
         var tagItemId = PlacesUtils.getConcreteItemId(node.parent);
         var uri = PlacesUtils._uri(node.uri);
         transactions.push(PlacesUIUtils.ptm.untagURI(uri, [tagItemId]));
-        continue;
       }
       else if (PlacesUtils.nodeIsTagQuery(node) && node.parent &&
                PlacesUtils.nodeIsQuery(node.parent) &&
                asQuery(node.parent).queryOptions.resultType ==
                  Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) {
         // This is a tag container.
         // Untag all URIs tagged with this tag only if the tag container is
         // child of the "Tags" query in the library, in all other places we
         // must only remove the query node.
         var tag = node.title;
         var URIs = PlacesUtils.tagging.getURIsForTag(tag);
         for (var j = 0; j < URIs.length; j++)
           transactions.push(PlacesUIUtils.ptm.untagURI(URIs[j], [tag]));
-        continue;
       }
       else if (PlacesUtils.nodeIsURI(node) &&
                PlacesUtils.nodeIsQuery(node.parent) &&
                asQuery(node.parent).queryOptions.queryType ==
                  Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) {
         // This is a uri node inside an history query.
         var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);
         bhist.removePage(PlacesUtils._uri(node.uri));
         // History deletes are not undoable, so we don't have a transaction.
-        continue;
       }
-      else if (PlacesUtils.nodeIsQuery(node) &&
+      else if (node.itemId == -1 &&
+               PlacesUtils.nodeIsQuery(node) &&
                asQuery(node).queryOptions.queryType ==
                  Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) {
-        // This is a special history query, could be a query grouped by site,
-        // time or both.
+        // This is a dynamically generated history query, like queries
+        // grouped by site, time or both.  Dynamically generated queries don't
+        // have an itemId even if they are descendants of a bookmark.
         this._removeHistoryContainer(node);
         // History deletes are not undoable, so we don't have a transaction.
-        continue;
       }
-
-      transactions.push(PlacesUIUtils.ptm.removeItem(node.itemId));
+      else {
+        // This is a common bookmark item.
+        if (PlacesUtils.nodeIsFolder(node)) {
+          // If this is a folder we add it to our array of folders, used
+          // to skip nodes that are children of an already removed folder.
+          removedFolders.push(node);
+        }
+        transactions.push(PlacesUIUtils.ptm.removeItem(node.itemId));
+      }
     }
   },
 
   /**
    * Removes the set of selected ranges from bookmarks.
    * @param   txnName
    *          See |remove|.
    */
--- a/browser/components/places/tests/browser/browser_library_left_pane_commands.js
+++ b/browser/components/places/tests/browser/browser_library_left_pane_commands.js
@@ -97,20 +97,78 @@ gTests.push({
 
     historyNode.containerOpen = false;
     nextTest();
   }
 });
 
 //------------------------------------------------------------------------------
 
+gTests.push({
+  desc: "Bug 490156 - Can't delete smart bookmark containers",
+  run: function() {
+    // Select and open the left pane "Bookmarks Toolbar" folder.
+    var PO = gLibrary.PlacesOrganizer;
+    PO.selectLeftPaneQuery('BookmarksToolbar');
+    isnot(PO._places.selectedNode, null, "We have a valid selection");
+    is(PlacesUtils.getConcreteItemId(PO._places.selectedNode),
+       PlacesUtils.toolbarFolderId,
+       "We have correctly selected bookmarks toolbar node.");
+
+    // Check that both cut and delete commands are disabled.
+    ok(!PO._places.controller.isCommandEnabled("cmd_cut"),
+       "Cut command is disabled");
+    ok(!PO._places.controller.isCommandEnabled("cmd_delete"),
+       "Delete command is disabled");
+
+    var toolbarNode = PO._places.selectedNode
+                        .QueryInterface(Ci.nsINavHistoryContainerResultNode);
+    toolbarNode.containerOpen = true;
+
+    // Add an History query to the toolbar.
+    PlacesUtils.bookmarks.insertBookmark(PlacesUtils.toolbarFolderId,
+                                         PlacesUtils._uri("place:sort=4"),
+                                         0, // Insert at start.
+                                         "special_query");
+    // Get first child and check it is the "Most Visited" smart bookmark.
+    ok(toolbarNode.childCount > 0, "Toolbar node has children");
+    var queryNode = toolbarNode.getChild(0);
+    is(queryNode.title, "special_query", "Query node is correctly selected");
+
+    // Select query node.
+    PO._places.selectNode(queryNode);
+    is(PO._places.selectedNode, queryNode, "We correctly selected query node");
+
+    // Check that both cut and delete commands are enabled.
+    ok(PO._places.controller.isCommandEnabled("cmd_cut"),
+       "Cut command is enabled");
+    ok(PO._places.controller.isCommandEnabled("cmd_delete"),
+       "Delete command is enabled");
+
+    // Execute the delete command and check bookmark has been removed.
+    PO._places.controller.doCommand("cmd_delete");
+    try {
+      PlacesUtils.bookmarks.getFolderIdForItem(queryNode.itemId);  
+      ok(false, "Unable to remove query node bookmark");
+    } catch(ex) {
+      ok(true, "Query node bookmark has been correctly removed");
+    }
+
+    toolbarNode.containerOpen = false;
+    nextTest();
+  }
+});
+
+//------------------------------------------------------------------------------
+
 function nextTest() {
   if (gTests.length) {
     var test = gTests.shift();
     ok(true, "TEST: " + test.desc);
+    dump("TEST: " + test.desc + "\n");
     test.run();
   }
   else {
     // Close Library window.
     gLibrary.close();
     // No need to cleanup anything, we have a correct left pane now.
     finish();
   }
@@ -131,16 +189,17 @@ var windowObserver = {
           nextTest();
         });
       }, false);
     }
   }
 };
 
 function test() {
+  dump("Starting test browser_library_left_pane_commands.js\n");
   waitForExplicitFinish();
   // Sanity checks.
   ok(PlacesUtils, "PlacesUtils is running in chrome context");
   ok(PlacesUIUtils, "PlacesUIUtils is running in chrome context");
 
   // Open Library.
   ww.registerNotification(windowObserver);
   ww.openWindow(null,