Closed Bug 490156 Opened 11 years ago Closed 11 years ago

Can't delete smart bookmark containers


(Firefox :: Bookmarks & History, defect)

Windows XP
Not set



Firefox 3.6a1


(Reporter: luke.iliffe, Assigned: mak)



(Keywords: fixed1.9.1, regression)


(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090425 Shiretoko/3.5b5pre Firefox/3.0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090425 Minefield/3.6a1pre

Can no longer delete 'smart bookmark' containers such as the default 'Most Visited' on the bookmarks toolbar in a new profile. Neither the context menu | Delete, or the library | Delete does anything. No errors in the error console.

Narrowed down hourly regression range:
Works: 20090424092234 built from
Broken: 20090424092412 built from

There were several places bugs in that range Bug 484019 Bug 489351 Bug 489701 Bug 444849 

Reproducible: Always

Steps to Reproduce:
1. Open new profile in minefield
2. Right click and delete the Most Visited container on the bookmarks toolbar
Actual Results:  
nothing happens

Expected Results:  
Container is deleted
Version: unspecified → Trunk
Keywords: regression
Confirming, cannot/won't delete as per STR above. 

Latest hourly:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090425 Minefield/3.6a1pre Firefox/3.0.7 ID:20090425104144

Vista HP SP1
Ever confirmed: true
Forgot to include the pushlog between changes identified in the regression range in comment 0.
i can guess is bug 489351.
Blocks: 489351
Flags: in-testsuite?
Assignee: nobody → mak77
Attached patch patch v1.0 (obsolete) — Splinter Review
We should still threat bookmarks like bookmarks when they are!
Attachment #374855 - Flags: review?(adw)
Attachment #374855 - Flags: review?(adw) → review+
Comment on attachment 374855 [details] [diff] [review]
patch v1.0

>@@ -945,11 +945,12 @@ PlacesController.prototype = {
>         // 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.
>         this._removeHistoryContainer(node);
>         // History deletes are not undoable, so we don't have a transaction.
>         continue;

This whole loop with the conditionals is harder to follow than it should be, even with the comments you added.  Several of the conditionals use continue, but they are all else if's, and the very last line in the loop is not wrapped in a conditional at all.  Could you take this opportunity to clean up this loop?  IMHO eliminating the continues and wrapping the last line in else if would be best.  A final comment on what the last line catches would help, too.

>diff --git a/browser/components/places/tests/browser/browser_library_left_pane_commands.js b/browser/components/places/tests/browser/browser_library_left_pane_commands.js
>+    // 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.");
>+    var toolbarNode = PO._places.selectedNode
>+                        .QueryInterface(Ci.nsINavHistoryContainerResultNode);

Could you check that cmd_cut and cmd_delete are disabled here?

>+    // Get first child and check it is the "Most Visited" smart bookmark.
>+    var mostVisitedNode = toolbarNode.getChild(0);

Could you first check that the childCount is > 0?  Actually, it would be nice if you didn't rely on Most Visited being the first child...

With those changes, r=adw
Attached patch patch v1.1Splinter Review
addressed comments
Attachment #374855 - Attachment is obsolete: true
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Flags: in-testsuite? → in-testsuite+
Attachment #375058 - Flags: approval1.9.1?
Comment on attachment 375058 [details] [diff] [review]
patch v1.1

fixes a regression from bug 489351, that should be approved with this.
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre
Comment on attachment 375058 [details] [diff] [review]
patch v1.1

> (From update of attachment 375058 [details] [diff] [review])
> fixes a regression from bug 489351, that should be approved with this.

We can't keep tracing through your dependency trees this way; something's going to be missed. Please give us a roll-up patch and ask for approval in a single place if this sort of thing comes up.
Attachment #375058 - Flags: approval1.9.1? → approval1.9.1-
fixed in 1.9.1 with bug 489351
Keywords: fixed1.9.1
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.