490156 - Can't delete smart bookmark containers

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Luke Iliffe (Harlequin99)]

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 http://hg.mozilla.org/mozilla-central/rev/0390afd7fc62
Broken: 20090424092412 built from http://hg.mozilla.org/mozilla-central/rev/b52c15e23bd2

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
3.
Actual Results:  
nothing happens

Expected Results:  
Container is deleted

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 2

[Luke Iliffe (Harlequin99)]

Forgot to include the pushlog between changes identified in the regression range in comment 0.
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0390afd7fc62&tochange=b52c15e23bd2

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i can guess is bug 489351.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

We should still threat bookmarks like bookmarks when they are!

Comment 5

[Drew Willcoxon :adw]

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

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

addressed comments

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/9392b394b383

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 375058 [details] [diff] [review]
patch v1.1

fixes a regression from bug 489351, that should be approved with this.

Comment 9

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

fixed in 1.9.1 with bug 489351

Comment 12

[Gervase Markham [:gerv]]

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.

Gerv