stop hard coding folder roots in place: urls spun off from bug #387996 comment #23 we currently hard code the bookmark root (usually that's id 2) in eight places: from http://lxr.mozilla.org/seamonkey/search?string=place%3Afolder%3D2 /browser/base/content/browser-menubar.inc, line 378 -- place="place:folder=2&group=3&expandQueries=1" /browser/components/preferences/selectBookmark.xul, line 28 -- place="place:folder=2&group=3&excludeQueries=1" /browser/components/places/content/bookmarksPanel.xul, line 73 -- place="place:folder=2&queryType=1" /browser/components/places/content/bookmarkProperties.xul, line 167 -- place="place:folder=2&group=3&excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1" /browser/components/places/content/editBookmarkOverlay.xul, line 147 -- place="place:folder=2&group=3&excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1" /browser/components/places/content/moveBookmarks.xul, line 72 -- place="place:folder=2&group=3&excludeItems=1&excludeReadOnlyFolders=1" /browser/components/places/content/controller.js, line 43 -- const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&group=3&excludeItems=1&queryType=1"; /browser/components/places/content/places.xul, line 355 -- place="place:folder=2&group=3&excludeItems=1&queryType=1"

drivers: saved searches are not portable across profiles, nor across restores due to this bug.

note to dietrich and drivers, I think we could still hardcode folder roots, if we fixed bug #405938. note, separate from folder roots, we will have to hard code folder ids in place queries for user created folders.

> I think we could still hardcode folder roots actually, this bug is about something else. it is about the hard coding (and assumption) that folder id 2 is the bookmark root, which it may not always be. it appears that we only do this in 3 places now: /browser/base/content/browser-menubar.inc, line 378 -- place="place:folder=2&expandQueries=1" /browser/components/preferences/selectBookmark.xul, line 28 -- place="place:folder=2&excludeQueries=1" /browser/components/places/content/controller.js, line 43 -- const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&excludeItems=1&queryType=1"; > drivers: saved searches are not portable across profiles, nor across restores > due to this bug. that is bug #405938.

I'm not sure why bug 408125 depends on this - some care to explain?

I'm not doing so well with fixing this bug - suggestions on how to attack this?

(In reply to comment #4) > I'm not sure why bug 408125 depends on this - some care to explain? there is an hardcoded value there 28 place="place:folder=2&excludeQueries=1" so fixing there could remove one of the hardcoded values here... probably not really a dependance.

(In reply to comment #5) > I'm not doing so well with fixing this bug - suggestions on how to attack this? > what are you having problems with? you could define some consts on the idl that represent the special folders, and use those when encountered while parsing/serializing the query strings.

Well, a few issues really. The big one is how (even a general idea I can work off of would be great) do I go from that static query to doing a generated one? I'm going to assume now that it's an xbl binding (which I haven't been able to find yet) that takes that static query and generates the content. Is this correct? Pointing me to the xbl binding may be sufficient for all I know too. Did I ever mention that the places API is hard to get your head around?

test isn't working, but I'm not really sure how to fix it either...

yay - test finally works!

wrong root folder :/

if LXR does not fail ORGANIZER_ROOT_BOOKMARKS is not used anywhere (http://lxr.mozilla.org/seamonkey/search?string=ORGANIZER_ROOT_BOOKMARKS)... we could drop it (since that should be a root why is it actually pointing to folder=2 that appear to be the bookmarks menu?)

Comment on attachment 306071 [details] [diff] [review] v1.1 this looks fine so far. however, the other half of this fix is emitting the special identifiers when serializing queries (queriesToQueryString).

Now with more tests! (I ran out of cowbell) In an effort to better organize some of this code, I've gone and namespace'd the methods I added. Using namespaces is fine - we've done it in storage code already and nobody has complained. Self organizing code FTW!

Comment on attachment 306584 [details] [diff] [review] v2.0 whoops - should have checked make check output before submitting. I caused another test to fail because it was expecting a number...

silly hardcoded queries :)

Note: looking in the places organizer, I'm still seeing hardcoded values for the primary folders even though things are "fixed". Are those saved somewhere?

(In reply to comment #17) > Note: looking in the places organizer, I'm still seeing hardcoded values for > the primary folders even though things are "fixed". Are those saved somewhere? > here are the places that immediately come to mind: smart folders: http://mxr.mozilla.org/seamonkey/source/browser/components/nsBrowserGlue.js#539 "all bookmarks" folder children: http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js#1952 we really should fix that code to use queriesToQueryString() instead of manually building the URIs.

Why? place: uris are supposed to be human readable & writable. This is especially wrong given that this code is perf-sensitive.

(In reply to comment #19) > Why? place: uris are supposed to be human readable & writable. This is > especially wrong given that this code is perf-sensitive. > so that in cases like this we don't have to go update all callers? that said, i buy that the perf-sensitive nature of the browserglue code justifies manually constructing the URIs there.

Comment on attachment 306586 [details] [diff] [review] v2.1 >+ >+ // It wasn't one of our named constants, so just convert it to a string >+ aQuery.AppendInt(aFolderID); >+ } >+}; i don't think you need the end semicolon there. no compiler complaints on mac though... add the smart-bookmark and left-pane fixes previously mentioned and this should be good to go.

addresses review comments

Comment on attachment 308100 [details] [diff] [review] v2.2 >+function folder_id(aQuery) >+{ >+ var hs = Cc["@mozilla.org/browser/nav-history-service;1"]. >+ getService(Ci.nsINavHistoryService); >+ >+ dump("Checking query '" + aQuery + "'\n"); >+ var options = { }; >+ var queries = { }; >+ var size = { }; >+ hs.queryStringToQueries(aQuery, queries, size, options); >+ var result = hs.executeQueries(queries.value, size.value, options.value); >+ var root = result.root; >+ root.containerOpen = true; >+ do_check_true(root.hasChildren); >+ var folderID = root.getChild(0).parent.itemId; root and parent are the same node, should be able to do root.itemId instead. in fact, you could probably not even add the items or open the container for an isolated test of this change, just check root.itemId. no matter though, broader coverage is a-ok :) r=me

I found that root.hasChildren would be false if I didn't add anything. I never thought of checking the root.itemId. I'll look into that maybe later today.

talk about the bit rot :( Checking in browser/base/content/browser-menubar.inc; new revision: 1.153; previous revision: 1.152 Checking in browser/base/content/browser-places.js; new revision: 1.121; previous revision: 1.120 Checking in browser/components/nsBrowserGlue.js; new revision: 1.76; previous revision: 1.75 Checking in browser/components/places/content/controller.js; new revision: 1.224; previous revision: 1.223 Checking in browser/components/places/content/utils.js; new revision: 1.124; previous revision: 1.123 Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp; new revision: 1.37; previous revision: 1.36 Checking in toolkit/components/places/tests/unit/test_399264_query_to_string.js; initial revision: 1.1 Checking in toolkit/components/places/tests/unit/test_399264_string_to_query.js; initial revision: 1.1 Checking in toolkit/components/places/tests/unit/test_placeURIs.js; new revision: 1.3; previous revision: 1.2

Sometime between 2008031506 (OK) and 2008031514 (Broken), Places in new profiles is broken, the Library window is empty and nothing shows up in the Bookmarks sidebar. Could this be the cause?

(In reply to comment #26) > Sometime between 2008031506 (OK) and 2008031514 (Broken), Places in new > profiles is broken, the Library window is empty and nothing shows up in the > Bookmarks sidebar. Could this be the cause? > OK : 20080315_1126_firefox-3.0b5pre.en-US.win32.zip NG : 20080315_1259_firefox-3.0b5pre.en-US.win32.zip (this bug checked in) in Error Console, [open sidebar] Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.document]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/content/bindings/browser.xml :: get_contentDocument :: line 0" data: no] Error: uncaught exception: [Exception... "Component returned failure code: 0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS) [nsINavBookmarksService.runInBatchMode]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://browser/content/places/utils.js :: anonymous :: line 1180" data: no] [open Library] Error: uncaught exception: [Exception... "Component returned failure code: 0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS) [nsINavBookmarksService.runInBatchMode]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://browser/content/places/utils.js :: anonymous :: line 1180" data: no]

also: browser/components/places/utils.js 1145 uri = self._uri("place:folder=TAGS"); 1146 uri = PlacesUtils._uri("place:folder=" + PlacesUtils.tagsFolderId); this is not in https://bugzilla.mozilla.org/attachment.cgi?id=308100, did you checked in a different patch?

(In reply to comment #28) > this is not in https://bugzilla.mozilla.org/attachment.cgi?id=308100, did you > checked in a different patch? No, but the patch was fairly bitrotten so I had fun trying to make sure everything was still caught :/

fixed in bug 423267.

(In reply to comment #31) > fixed in bug 423267. > not all calls to _uri are fixed there... do we need a new bug?

probably best to do a new bug.

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