387746 - New left pane for Places Organizer

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Dietrich Ayala (:dietrich)]

see the bug URL for mockups and notes.

* all bookmarks, all tags, toolbar, menu, downloads, history

Comment 1

[Dietrich Ayala (:dietrich)]

iirc, mano had a WIP patch for something like this, but i couldn't find the bug it was on. mano?

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Here is the latest iteration of the Places Organizer:

http://people.mozilla.com/~faaborg/files/granParadisoUI/placesOrganizer_i5WindowLayout.png

Note the changes to the finder/favorites area, which now contains expanding elements.

Comment 3

[Asaf Romano (gone)]

faaborg, what's under the _expandable_ all bookmarks item? bookmarks menu & bookmarks toolbar? :p

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>what's under the _expandable_ all bookmarks item? bookmarks menu &
>bookmarks toolbar? :p


The rest of the bookmarks hierarchy, so bookmarks toolbar and menu would be under All Bookmarks.  The bookmarks toolbar and menu top level items would only show their subfolders if expanded.

Comment 5

[Dietrich Ayala (:dietrich)]

mano, is this possible for M8? given the latest mockup, will this remain a tree, possibly with pre-populated queries (bookmarks) for the top-level items?

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

The whole finder/favorites area doesn't need to land for M9, but it would be good to get the "All Bookmarks" root accessible for M9, since starring a page doesn't seem to do anything without it.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

Mano: do you want me to file a separate bug for the "All Bookmarks" root, or are we close to landing this one?

Comment 8

[Asaf Romano (gone)]

Attachment: wip

Comment 9

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

Blocking yes, but not the beta.

Comment 10

[Asaf Romano (gone)]

Attachment: more...

Comment 11

[Asaf Romano (gone)]

Attachment: more refactoring..

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 287245 [details] [diff] [review]
more refactoring..

some general comments that can be filed as followups:

- the properties pane is squished and ugly when disabled for history items
- the "last editable item's properties are in the disabled pane" bug is even more awkward now
- seems weird to click on "organize bookmarks" and have history be the default view, should be "all bookmarks" maybe?
- "all bookmarks" should be above the menu and toolbar items

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.49
>diff -u -p -8 -r1.49 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl	17 Sep 2007 01:42:16 -0000	1.49
>+++ toolkit/components/places/public/nsINavBookmarksService.idl	3 Nov 2007 19:56:49 -0000
>@@ -52,17 +52,17 @@ interface nsITransaction;
> interface nsINavHistoryBatchCallback;
> 
> [ptr] native PRInt64Array(nsTArray<PRInt64>);
> 
> /**
>  * Observer for bookmark changes.
>  */
> 
>-[scriptable, uuid(f9828ba8-9c70-4d95-b926-60d9e4378d7d)]
>+[scriptable, uuid(fb8dc678-681b-453b-a5fe-ab3cc19b3698)]
> interface nsINavBookmarkObserver : nsISupports

this is probably intended for nsINavBookmarkService

> 
> nsresult
> nsNavBookmarks::ResultNodeForContainer(PRInt64 aID,
>                                        nsNavHistoryQueryOptions *aOptions,
>-                                       nsNavHistoryResultNode **aNode)
>+                                       nsNavHistoryResultNode **aNode,
>+                                       PRInt64 aGeneratingItemId)

please add a comment explaining the purpose of aGeneratingItemId.

>@@ -2190,17 +2187,19 @@ static
> PRBool NeedToFilterResultSet(const nsCOMArray<nsNavHistoryQuery>& aQueries, 
>                              nsNavHistoryQueryOptions *aOptions)
> {
>   // optimize the case where we just want a list with no grouping: this
>   // directly fills in the results and we avoid a copy of the whole list
>   PRUint32 groupCount;
>   const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
> 
>-  if (groupCount != 0 || aOptions->ExcludeQueries())
>+  // Always filter bookmarks queries to avoid  the inclusion of query nodes
>+  if (groupCount != 0 ||
>+      aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS)
>     return PR_TRUE;

did you intend to remove the ExcludeQueries check?

>@@ -2298,26 +2297,17 @@ nsNavHistory::ConstructQueryString(const
>       queryString = NS_LITERAL_CSTRING(
>         "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(visit_date), "
>         "f.url, null, null "
>         "FROM moz_places h "
>         "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
>         "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
>       groupBy = NS_LITERAL_CSTRING(" GROUP BY h.id");
>     } else if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) {
>-      queryString = NS_LITERAL_CSTRING("SELECT b.fk, h.url, ");
>-      if (aOptions->ResolveNullBookmarkTitles()) {
>-        // COALESCE, first non NULL param
>-        queryString += NS_LITERAL_CSTRING(
>-          "COALESCE(b.title, "
>-          "(SELECT h.title FROM moz_bookmarks b2, moz_places h2 WHERE b2.fk = h2.id AND b2.id = b.id)), ");
>-      }
>-      else {
>-        queryString += NS_LITERAL_CSTRING("b.title, ");
>-      }
>+      queryString = NS_LITERAL_CSTRING("SELECT b.fk, h.url, COALESCE(b.title, h.title), ");

hrm, your coalesce looks ok, but why did seth originally implement this with that sub-select?

>Index: toolkit/components/places/tests/unit/test_331487.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_331487.js,v
>retrieving revision 1.1
>diff -u -p -8 -r1.1 test_331487.js
>--- toolkit/components/places/tests/unit/test_331487.js	11 Oct 2007 06:42:42 -0000	1.1
>+++ toolkit/components/places/tests/unit/test_331487.js	3 Nov 2007 19:57:07 -0000
>@@ -132,21 +132,18 @@ function run_test() {
>   query.setFolders([folder], 1);
>   var result = histsvc.executeQuery(query, options);
>   var root = result.root;
>   root.containerOpen = true;
>   do_check_eq(root.childCount, 3);
>   // these containers are nsNavHistoryContainerResultNodes, 
>   // created in GroupByFolder() and not nsNavHistoryFolderResultNodes, 
>   // so they should not have an item id.
>-  do_check_eq(root.getChild(0).itemId, -1);
>   do_check_eq(root.getChild(0).title, "test folder");
>-  do_check_eq(root.getChild(1).itemId, -1);
>   do_check_eq(root.getChild(1).title, "subfolder 1");
>-  do_check_eq(root.getChild(2).itemId, -1);
>   do_check_eq(root.getChild(2).title, "subfolder 2");
> 
>   // check the contents of the "test folder" container
>   var rfNode = root.getChild(0);
>   rfNode = rfNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>   rfNode.containerOpen = true;
>   do_check_eq(rfNode.childCount, 1);
>   do_check_eq(rfNode.getChild(0).itemId, b1);

What caused this change?

>Index: browser/components/places/content/menu.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
>retrieving revision 1.91
>diff -u -p -8 -r1.91 menu.xml
>--- browser/components/places/content/menu.xml	31 Oct 2007 00:15:55 -0000	1.91
>+++ browser/components/places/content/menu.xml	3 Nov 2007 19:57:16 -0000
>@@ -319,16 +319,20 @@
>               menuitem.setAttribute("image", spec);
>           }
>           else
>             menuitem.removeAttribute("image");
> 
>           var title = aNode.title;
>           if (menuitem.getAttribute("label") != title)
>             menuitem.setAttribute("label", title);
>+
>+          if (!menuItem.hasAttribute("livemark") &&
>+					    PlacesUtils.nodeIsLivemarkContainer(aNode))
>+					  menuItem.setAttribute("livemark", "true");

nit: wonky indent

>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.111
>diff -u -p -8 -r1.111 places.js
>--- browser/components/places/content/places.js	25 Oct 2007 02:37:05 -0000	1.111
>+++ browser/components/places/content/places.js	3 Nov 2007 19:57:18 -0000
>@@ -32,39 +32,107 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+const QUERIES_ROOT_ANNO = "PlacesOrganizer/QueriesRoot";
>+const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
> 
> /**
>  * Selects a place URI in the places list.
>  * This function is global so it can be easily accessed by openers.
>  * @param   placeURI
>  *          A place: URI string to select
>  */
> function selectPlaceURI(placeURI) {
>   PlacesOrganizer._places.selectPlaceURI(placeURI);
> }
> 
> var PlacesOrganizer = {
>   _places: null,
>   _content: null,
> 
>+  _initFolderTree: function() {
>+    var queriesRoot;
>+    var items = PlacesUtils.annotations
>+                           .getItemsWithAnnotation(QUERIES_ROOT_ANNO, { });
>+    if (items.length > 0)
>+      queriesRoot = items[0];
>+    else {
>+      var callback = {
>+        runBatched: function(aUserData) {
>+          var bms = PlacesUtils.bookmarks;
>+          var ans = PlacesUtils.annotations;
>+
>+          // Root of All Magic Queries
>+          queriesRoot = bms.createFolder(PlacesUtils.placesRootId, "", -1);
>+          ans.setItemAnnotation(queriesRoot, QUERIES_ROOT_ANNO, 1, 0,
>+                                ans.EXPIRE_NEVER);
>+
>+          // History Query
>+          var uri = IO.newURI("place:sort=4&");
>+          var title = PlacesUtils.getString("OrganizerQueryHistory");
>+          var itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
>+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, "History", 0,
>+                                ans.EXPIRE_NEVER);
>+
>+          // Tags Query
>+          uri = IO.newURI("place:excludeItems=1&folder=" + PlacesUtils.tagRootId);
>+          title = PlacesUtils.getString("OrganizerQueryTags");
>+          itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
>+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, "Tags", 0,
>+                                ans.EXPIRE_NEVER);
>+
>+          // Bookmarks Menu Query
>+          uri = IO.newURI("place:excludeItems=1&folder=" +
>+                          PlacesUtils.bookmarksRootId);
>+          title = PlacesUtils.getString("OrganizerQueryBookmarksMenu");
>+          itemId = bms.insertBookmark(queriesRoot, uri, -1,title);

nit: space before "title"

>+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, "BookmarksMenu",
>+                                0, ans.EXPIRE_NEVER);
>+
>+          // Bookmarks Toolbar Query
>+          uri = IO.newURI("place:excludeItems=1&folder=" +
>+                          PlacesUtils.toolbarFolderId);
>+          title = PlacesUtils.getString("OrganizerQueryBookmarksToolbar");
>+          itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
>+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO,
>+                                "BookmarksToolbar", 0, ans.EXPIRE_NEVER);
>+
>+          // All Bookmarks query
>+          uri = IO.newURI("place:queryType=1&folder=" + PlacesUtils.bookmarksRootId +
>+                          "&folder=" + PlacesUtils.unfiledRootId +
>+                          "&excludeItemIfParentHasAnnotation=livemark/feedURI");
>+          title = PlacesUtils.getString("OrganizerQueryAllBookmarks");
>+          itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
>+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO,
>+                                "AllBookmarks", 0, ans.EXPIRE_NEVER);
>+
>+          // disallow manipulating this folder within the organizer UI
>+          PlacesUtils.bookmarks.setFolderReadonly(queriesRoot, true);

nit: could do s/PlacesUtils.bookmarks/bms/ here

>+        }
>+      };
>+      PlacesUtils.bookmarks.runInBatchMode(callback, null);
>+    }
>+    
>+    this._places.place = "place:excludeItems=1&expandQueries=0&folder=" + queriesRoot;
>+  },

hm, it's conceivable that these could get deleted somehow. should there be a pref to regenerate them?

also, this has the same locale-switching problem as the Places folder. just noting, don't need to resolve that here.

>+
>   init: function PO_init() {
>     this._places = document.getElementById("placesList");
>     this._content = document.getElementById("placeContent");
>+    this._initFolderTree();
> 
>     OptionsFilter.init(Groupers);
>     Groupers.init();
>-
>+    

nit: whitespace

>Index: browser/components/places/content/toolbar.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v
>retrieving revision 1.110
>diff -u -p -8 -r1.110 toolbar.xml
>--- browser/components/places/content/toolbar.xml	31 Oct 2007 00:15:56 -0000	1.110
>+++ browser/components/places/content/toolbar.xml	3 Nov 2007 19:57:20 -0000
>@@ -537,16 +537,20 @@
>           if (iconURI) {
>             var spec = iconURI.spec;
>             if (element.getAttribute("image") != spec)
>               element.setAttribute("image", spec);
>           }
>           else
>             element.removeAttribute("image");
> 
>+          if (!element.hasAttribute("livemark") &&
>+					    PlacesUtils.nodeIsLivemarkContainer(aNode))
>+					  element.setAttribute("livemark", "true");
>+

nit: wonky indent

>Index: browser/components/places/content/treeView.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
>retrieving revision 1.22
>diff -u -p -8 -r1.22 treeView.js
>--- browser/components/places/content/treeView.js	25 Oct 2007 02:02:29 -0000	1.22
>+++ browser/components/places/content/treeView.js	3 Nov 2007 19:57:22 -0000
>@@ -366,34 +349,36 @@ PlacesTreeView.prototype = {
>     if (nodesToSelect.length > 0)
>       selection.selectEventsSuppressed = true;
> 
>     this._tree.beginUpdateBatch();
>     if (replaceCount)
>       this._tree.rowCountChanged(startReplacement, -replaceCount);
>     if (newElements.length)
>       this._tree.rowCountChanged(startReplacement, newElements.length);
>-    this._tree.endUpdateBatch();
> 
>     // now, open any containers that were persisted
>     for (var i = 0; i < toOpenElements.length; i++) {
>       var item = toOpenElements[i];
>       var parent = item.parent;
>       // avoid recursively opening containers
>       while (parent) {
>+        dump("Comparing " + parent.uri + " " + item.uri + "\n");

remove

Comment 13

[Dietrich Ayala (:dietrich)]

- selecting "sort by name" on the context menu of tag containers in the left pane causes the content view to sort and all item titles change to "(no title)".

- you can select "properties" from a tag container's context menu and edit it's title. "properties" should probably be disabled.

Comment 14

[Asaf Romano (gone)]

(In reply to comment #12)
> (From update of attachment 287245 [details] [diff] [review])
> some general comments that can be filed as followups:
> 
> - the properties pane is squished and ugly when disabled for history items
> - the "last editable item's properties are in the disabled pane" bug is even
> more awkward now

follow up please.

> - seems weird to click on "organize bookmarks" and have history be the default
> view, should be "all bookmarks" maybe?

Bookmarks-Menu was alex's plan, I think.

> - "all bookmarks" should be above the menu and toolbar items

not according to the mockup
 

> please add a comment explaining the purpose of aGeneratingItemId.
> 

In the view, we show the title of the place: item even if it just points to another folder.

> >@@ -2190,17 +2187,19 @@ static
> > PRBool NeedToFilterResultSet(const nsCOMArray<nsNavHistoryQuery>& aQueries, 
> >                              nsNavHistoryQueryOptions *aOptions)
> > {
> >   // optimize the case where we just want a list with no grouping: this
> >   // directly fills in the results and we avoid a copy of the whole list
> >   PRUint32 groupCount;
> >   const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
> > 
> >-  if (groupCount != 0 || aOptions->ExcludeQueries())
> >+  // Always filter bookmarks queries to avoid  the inclusion of query nodes
> >+  if (groupCount != 0 ||
> >+      aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS)
> >     return PR_TRUE;
> 
> did you intend to remove the ExcludeQueries check?
> 

Yeah, in some other bug though (this has already landed).

> >@@ -2298,26 +2297,17 @@ nsNavHistory::ConstructQueryString(const
> >       queryString = NS_LITERAL_CSTRING(
> >         "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(visit_date), "
> >         "f.url, null, null "
> >         "FROM moz_places h "
> >         "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
> >         "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
> >       groupBy = NS_LITERAL_CSTRING(" GROUP BY h.id");
> >     } else if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) {
> >-      queryString = NS_LITERAL_CSTRING("SELECT b.fk, h.url, ");
> >-      if (aOptions->ResolveNullBookmarkTitles()) {
> >-        // COALESCE, first non NULL param
> >-        queryString += NS_LITERAL_CSTRING(
> >-          "COALESCE(b.title, "
> >-          "(SELECT h.title FROM moz_bookmarks b2, moz_places h2 WHERE b2.fk = h2.id AND b2.id = b.id)), ");
> >-      }
> >-      else {
> >-        queryString += NS_LITERAL_CSTRING("b.title, ");
> >-      }
> >+      queryString = NS_LITERAL_CSTRING("SELECT b.fk, h.url, COALESCE(b.title, h.title), ");
> 
> hrm, your coalesce looks ok, but why did seth originally implement this with
> that sub-select?
> 
https://bugzilla.mozilla.org/show_bug.cgi?id=387996#c80


> >Index: toolkit/components/places/tests/unit/test_331487.js
> >===================================================================

> >@@ -132,21 +132,18 @@ function run_test() {
> >   query.setFolders([folder], 1);
> >   var result = histsvc.executeQuery(query, options);
> >   var root = result.root;
> >   root.containerOpen = true;
> >   do_check_eq(root.childCount, 3);
> >   // these containers are nsNavHistoryContainerResultNodes, 
> >   // created in GroupByFolder() and not nsNavHistoryFolderResultNodes, 
> >   // so they should not have an item id.
> >-  do_check_eq(root.getChild(0).itemId, -1);
> >   do_check_eq(root.getChild(0).title, "test folder");
> >-  do_check_eq(root.getChild(1).itemId, -1);
> >   do_check_eq(root.getChild(1).title, "subfolder 1");
> >-  do_check_eq(root.getChild(2).itemId, -1);
> >   do_check_eq(root.getChild(2).title, "subfolder 2");
> > 
> >   // check the contents of the "test folder" container
> >   var rfNode = root.getChild(0);
> >   rfNode = rfNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
> >   rfNode.containerOpen = true;
> >   do_check_eq(rfNode.childCount, 1);
> >   do_check_eq(rfNode.getChild(0).itemId, b1);
> 
> What caused this change?
> 

See @@ -4219,27 +4201,26 @@ nsNavHistory::GroupByFolder(nsNavHistory

> >Index: browser/components/places/content/places.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
> >retrieving revision 1.111
> >diff -u -p -8 -r1.111 places.js
> >--- browser/components/places/content/places.js	25 Oct 2007 02:37:05 -0000	1.111
> >+++ browser/components/places/content/places.js	3 Nov 2007 19:57:18 -0000
> >@@ -32,39 +32,107 @@
> >  * use your version of this file under the terms of the MPL, indicate your
> >  * decision by deleting the provisions above and replace them with the notice
> >  * and other provisions required by the GPL or the LGPL. If you do not delete
> >  * the provisions above, a recipient may use your version of this file under
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> >-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+const QUERIES_ROOT_ANNO = "PlacesOrganizer/QueriesRoot";
> >+const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
> > 
> > /**
> >  * Selects a place URI in the places list.
> >  * This function is global so it can be easily accessed by openers.
> >  * @param   placeURI
> >  *          A place: URI string to select
> >  */
> > function selectPlaceURI(placeURI) {
> >   PlacesOrganizer._places.selectPlaceURI(placeURI);
> > }
> > 
> > var PlacesOrganizer = {
> >   _places: null,
> >   _content: null,
> > 
> >+  _initFolderTree: function() {
> >+    var queriesRoot;
> >+    var items = PlacesUtils.annotations
> >+                           .getItemsWithAnnotation(QUERIES_ROOT_ANNO, { });
> >+    if (items.length > 0)
> >+      queriesRoot = items[0];
> >+    else {
> >+      var callback = {
> >+        runBatched: function(aUserData) {
> >+          var bms = PlacesUtils.bookmarks;
> >+          var ans = PlacesUtils.annotations;
> >+
> >+          // Root of All Magic Queries
> >+          queriesRoot = bms.createFolder(PlacesUtils.placesRootId, "", -1);
> >+          ans.setItemAnnotation(queriesRoot, QUERIES_ROOT_ANNO, 1, 0,
> >+                                ans.EXPIRE_NEVER);
> >+
> >+          // History Query
> >+          var uri = IO.newURI("place:sort=4&");
> >+          var title = PlacesUtils.getString("OrganizerQueryHistory");
> >+          var itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
> >+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, "History", 0,
> >+                                ans.EXPIRE_NEVER);
> >+
> >+          // Tags Query
> >+          uri = IO.newURI("place:excludeItems=1&folder=" + PlacesUtils.tagRootId);
> >+          title = PlacesUtils.getString("OrganizerQueryTags");
> >+          itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
> >+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, "Tags", 0,
> >+                                ans.EXPIRE_NEVER);
> >+
> >+          // Bookmarks Menu Query
> >+          uri = IO.newURI("place:excludeItems=1&folder=" +
> >+                          PlacesUtils.bookmarksRootId);
> >+          title = PlacesUtils.getString("OrganizerQueryBookmarksMenu");
> >+          itemId = bms.insertBookmark(queriesRoot, uri, -1,title);
> 
> nit: space before "title"
> 
> >+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, "BookmarksMenu",
> >+                                0, ans.EXPIRE_NEVER);
> >+
> >+          // Bookmarks Toolbar Query
> >+          uri = IO.newURI("place:excludeItems=1&folder=" +
> >+                          PlacesUtils.toolbarFolderId);
> >+          title = PlacesUtils.getString("OrganizerQueryBookmarksToolbar");
> >+          itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
> >+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO,
> >+                                "BookmarksToolbar", 0, ans.EXPIRE_NEVER);
> >+
> >+          // All Bookmarks query
> >+          uri = IO.newURI("place:queryType=1&folder=" + PlacesUtils.bookmarksRootId +
> >+                          "&folder=" + PlacesUtils.unfiledRootId +
> >+                          "&excludeItemIfParentHasAnnotation=livemark/feedURI");
> >+          title = PlacesUtils.getString("OrganizerQueryAllBookmarks");
> >+          itemId = bms.insertBookmark(queriesRoot, uri, -1, title);
> >+          ans.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO,
> >+                                "AllBookmarks", 0, ans.EXPIRE_NEVER);
> >+
> >+          // disallow manipulating this folder within the organizer UI
> >+          PlacesUtils.bookmarks.setFolderReadonly(queriesRoot, true);
> 
> nit: could do s/PlacesUtils.bookmarks/bms/ here
> 
> >+        }
> >+      };
> >+      PlacesUtils.bookmarks.runInBatchMode(callback, null);
> >+    }
> >+    
> >+    this._places.place = "place:excludeItems=1&expandQueries=0&folder=" + queriesRoot;
> >+  },
> 
> hm, it's conceivable that these could get deleted somehow. should there be a
> pref to regenerate them?
> 

it's marked read-only, meaning you cannot remove the items from within the UI.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

>>- seems weird to click on "organize bookmarks" and have history be the default
>>view, should be "all bookmarks" maybe?

>Bookmarks-Menu was alex's plan, I think.

>>- "all bookmarks" should be above the menu and toolbar items

>not according to the mockup

The default view should be All Bookmarks, the reason this is last is so when the tree is expanded one level by default, the rest of the top level items are still visible.

Comment 16

[Asaf Romano (gone)]

Attachment: more...

Comment 17

[Asaf Romano (gone)]

Attachment: more...

Comment 18

[Asaf Romano (gone)]

Attachment: more...

Comment 19

[Asaf Romano (gone)]

Attachment: more...

Comment 20

[Asaf Romano (gone)]

Attachment: fix the tests

Comment 21

[Dietrich Ayala (:dietrich)]

tested it out, looks ok for the most part. a couple of comments:

- the 3 entries under "All Bookmarks" are all cut off in the default view, which is awkward. increasing the default width of the left pane would help somewhat. actually, since it's now the only tree, and the main navigational device, we should do that anyway.

- if you click on the "Tags" folder, then hit tab, the first folder in the right pane is not selected. you can still arrow up and down, and selection works fine after that (even back and forth between panes).

- the lack of keyboard access for browsing in the right pane is painful. maybe enter/return on a container should open it?

- selecting "All Bookmarks" on load is sometimes odd, for example, you'd previously expanded the "Tags" folder.

Comment 22

[Asaf Romano (gone)]

Attachment: more...

Comment 23

[Asaf Romano (gone)]

Comment on attachment 289088 [details] [diff] [review]
more...

So, as noted on IRC, there's still a little more work to do here (in particular, fix importDefaults), but otherwise this is ready for a first pass.

Comment 24

[Dietrich Ayala (:dietrich)]

Comment on attachment 289088 [details] [diff] [review]
more...

></div></DIV><BODY><PRE>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.49
>diff -u -p -8 -r1.49 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl	17 Sep 2007 01:42:16 -0000	1.49
>+++ toolkit/components/places/public/nsINavBookmarksService.idl	17 Nov 2007 01:20:19 -0000
>@@ -158,43 +158,45 @@ interface nsINavBookmarkObserver : nsISu
> };
> 
> /**
>  * The BookmarksService interface provides methods for managing bookmarked
>  * history items.  Bookmarks consist of a set of user-customizable
>  * folders.  A URI in history can be contained in one or more such folders.
>  */
> 
>-[scriptable, uuid(3ba9f6ca-0003-43b9-bdfb-7014dfec5b76)]
>+[scriptable, uuid(fb8dc678-681b-453b-a5fe-ab3cc19b3698)]
> interface nsINavBookmarksService : nsISupports
> {
>+  // XXXmano: rename these attributes, there's only one root!
>+

please file a followup, or do it in this bug. if we don't do it in fx3, it ain't gonna happen.

>@@ -436,81 +433,102 @@ nsNavBookmarks::InitRoots()
>   PRBool importDefaults = PR_FALSE;
>   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("places"), &mRoot, 0, &importDefaults);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   getRootStatement->Reset();
>   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("menu"), &mBookmarksRoot, mRoot, nsnull);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  PRBool createdToolbarFolder;
>+  getRootStatement->Reset();
>+  rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("toolbar"), &mToolbarFolder, mRoot, &createdToolbarFolder);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Once toolbar was not a root, we may need to move over the items and
>+  // delete the custom folder
>+  if (!importDefaults && createdToolbarFolder) {
>+    nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
>+    NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
>+
>+    nsTArray<PRInt64> folders;
>+    annosvc->GetItemsWithAnnotationTArray(BOOKMARKS_TOOLBAR_FOLDER_ANNO,
>+                                          &folders);
>+    if (folders.Length() > 0) {
>+      nsCOMPtr<mozIStorageStatement> moveItems;
>+      rv = DBConn()->CreateStatement(NS_LITERAL_CSTRING("UPDATE moz_bookmarks SET parent = ?1 WHERE parent=?2"),
>+                                     getter_AddRefs(moveItems));
>+      rv = moveItems->BindInt64Parameter(0, mToolbarFolder);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = moveItems->BindInt64Parameter(1, folders[0]);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = moveItems->Execute();
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = RemoveFolder(folders[0]);
>+      NS_ENSURE_SUCCESS(rv, rv);

maybe we should only remove the old folder if it's parent is not the bookmarks menu, as some users may have set their toolbar folder as one in a specifically constructed hierarchy.

>+    }
>+  }
>+
>   getRootStatement->Reset();
>   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("tags"), &mTagRoot, mRoot, nsnull);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   getRootStatement->Reset();
>   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("unfiled"), &mUnfiledRoot, mRoot, nsnull);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (importDefaults) {
>+  //if (importDefaults) {
>     // when there is no places root, we should define the hierarchy by
>     // importing the default one.
>     rv = InitDefaults();
>     NS_ENSURE_SUCCESS(rv, rv);
>-  }
>+  //}

why do we want this to run at every startup?

> 
> NS_IMETHODIMP
>-nsNavBookmarks::SetToolbarFolder(PRInt64 aFolderId)
>-{

should relnote the loss of this functionality?

>Index: toolkit/components/places/src/nsNavHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v
>retrieving revision 1.190
>diff -u -p -8 -r1.190 nsNavHistory.cpp
>--- toolkit/components/places/src/nsNavHistory.cpp	13 Nov 2007 02:54:32 -0000	1.190
>+++ toolkit/components/places/src/nsNavHistory.cpp	17 Nov 2007 01:20:45 -0000

>@@ -4185,27 +4170,26 @@ nsNavHistory::GroupByFolder(nsNavHistory
>       rv = CreatePlacesPersistURN(aResultNode, parentId, NS_ConvertUTF16toUTF8(title), urn);
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       PRInt64 grandparentId;
>       rv = bookmarks->GetFolderIdForItem(parentId, &grandparentId);
>       NS_ENSURE_SUCCESS(rv, rv);

unused now.

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.183
>diff -u -p -8 -r1.183 controller.js
>--- browser/components/places/content/controller.js	25 Oct 2007 02:02:28 -0000	1.183
>+++ browser/components/places/content/controller.js	17 Nov 2007 01:21:35 -0000
>@@ -443,17 +418,16 @@ PlacesController.prototype = {
>         case Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY:
>           nodeData["query"] = true;
>           break;
>         case Ci.nsINavHistoryResultNode.RESULT_TYPE_DYNAMIC_CONTAINER:
>           nodeData["dynamiccontainer"] = true;
>           break;
>         case Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER:
>           nodeData["folder"] = true;
>-          uri = PlacesUtils.bookmarks.getFolderURI(node.itemId);
>           break;

this causes annotations to not be included in the node metadata for folders.

does this matter? are there any consumers of folder node annotation metadata?

>@@ -717,36 +677,20 @@ PlacesController.prototype = {
> 
>   /**
>    * Reloads the livemarks associated with the selection. For the 
>    * "Subscriptions" folder, reloads all livemarks; for a livemark folder,
>    * reloads its children; for a single livemark, reloads its siblings (the
>    * children of its parent).
>    */
>   reloadSelectedLivemarks: function PC_reloadSelectedLivemarks() {

can you update the comment? should probably also rename the method at some point.

>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.111
>diff -u -p -8 -r1.111 places.js
>--- browser/components/places/content/places.js	25 Oct 2007 02:37:05 -0000	1.111
>+++ browser/components/places/content/places.js	17 Nov 2007 01:21:45 -0000
>@@ -32,49 +32,56 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>-
> /**
>  * Selects a place URI in the places list.
>  * This function is global so it can be easily accessed by openers.
>  * @param   placeURI
>  *          A place: URI string to select
>  */
> function selectPlaceURI(placeURI) {
>   PlacesOrganizer._places.selectPlaceURI(placeURI);
> }
> 
> var PlacesOrganizer = {
>   _places: null,
>   _content: null,
> 
>+  _initFolderTree: function() {
>+    var leftPaneRoot = PlacesUtils.leftPaneFolderId;
>+    var allBookmarksId = PlacesUtils.allBookmarksFolderId;
>+    this._places.place = "place:excludeItems=1&expandQueries=0&folder=" + leftPaneRoot;
>+    this._places.selectItems([allBookmarksId]);
>+    asContainer(this._places.selectedNode).containerOpen = true;
>+  },
>+
>   init: function PO_init() {
>     this._places = document.getElementById("placesList");
>     this._content = document.getElementById("placeContent");
>+    this._initFolderTree();
> 
>     OptionsFilter.init(Groupers);
>     Groupers.init();
>-
>+    

nit: whitespace

>     // Select the specified place in the places list.
>     var placeURI = "place:";
>     if ("arguments" in window)
>       placeURI = window.arguments[0];
> 
>-    selectPlaceURI(placeURI);
>+    //selectPlaceURI(placeURI);

nit: begone

also, seems like we should only do initFolderTree if there's not a placeURI passed in.

> 
>     var view = this._content.treeBoxObject.view;
>     if (view.rowCount > 0)
>-      view.selection.select(0);
>+     view.selection.select(0);

nit: indent

>@@ -1500,17 +1411,17 @@ var ViewMenu = {
> 
>     // If no column is "sort-active", the "Unsorted" item needs to be checked,
>     // so track whether or not we find a column that is sort-active.
>     var isSorted = false;
>     var content = document.getElementById("placeContent");
>     var columns = content.columns;
>     for (var i = 0; i < columns.count; ++i) {
>       var column = columns.getColumnAt(i).element;
>-      var menuitem = document.createElementNS(XUL_NS, "menuitem");
>+      var menuitem = document.createElement("menuitem");

why is that no longer needed? do nodes created via script inherit the document's default namespace?

>Index: browser/components/places/content/places.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.xul,v
>retrieving revision 1.93
>diff -u -p -8 -r1.93 places.xul
>--- browser/components/places/content/places.xul	9 Nov 2007 02:27:36 -0000	1.93
>+++ browser/components/places/content/places.xul	17 Nov 2007 01:21:46 -0000

>@@ -229,31 +226,36 @@
>           <menuseparator id="selectAllSeparator"/>
>           
>           <menuitem id="orgSelectAll"
>                     command="cmd_selectAll"
>                     label="&selectAllCmd.label;"
>                     key="key_selectAll"
>                     accesskey="&selectAllCmd.accesskey;"/>
> 
>+#endif
>           <menuseparator id="orgMoveSeparator"/>
>-
>+          <menuitem id="orgMoveBookmarks"
>+	          command="placesCmd_moveBookmarks"
>+		label="&cmd.moveBookmarks.label;"
>+		accesskey="cmd.moveBookmarks.accesskey"/>

nit: wonky indent, or wonky bugzilla


>Index: browser/components/places/content/tree.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/tree.xml,v
>retrieving revision 1.85
>diff -u -p -8 -r1.85 tree.xml
>--- browser/components/places/content/tree.xml	27 Oct 2007 21:55:53 -0000	1.85
>+++ browser/components/places/content/tree.xml	17 Nov 2007 01:21:53 -0000

>           /**
>-           * Recursively search through a node's children for folders
>-           * with IDs in the set "indexes".  When a matching folder
>-           * is found, remove its ID from "indexes" and store the
>-           * node in the dictionary "nodes".
>-           *
>-           * NOTE: This method will leave open any node that had
>-           *       matching folders in its subtree.
>+           * Recursively search through a node's children for items
>+           * the given IDs.  When a matching item is found, remove its ID

nit: s/items the given/items with the given/

>Index: browser/components/places/content/treeView.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
>retrieving revision 1.22
>diff -u -p -8 -r1.22 treeView.js
>--- browser/components/places/content/treeView.js	25 Oct 2007 02:02:29 -0000	1.22
>+++ browser/components/places/content/treeView.js	17 Nov 2007 01:21:56 -0000
>@@ -135,17 +119,16 @@ PlacesTreeView.prototype = {
> 
>     this._showSessions = true;
>   },
> 
>   SESSION_STATUS_NONE: 0,
>   SESSION_STATUS_START: 1,
>   SESSION_STATUS_CONTINUE: 2,
>   _getRowSessionStatus: function PTV__getRowSessionStatus(aRow) {
>-    this._ensureValidRow(aRow);

why?

>Index: browser/components/places/content/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/utils.js,v
>retrieving revision 1.76
>diff -u -p -8 -r1.76 utils.js
>--- browser/components/places/content/utils.js	13 Nov 2007 05:24:55 -0000	1.76
>+++ browser/components/places/content/utils.js	17 Nov 2007 01:22:00 -0000

>@@ -1702,16 +1702,99 @@ var PlacesUtils = {
>       element.setAttribute("label", aNode.title);
>       if (iconURISpec)
>         element.setAttribute("image", iconURISpec);
>     }
>     element.node = aNode;
>     element.node.viewIndex = 0;
> 
>     return element;
>+  },
>+
>+  // get the folder id for the organizer left-pane folder
>+  get leftPaneFolderId() {
>+    var prefs = Cc["@mozilla.org/preferences-service;1"].
>+                getService(Ci.nsIPrefBranch2);
>+    var leftPaneRoot = -1;
>+    var allBookmarksId;
>+    try {
>+      leftPaneRoot = prefs.getIntPref("browser.places.leftPaneFolderId");
>+      // oh, easy!
>+      delete this.leftPaneFolder;
>+      return this.leftPaneFolder = leftPaneRoot;

s/leftPaneFolder/leftPaneFolderId/

>Index: browser/components/places/src/nsPlacesImportExportService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v
>retrieving revision 1.32
>diff -u -p -8 -r1.32 nsPlacesImportExportService.cpp
>--- browser/components/places/src/nsPlacesImportExportService.cpp	13 Nov 2007 02:12:16 -0000	1.32
>+++ browser/components/places/src/nsPlacesImportExportService.cpp	17 Nov 2007 01:22:07 -0000

>@@ -2427,26 +2387,16 @@ nsPlacesImportExportService::ExportHTMLT
>   nsCOMPtr<nsINavHistoryContainerResultNode> rootNode;
>   rv = result->GetRoot(getter_AddRefs(rootNode));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // '<H1'
>   rv = strm->Write(kRootIntro, sizeof(kRootIntro)-1, &dummy); // <H1
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  // bookmarks menu favicon
>-  nsCOMPtr<nsIURI> folderURI;
>-  rv = mBookmarksService->GetFolderURI(bookmarksRoot, getter_AddRefs(folderURI));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  nsCAutoString folderSpec;
>-  rv = folderURI->GetSpec(folderSpec);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  rv = WriteFaviconAttribute(folderSpec, strm);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>   // '>Bookmarks</H1>
>   rv = strm->Write(kCloseAngle, sizeof(kCloseAngle)-1, &dummy); // >
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = WriteTitle(rootNode, strm);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = strm->Write(kCloseRootH1, sizeof(kCloseRootH1)-1, &dummy); // </H1>
>   NS_ENSURE_SUCCESS(rv, rv);

Given the toolbar reparenting, this won't write out the toolbar folder anymore, right?

Comment 25

[Asaf Romano (gone)]

(In reply to comment #24)
> >+  // XXXmano: rename these attributes, there's only one root!
> >+
> 
> please file a followup, or do it in this bug. if we don't do it in fx3, it
> ain't gonna happen.
> 

fixed.

> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+      rv = moveItems->Execute();
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+      rv = RemoveFolder(folders[0]);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> 
> maybe we should only remove the old folder if it's parent is not the bookmarks
> menu, as some users may have set their toolbar folder as one in a specifically
> constructed hierarchy.

Er, we're making the (custom or not) folder empty by moving all of its content to the special bookmarks toolbar folder, there's not much point in keeping the old folder in place (btw, this code path only affects alpha/trunk/beta users)

> >+    }
> >+  }
> >+
> >   getRootStatement->Reset();
> >   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("tags"), &mTagRoot, mRoot, nsnull);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   getRootStatement->Reset();
> >   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("unfiled"), &mUnfiledRoot, mRoot, nsnull);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-  if (importDefaults) {
> >+  //if (importDefaults) {
> >     // when there is no places root, we should define the hierarchy by
> >     // importing the default one.
> >     rv = InitDefaults();
> >     NS_ENSURE_SUCCESS(rv, rv);
> >-  }
> >+  //}
> 
> why do we want this to run at every startup?

I'm going to add a pref for this.
 
 
> >Index: toolkit/components/places/src/nsNavHistory.cpp
> >===================================================================
> >RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v
> >retrieving revision 1.190
> >diff -u -p -8 -r1.190 nsNavHistory.cpp
> >--- toolkit/components/places/src/nsNavHistory.cpp	13 Nov 2007 02:54:32 -0000	1.190
> >+++ toolkit/components/places/src/nsNavHistory.cpp	17 Nov 2007 01:20:45 -0000
> 
> >@@ -4185,27 +4170,26 @@ nsNavHistory::GroupByFolder(nsNavHistory
> >       rv = CreatePlacesPersistURN(aResultNode, parentId, NS_ConvertUTF16toUTF8(title), urn);
> >       NS_ENSURE_SUCCESS(rv, rv);
> > 
> >       PRInt64 grandparentId;
> >       rv = bookmarks->GetFolderIdForItem(parentId, &grandparentId);
> >       NS_ENSURE_SUCCESS(rv, rv);
> 
> unused now.

fixed.

 
> >Index: browser/components/places/content/controller.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
> >retrieving revision 1.183
> >diff -u -p -8 -r1.183 controller.js
> >--- browser/components/places/content/controller.js	25 Oct 2007 02:02:28 -0000	1.183
> >+++ browser/components/places/content/controller.js	17 Nov 2007 01:21:35 -0000
> >@@ -443,17 +418,16 @@ PlacesController.prototype = {
> >         case Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY:
> >           nodeData["query"] = true;
> >           break;
> >         case Ci.nsINavHistoryResultNode.RESULT_TYPE_DYNAMIC_CONTAINER:
> >           nodeData["dynamiccontainer"] = true;
> >           break;
> >         case Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER:
> >           nodeData["folder"] = true;
> >-          uri = PlacesUtils.bookmarks.getFolderURI(node.itemId);
> >           break;
> 
> this causes annotations to not be included in the node metadata for folders.
> 
> does this matter? are there any consumers of folder node annotation metadata?
>

Er, folder-uri annotations are no longer supported, item-annotations are handled after the switch block.
 
> >@@ -717,36 +677,20 @@ PlacesController.prototype = {
> > 
> >   /**
> >    * Reloads the livemarks associated with the selection. For the 
> >    * "Subscriptions" folder, reloads all livemarks; for a livemark folder,
> >    * reloads its children; for a single livemark, reloads its siblings (the
> >    * children of its parent).
> >    */
> >   reloadSelectedLivemarks: function PC_reloadSelectedLivemarks() {
> 
> can you update the comment? should probably also rename the method at some
> point.
> 
fixed.

> >Index: browser/components/places/content/places.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
> >retrieving revision 1.111
> >diff -u -p -8 -r1.111 places.js
> >--- browser/components/places/content/places.js	25 Oct 2007 02:37:05 -0000	1.111
> >+++ browser/components/places/content/places.js	17 Nov 2007 01:21:45 -0000
> >@@ -32,49 +32,56 @@
> >  * use your version of this file under the terms of the MPL, indicate your
> >  * decision by deleting the provisions above and replace them with the notice
> >  * and other provisions required by the GPL or the LGPL. If you do not delete
> >  * the provisions above, a recipient may use your version of this file under
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> >-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >-
> > /**
> >  * Selects a place URI in the places list.
> >  * This function is global so it can be easily accessed by openers.
> >  * @param   placeURI
> >  *          A place: URI string to select
> >  */
> > function selectPlaceURI(placeURI) {
> >   PlacesOrganizer._places.selectPlaceURI(placeURI);
> > }
> > 
> > var PlacesOrganizer = {
> >   _places: null,
> >   _content: null,
> > 
> >+  _initFolderTree: function() {
> >+    var leftPaneRoot = PlacesUtils.leftPaneFolderId;
> >+    var allBookmarksId = PlacesUtils.allBookmarksFolderId;
> >+    this._places.place = "place:excludeItems=1&expandQueries=0&folder=" + leftPaneRoot;
> >+    this._places.selectItems([allBookmarksId]);
> >+    asContainer(this._places.selectedNode).containerOpen = true;
> >+  },
> >+
> >   init: function PO_init() {
> >     this._places = document.getElementById("placesList");
> >     this._content = document.getElementById("placeContent");
> >+    this._initFolderTree();

> >     // Select the specified place in the places list.
> >     var placeURI = "place:";
> >     if ("arguments" in window)
> >       placeURI = window.arguments[0];
> > 
> >-    selectPlaceURI(placeURI);
> >+    //selectPlaceURI(placeURI);
> 
> nit: begone
> 
> also, seems like we should only do initFolderTree if there's not a placeURI
> passed in.
>

Well, i'm removing selectPlaceURI for now, better fix is coming in bug 399800.

> >@@ -1500,17 +1411,17 @@ var ViewMenu = {
> > 
> >     // If no column is "sort-active", the "Unsorted" item needs to be checked,
> >     // so track whether or not we find a column that is sort-active.
> >     var isSorted = false;
> >     var content = document.getElementById("placeContent");
> >     var columns = content.columns;
> >     for (var i = 0; i < columns.count; ++i) {
> >       var column = columns.getColumnAt(i).element;
> >-      var menuitem = document.createElementNS(XUL_NS, "menuitem");
> >+      var menuitem = document.createElement("menuitem");
> 
> why is that no longer needed? do nodes created via script inherit the
> document's default namespace?
>

yes, createElementNS is only necessary if the doucment namespace is unknow (toolkit/content/widgets for example).

> >Index: browser/components/places/content/treeView.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
> >retrieving revision 1.22
> >diff -u -p -8 -r1.22 treeView.js
> >--- browser/components/places/content/treeView.js	25 Oct 2007 02:02:29 -0000	1.22
> >+++ browser/components/places/content/treeView.js	17 Nov 2007 01:21:56 -0000
> >@@ -135,17 +119,16 @@ PlacesTreeView.prototype = {
> > 
> >     this._showSessions = true;
> >   },
> > 
> >   SESSION_STATUS_NONE: 0,
> >   SESSION_STATUS_START: 1,
> >   SESSION_STATUS_CONTINUE: 2,
> >   _getRowSessionStatus: function PTV__getRowSessionStatus(aRow) {
> >-    this._ensureValidRow(aRow);
> 
> why?
>

see callers.
 
> >Index: browser/components/places/content/utils.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/content/utils.js,v
> >retrieving revision 1.76
> >diff -u -p -8 -r1.76 utils.js
> >--- browser/components/places/content/utils.js	13 Nov 2007 05:24:55 -0000	1.76
> >+++ browser/components/places/content/utils.js	17 Nov 2007 01:22:00 -0000
> 
> >@@ -1702,16 +1702,99 @@ var PlacesUtils = {
> >       element.setAttribute("label", aNode.title);
> >       if (iconURISpec)
> >         element.setAttribute("image", iconURISpec);
> >     }
> >     element.node = aNode;
> >     element.node.viewIndex = 0;
> > 
> >     return element;
> >+  },
> >+
> >+  // get the folder id for the organizer left-pane folder
> >+  get leftPaneFolderId() {
> >+    var prefs = Cc["@mozilla.org/preferences-service;1"].
> >+                getService(Ci.nsIPrefBranch2);
> >+    var leftPaneRoot = -1;
> >+    var allBookmarksId;
> >+    try {
> >+      leftPaneRoot = prefs.getIntPref("browser.places.leftPaneFolderId");
> >+      // oh, easy!
> >+      delete this.leftPaneFolder;
> >+      return this.leftPaneFolder = leftPaneRoot;
> 
> s/leftPaneFolder/leftPaneFolderId/
>

*oops*
 
> >Index: browser/components/places/src/nsPlacesImportExportService.cpp
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v
> >retrieving revision 1.32
> >diff -u -p -8 -r1.32 nsPlacesImportExportService.cpp
> >--- browser/components/places/src/nsPlacesImportExportService.cpp	13 Nov 2007 02:12:16 -0000	1.32
> >+++ browser/components/places/src/nsPlacesImportExportService.cpp	17 Nov 2007 01:22:07 -0000
> 
> >@@ -2427,26 +2387,16 @@ nsPlacesImportExportService::ExportHTMLT
> >   nsCOMPtr<nsINavHistoryContainerResultNode> rootNode;
> >   rv = result->GetRoot(getter_AddRefs(rootNode));
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   // '<H1'
> >   rv = strm->Write(kRootIntro, sizeof(kRootIntro)-1, &dummy); // <H1
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-  // bookmarks menu favicon
> >-  nsCOMPtr<nsIURI> folderURI;
> >-  rv = mBookmarksService->GetFolderURI(bookmarksRoot, getter_AddRefs(folderURI));
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-  nsCAutoString folderSpec;
> >-  rv = folderURI->GetSpec(folderSpec);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-  rv = WriteFaviconAttribute(folderSpec, strm);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-
> >   // '>Bookmarks</H1>
> >   rv = strm->Write(kCloseAngle, sizeof(kCloseAngle)-1, &dummy); // >
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   rv = WriteTitle(rootNode, strm);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   rv = strm->Write(kCloseRootH1, sizeof(kCloseRootH1)-1, &dummy); // </H1>
> >   NS_ENSURE_SUCCESS(rv, rv);
> 
> Given the toolbar reparenting, this won't write out the toolbar folder anymore,
> right?
> 
yes, should probably get fixed along with the export-roots bug.

Comment 26

[Asaf Romano (gone)]

Attachment: another checkpoint

Comment 27

[Asaf Romano (gone)]

Attachment: all of it

Comment 28

[Dietrich Ayala (:dietrich)]

Comment on attachment 289256 [details] [diff] [review]
all of it

nothing major in the latest changes, some nits below. thanks for fixing export and the other review changes.

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.49
>diff -u -p -8 -r1.49 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl  17 Sep 2007 01:42:16 -0000  1.49
>+++ toolkit/components/places/public/nsINavBookmarksService.idl  18 Nov 2007 23:33:47 -0000
>@@ -158,43 +158,43 @@ interface nsINavBookmarkObserver : nsISu
> };
> 
> 
>   /**
>    * The folder ID of the personal toolbar.
>    */
>-  attribute long long toolbarFolder;
>+  readonly attribute long long toolbarFolder;
> 

nit: make the same s/folder/item/ comment change here?


>Index: toolkit/components/places/src/nsNavBookmarks.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v
>retrieving revision 1.126
>diff -u -p -8 -r1.126 nsNavBookmarks.cpp
>--- toolkit/components/places/src/nsNavBookmarks.cpp 9 Nov 2007 21:21:38 -0000 1.126
>+++ toolkit/components/places/src/nsNavBookmarks.cpp 18 Nov 2007 23:34:04 -0000

>+  PRBool importDefaults = PR_TRUE;
>+  rv = prefBranch->GetBoolPref("browser.places.importDefaults", &importDefaults);
>+  if (NS_FAILED(rv) || importDefaults) {
>     rv = InitDefaults();
>     NS_ENSURE_SUCCESS(rv, rv);
>+    prefBranch->SetBoolPref("browser.places.importDefaults", PR_FALSE);

nit: check rv?

>Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v
>retrieving revision 1.37
>diff -u -p -8 -r1.37 test_bookmarks.js
>--- toolkit/components/places/tests/bookmarks/test_bookmarks.js  11 Oct 2007 06:42:40 -0000  1.37
>+++ toolkit/components/places/tests/bookmarks/test_bookmarks.js  18 Nov 2007 23:34:36 -0000
>@@ -99,47 +99,49 @@ var observer = {
>         iid.equals(Ci.nsISupports)) {
>       return this;
>     }
>     throw Cr.NS_ERROR_NO_INTERFACE;
>   },
> };
> bmsvc.addObserver(observer, false);
> 
>-// get bookmarks root id
>-var root = bmsvc.bookmarksRoot;
>+// get bookmarks menu folder id
>+var root = bmsvc.bookmarksMenuFolder;
> 
> // index at which items should begin
>-var bmStartIndex = 1;
>+var bmStartIndex = 0;
> 
> // main
> function run_test() {
>-  // test roots
>+  // test special folder

nit: s/folder/folders/

>Index: toolkit/components/places/tests/unit/test_331487.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_331487.js,v
>retrieving revision 1.1
>diff -u -p -8 -r1.1 test_331487.js
>--- toolkit/components/places/tests/unit/test_331487.js  11 Oct 2007 06:42:42 -0000  1.1
>+++ toolkit/components/places/tests/unit/test_331487.js  18 Nov 2007 23:34:38 -0000
>@@ -132,21 +132,18 @@ function run_test() {
>   query.setFolders([folder], 1);
>   var result = histsvc.executeQuery(query, options);
>   var root = result.root;
>   root.containerOpen = true;
>   do_check_eq(root.childCount, 3);
>   // these containers are nsNavHistoryContainerResultNodes, 
>   // created in GroupByFolder() and not nsNavHistoryFolderResultNodes, 
>   // so they should not have an item id.
>-  do_check_eq(root.getChild(0).itemId, -1);
>   do_check_eq(root.getChild(0).title, "test folder");
>-  do_check_eq(root.getChild(1).itemId, -1);
>   do_check_eq(root.getChild(1).title, "subfolder 1");
>-  do_check_eq(root.getChild(2).itemId, -1);
>   do_check_eq(root.getChild(2).title, "subfolder 2");

please change this comment

>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.111
>diff -u -p -8 -r1.111 places.js
>--- browser/components/places/content/places.js  25 Oct 2007 02:37:05 -0000  1.111
>+++ browser/components/places/content/places.js  18 Nov 2007 23:35:32 -0000

>   init: function PO_init() {
>     this._places = document.getElementById("placesList");
>     this._content = document.getElementById("placeContent");
>-
>-    OptionsFilter.init(Groupers);
>-    Groupers.init();
>-
>-    // Select the specified place in the places list.
>-    var placeURI = "place:";
>-    if ("arguments" in window)
>-      placeURI = window.arguments[0];

chucking this, and fixing in the other bug?

Comment 29

[Asaf Romano (gone)]

> chucking this, and fixing in the other bug?

correct.

Comment 30

[Asaf Romano (gone)]

Attachment: for checkin

Comment 31

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-places.js, 1.66
mozilla/browser/base/content/browser-sets.inc 1.105
mozilla/browser/base/content/browser.js 1.894
mozilla/browser/base/content/nsContextMenu.js 1.29
mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp 1.27
mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp 1.67
mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 1.73
mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp 1.47
mozilla/browser/components/places/content/bookmarkProperties.js 1.64
mozilla/browser/components/places/content/bookmarkProperties.xul 1.30
mozilla/browser/components/places/content/bookmarksPanel.js 1.5
mozilla/browser/components/places/content/bookmarksPanel.xul 1.8
mozilla/browser/components/places/content/controller.js 1.185
mozilla/browser/components/places/content/editBookmarkOverlay.js 1.11
mozilla/browser/components/places/content/editBookmarkOverlay.xul 1.9
mozilla/browser/components/places/content/menu.xml 1.92
mozilla/browser/components/places/content/moveBookmarks.js  1.12
mozilla/browser/components/places/content/moveBookmarks.xul 1.8
mozilla/browser/components/places/content/places.js 1.112
mozilla/browser/components/places/content/places.xul 1.95
mozilla/browser/components/places/content/placesOverlay.xul 1.20
mozilla/browser/components/places/content/toolbar.xml 1.111
mozilla/browser/components/places/content/tree.xml 1.86
mozilla/browser/components/places/content/treeView.js 1.23
mozilla/browser/components/places/content/utils.js 1.77
mozilla/browser/components/places/public/nsIPlacesTransactionsService.idl 1.4
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.33
mozilla/browser/components/places/src/nsPlacesTransactionsService.js 1.6
mozilla/browser/components/places/tests/unit/test_bookmarks_html.js 1.12
mozilla/browser/components/places/tests/unit/test_placesTxn.js 1.7
mozilla/browser/locales/en-US/chrome/browser/places/editBookmarkOverlay.dtd 1.5
mozilla/browser/locales/en-US/chrome/browser/places/places.dtd 1.37
mozilla/browser/locales/en-US/chrome/browser/places/places.properties 1.31
mozilla/browser/themes/pinstripe/browser/browser.css 1.95
mozilla/browser/themes/pinstripe/browser/places/places.css 1.17
mozilla/browser/themes/winstripe/browser/browser.css 1.134
mozilla/browser/themes/winstripe/browser/places/places.css 1.19
mozilla/extensions/metrics/src/nsProfileCollector.cpp 1.17
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.50
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.72
mozilla/toolkit/components/places/public/nsITaggingService.idl 1.8
mozilla/toolkit/components/places/src/nsLivemarkService.js 1.27
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.128
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.49
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.192
mozilla/toolkit/components/places/src/nsNavHistory.h 1.111
mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp 1.25
mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp 1.35
mozilla/toolkit/components/places/src/nsNavHistoryQuery.h 1.18
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.122
mozilla/toolkit/components/places/src/nsTaggingService.js 1.5
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.38
mozilla/toolkit/components/places/tests/bookmarks/test_sort_by_count.js 1.2
mozilla/toolkit/components/places/tests/unit/test_331487.js 1.2
mozilla/toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js  1.3
mozilla/toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js 1.2
mozilla/toolkit/components/places/tests/unit/test_tagging.js 1.6
mozilla/toolkit/locales/en-US/chrome/places/places.properties 1.2

Comment 32

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111918 Minefield/3.0b2pre ID:2007111918

"search in bookmarks" is broken.

"bookmarks toolbar" under "Bookmarks" menu is gone.
http://forums.mozillazine.org/viewtopic.php?p=3148546#3148546

Comment 33

[Asaf Romano (gone)]

Attachment: the fix for import-test orange

Comment 34

[Asaf Romano (gone)]

for which the revisions are:
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.34
mozilla/browser/components/places/tests/unit/bookmarks.preplaces.html 1.6
mozilla/browser/components/places/tests/unit/test_bookmarks_html.js 1.13

Comment 35

[Dietrich Ayala (:dietrich)]

Comment on attachment 289431 [details] [diff] [review]
the fix for import-test orange

ok for wallpaper. however, we should retain the original position. please file a followup for this.

Comment 36

[Asaf Romano (gone)]

Fuel fix:
Checking in mozilla/browser/fuel/src/fuelApplication.js;
/cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v  <--  fuelApplication.js
new revision: 1.22; previous revision: 1.21
done

Comment 37

[Asaf Romano (gone)]

Attachment: fix style rules bustage

Organizer-specific rules were applied to the browser window after opening the editBookmarkPanel.

Comment 38

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111920 Minefield/3.0b2pre ID:2007111920
(20071119_1951_firefox-3.0b2pre.en-US.win32.zip)

after "restore", contents in bookmarks toolbar appears twice.
seems to be not replaced.

Comment 39

[Asaf Romano (gone)]

mozilla/browser/components/places/content/places.xul 1.96
mozilla/browser/themes/pinstripe/browser/jar.mn 1.64
mozilla/browser/themes/pinstripe/browser/places/organizer.css initial revision: 1.1
mozilla/browser/themes/pinstripe/browser/places/places.css 1.18
mozilla/browser/themes/winstripe/browser/jar.mn 1.60
mozilla/browser/themes/winstripe/browser/places/editBookmarkOverlay.css 1.2
mozilla/browser/themes/winstripe/browser/places/organizer.css initial revision: 1.1
mozilla/browser/themes/winstripe/browser/places/places.css 1.20

Comment 40

[Asaf Romano (gone)]

pal-moz: file bugs.

Comment 41

[pal-moz]

(In reply to comment #40)
> pal-moz: file bugs.
> 

which one? all?

(In reply to comment #32)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111918
> Minefield/3.0b2pre ID:2007111918
> 
> "search in bookmarks" is broken.
> 
> "bookmarks toolbar" under "Bookmarks" menu is gone.
> http://forums.mozillazine.org/viewtopic.php?p=3148546#3148546
> 

(In reply to comment #38)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111920
> Minefield/3.0b2pre ID:2007111920
> (20071119_1951_firefox-3.0b2pre.en-US.win32.zip)
> 
> after "restore", contents in bookmarks toolbar appears twice.
> seems to be not replaced.
> 

Comment 42

[Asaf Romano (gone)]

Re-parenting the toolbar folder is intentional (we're going to add a static menu-item for it under the bookmarks menu though), breaking search or duplicating your bookmarks isn't.

Comment 43

[pal-moz]

filed,
bug 404446
bug 404447
bug 404448

Comment 44

[Bill Gianopoulos [:WG9s]]

filed

bug 404452

Comment 45

[pal-moz]

20071119_2107_firefox-3.0b2pre.en-US.win32.zip

nothing in Places Organizer.

Comment 46

[pal-moz]

(In reply to comment #45)
> 20071119_2107_firefox-3.0b2pre.en-US.win32.zip
> 
> nothing in Places Organizer.
> 

in error console,

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsINavHistoryService.executeQueries]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/places/tree.xml :: load :: line 101"  data: no]

Comment 47

[Dietrich Ayala (:dietrich)]

hm, i'm not seeing that error, however i am seeing various states of brokenness in different profiles that i try.

pal-moz, can you try resetting these preferences and restarting:

browser.places.allBookmarksFolderId
browser.places.leftPaneFolderId

Comment 48

[Reed Loden [:reed]]

You completely forgot about gnomestripe here. Filed bug 404475 on that.

Comment 49

[Reed Loden [:reed]]

Could this patch have caused a 5ms Ts regression? It's one of three bugs in the bonsai query.

Comment 50

[Ronny Perinke]

the patch probably re-introduced the problems explained in bug 388148

Comment 51

[Ronny Perinke]

Attachment: fix typo in menu.xml

adding a new livemark is not marked as livemark because of a small typo

Comment 52

[Asaf Romano (gone)]

Comment on attachment 291106 [details] [diff] [review]
fix typo in menu.xml

r=mano, thanks!

Comment 53

[Reed Loden [:reed]]

Comment on attachment 291106 [details] [diff] [review]
fix typo in menu.xml

Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.93; previous revision: 1.92
done

Comment 54

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

Comment 55

[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

Comment 56

[Alfred Kayser]

Some left over 'deadcode' is:
#define BOOKMARKS_MENU_ICON_URI "chrome://browser/skin/places/bookmarksMenu.png"
This define is now no longer used, and can be removed.