We need a quick way to access the roots guids, and it should be ideally synchronous. Moreover we also would like to get rid of the moz_bookmarks_roots table. A possible implementation idea could be to enforce special GUIDs on the roots, for example "root________" "menu________" "toolbar_____" "unfiled_____" these would be valid guid, we might use them everywhere, even hardcoded, we could expose them synchronously. The database constraint would ensure we cannot have duplicates and we could likely get rid of moz_bookmarks_roots.

The size of this patch looks scary cause I decided to add 1 db per schema version, for future reference. All tests are passing so far, but I'm having issues (warnings) with a migration test that for whatever reason doesn't finalize statements properly in Sqlite.jsm The test is test_current_from_v24.js and the issue is activated when I use PlacesUtils.bookmarks.eraseEverything() in it. I fear there might be some race condition between Sqlite.jsm and Places shutdown :(

I will file a separate bug for the shutdown problem, there's no reason to block this change on that. so let's start the review process now, I don't plan to add more to this patch.

Comment on attachment 8514401 [details] [diff] [review] tests v1 Review of attachment 8514401 [details] [diff] [review]: ----------------------------------------------------------------- not particularly interesting changes

Comment on attachment 8514554 [details] [diff] [review] patch v1 Add a note that tagsGuid isn't going to grow old. This looks very good otherwise. Make sure to try-run this.

Comment on attachment 8514401 [details] [diff] [review] tests v1 rs=mano. Feel free (you really don't have to!) to fix all those callers in test_async_transactions.js (there's rootGuid = yield in almost every test there). But if you so, wait for bug 982115 to land first.

Richard, from my code checks looks like Sync won't have issues with this change, but I'd be more comfortable if you could evaluate it as well. What we are doing here, is just changing the roots (menu, toolbar, unfiled) guids to new constant values. From what I can tell Sync never accesses the root guids and always compares ids to find roots.

try is green https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f1ea9a53d71

Comment on attachment 8514554 [details] [diff] [review] patch v1 Review of attachment 8514554 [details] [diff] [review]: ----------------------------------------------------------------- This should be fine for Sync, so long as the old method of using parent IDs (not GUIDs) still works. Sync hits the DB to find the GUID for a record's numeric parent ID. If it's one of the numeric IDs for a root, it skips that and uses a special faux-GUID (e.g., "toolbar"). ::: toolkit/components/places/Bookmarks.jsm @@ +107,5 @@ > + menuGuid: "menu________", > + toolbarGuid: "toolbar_____", > + unfiledGuid: "unfiled_____", > + tagsGuid: "tags________", > + Sync uses the following for these: // Special IDs. Note that mobile can attempt to create a record on // dereference; special accessors are provided to prevent recursion within // observers. guids: ["menu", "places", "tags", "toolbar", "unfiled", "mobile"], Is there a reason you're using underscore as padding? I didn't think that was a valid character for the GUIDs we use?

(In reply to Richard Newman [:rnewman] from comment #9) > ::: toolkit/components/places/Bookmarks.jsm > @@ +107,5 @@ > > + menuGuid: "menu________", > > + toolbarGuid: "toolbar_____", > > + unfiledGuid: "unfiled_____", > > + tagsGuid: "tags________", > > + > > Sync uses the following for these: > > // Special IDs. Note that mobile can attempt to create a record on > // dereference; special accessors are provided to prevent recursion within > // observers. > guids: ["menu", "places", "tags", "toolbar", "unfiled", "mobile"], these are not valid guids, afaict it's just made up strings Sync uses as placeholders. Any new style GUID must be 12 chars. > Is there a reason you're using underscore as padding? I didn't think that > was a valid character for the GUIDs we use? "_" is a valid char, based on http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/SQLFunctions.cpp#50 and http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Helpers.cpp#295 Basically, any char that is invalid for an url, like "+" or "/", gets converted to "-" or "_" in the guids implementation, so "_" looks like a good placeholder here.

and I choose _ cause the alternatives don't sound any better (numbers, alphabet or -)

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #10) > these are not valid guids, afaict it's just made up strings Sync uses as > placeholders. > Any new style GUID must be 12 chars. Correct; these only go to the server and other implementations. > Basically, any char that is invalid for an url, like "+" or "/", gets > converted to "-" or "_" in the guids implementation, so "_" looks like a > good placeholder here. Sounds good. Thanks for clarifying.

https://hg.mozilla.org/integration/fx-team/rev/ad5327aa82fd https://hg.mozilla.org/integration/fx-team/rev/bbd631108069

https://hg.mozilla.org/mozilla-central/rev/ad5327aa82fd https://hg.mozilla.org/mozilla-central/rev/bbd631108069