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 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.

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

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

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

typo in menu.xml folderId = PlacesUtils.getConcreteItemId(selectedNode); need to be folderId = PlacesUtils.getConcreteItemId(this.selectedNode);

(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.

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

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

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