413107 - Add Bookmark dialog's folder selector broken

Status: VERIFIED FIXED

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

Description

[Ria Klaassen (not reading all bugmail)]

Steps to reproduce:

- start Firefox
- go to an un-bookmarked site (e.g. one of the URLs on this page)
- click the star on the location bar
- click the star once more
- in the add bookmark popup, click the rightmost arrow to select a folder
- choose bookmarks toolbar

Expected result: the bookmark appears on the bookmarks toolbar.

Actual result: no bookmark appears on the toolbar; the folder name in the pop-up disappears. The star is still yellow and stays yellow, also after a restart. A search in the Library window gives no result, but typing in the URL bar shows the bookmarked site in the autocomplete dropdown.

Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1199850360&maxdate=1199852579 
Bug 410346 ?

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 298811 [details] [diff] [review]
patch

>Index: toolkit/components/places/public/nsINavHistoryService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v
>retrieving revision 1.76
>diff -u -p -8 -r1.76 nsINavHistoryService.idl
>--- toolkit/components/places/public/nsINavHistoryService.idl	29 Dec 2007 02:59:23 -0000	1.76
>+++ toolkit/components/places/public/nsINavHistoryService.idl	24 Jan 2008 00:01:41 -0000
>@@ -78,18 +78,19 @@ interface nsINavHistoryResultNode : nsIS
>    */
>   const unsigned long RESULT_TYPE_URI = 0;               // nsINavHistoryResultNode
>   const unsigned long RESULT_TYPE_VISIT = 1;             // nsINavHistoryVisitResultNode
>   const unsigned long RESULT_TYPE_FULL_VISIT = 2;        // nsINavHistoryFullVisitResultNode
>   const unsigned long RESULT_TYPE_HOST = 3;              // nsINavHistoryContainerResultNode
>   const unsigned long RESULT_TYPE_DYNAMIC_CONTAINER = 4; // nsINavHistoryContainerResultNode
>   const unsigned long RESULT_TYPE_QUERY = 5;             // nsINavHistoryQueryResultNode
>   const unsigned long RESULT_TYPE_FOLDER = 6;            // nsINavHistoryQueryResultNode
>-  const unsigned long RESULT_TYPE_SEPARATOR = 7;         // nsINavHistoryResultNode
>-  const unsigned long RESULT_TYPE_DAY = 8;               // nsINavHistoryContainerResultNode
>+  const unsigned long RESULT_TYPE_FOLDER_SHORTCUT = 7;   // nsINavHistoryQueryResultNode
>+  const unsigned long RESULT_TYPE_SEPARATOR = 8;         // nsINavHistoryResultNode
>+  const unsigned long RESULT_TYPE_DAY = 9;               // nsINavHistoryContainerResultNode
>   readonly attribute unsigned long type;

update UUID

>@@ -751,40 +751,48 @@
>             if (PlacesUtils.nodeIsFolder(xulNode.node) &&
>                 !PlacesUtils.nodeIsReadOnly(xulNode.node)) {
>               NS_ASSERT(xulNode.getAttribute("type") == "menu");
>               // This is a folder. If the mouse is in the left 25% of the
>               // node, drop to the left of the folder.  If it's in the middle
>               // 50%, drop into the folder.  If it's past that, drop to the right.
>               if (event.clientX < xulNode.boxObject.x + (xulNode.boxObject.width * 0.25)) {
>                 // Drop to the left of this folder.
>-                dropPoint.ip = new InsertionPoint(result.root.itemId, i, -1);
>+                dropPoint.ip =
>+	        new InsertionPoint(PlacesUtils.getConcreteItemId(result.root),
>+	                           i, -1);

nit: funky indent (+ next 3 changes), or is this bugzilla prob, as there's indent issues throughout the patch...

>Index: browser/components/places/content/tree.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/tree.xml,v
>retrieving revision 1.89
>diff -u -p -8 -r1.89 tree.xml
>--- browser/components/places/content/tree.xml	11 Jan 2008 22:04:01 -0000	1.89
>+++ browser/components/places/content/tree.xml	24 Jan 2008 00:05:15 -0000
>@@ -509,34 +509,33 @@
>           // than adjacent to it. Note that this only applies to _single_ 
>           // selections - if the last element within a multi-selection is an
>           // open folder, insert _adajacent_ to the selection.
>           //
>           // If the sole selection is the bookmarks toolbar folder, we insert
>           // into it even if it is not opened
>           if (this.hasSingleSelection && resultView.isContainer(max.value) &&
>               (resultView.isContainerOpen(max.value) ||
>-               resultView.nodeForTreeIndex(max.value)
>-                         .itemId == PlacesUtils.bookmarksMenuFolderId))
>+               PlacesUtils.getConcreteItemId(
>+	       resultView.nodeForTreeIndex(max.value)) ==
>+                 PlacesUtils.bookmarksMenuFolderId))

that hurts. can you cache the concrete id in a local var or something?

>Index: browser/components/places/content/treeView.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
>retrieving revision 1.28
>diff -u -p -8 -r1.28 treeView.js
>--- browser/components/places/content/treeView.js	11 Jan 2008 22:04:01 -0000	1.28
>+++ browser/components/places/content/treeView.js	24 Jan 2008 00:05:33 -0000
>@@ -376,20 +376,21 @@ PlacesTreeView.prototype = {
>     // restore selection
>     if (previouslySelectedNodes.length > 0) {
>       for each (var nodeInfo in previouslySelectedNodes) {
>         var index = nodeInfo.node.viewIndex;
> 
>         // if the same node was used (happens on sorting-changes),
>         // just use viewIndex
>         if (index == -1) { // otherwise, try to find an equal node
>-          var itemId = nodeInfo.node.itemId;
>+          var itemId = PlacesUtils.getConcreteItemId(nodeInfo.node);
>           if (itemId != 1) { // bookmark-nodes in queries case
>             for (i=0; i < newElements.length && index == -1; i++) {
>-              if (newElements[i].itemId == itemId)
>+              if (PlacesUtils.getConcreteItemId(newElements[i]
>+                             .itemId) == itemId)
>                 index = newElements[i].viewIndex;

need to pass only the node to getConcreteItemId()...

r=me. i'm not happy about this, because it makes things a little trickier for extensions ("is this a real folder or a fake folder that might create a cycle??"), but i don't see a better way atm.

Comment 3

[Dietrich Ayala (:dietrich)]

cc'ing thunder as this will likely might require a weave update.

Comment 4

[Asaf Romano (gone)]

[20:59]	<Mano>	dietrich: why update uuid?
[21:00]	<Mano>	dietrich: adding constants doesn't break abi
[21:00]	<dietrich>	ok, nm then

Comment 5

[Asaf Romano (gone)]

Attachment: for checkin

Comment 6

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.194
mozilla/browser/components/places/content/editBookmarkOverlay.js 1.18
mozilla/browser/components/places/content/menu.xml 1.96
mozilla/browser/components/places/content/toolbar.xml 1.116
mozilla/browser/components/places/content/tree.xml 1.90
mozilla/browser/components/places/content/treeView.js 1.31
mozilla/browser/components/places/content/utils.js 1.97
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.77
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.148
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.128
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.53

Comment 7

[tabmix.onemen]

typo in menu.xml

folderId = PlacesUtils.getConcreteItemId(selectedNode);

need to be 

folderId = PlacesUtils.getConcreteItemId(this.selectedNode);

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #7)
> typo in menu.xml
> 
> folderId = PlacesUtils.getConcreteItemId(selectedNode);
> 
> need to be 
> 
> folderId = PlacesUtils.getConcreteItemId(this.selectedNode);

I have filed this as bug 414167.

Comment 9

[Mike Lierman]

*** VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

Comment 10

[Carsten Book [:Tomcat]]

verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032614 Minefield/3.0b5pre ID:2008032614 and the steps to reproduce from Ria

--> Verified

Comment 11

[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