Spawned from Bug 1012597. Sync still uses this pattern: newId = PlacesUtils.bookmarks.insertBookmark( record._parent, uri, PlacesUtils.bookmarks.DEFAULT_INDEX, record.title); ... if (newId) { // Livemarks can set the GUID through the API, so there's no need to // do that here. this._log.trace("Setting GUID of new item " + newId + " to " + record.id); this._setGUID(newId, record.id); } We should be creating records with the right GUID up-front.

yes please, this is pretty much important for perf and sanity.

Hm, my initial impression was that this probably would just be a matter of adding the extra GUID argument to the end of the relevant functions (insertBookmark, createFolder, insertSeparator), and removing the this._setGUID call. Unfortunately, doing this results in some test failures stemming from a failed NS_ENSURE_SUCCESS in nsINavBookmarksService.getFolderIdForItem after a FetchItemInfo call (specifically here[0]). I'll keep digging and try to figure out what the issue is, but any pointers would be appreciated, thanks. [0]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#2036-2037

the only things that come to my mind off-hand are: - setting a guid that already exist and not checking for an exception... - maybe the test was expecting to get the "wrong" guid since guid changes made by sync are not notified to anyone? The fetchItemCall can only fail if the id is invalid... and I suppose you get the id from some guidToId method?

Review commit: https://reviewboard.mozilla.org/r/43889/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43889/

Comment on attachment 8737314 [details] MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43889/diff/1-2/

Comment on attachment 8737314 [details] MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43889/diff/2-3/

what was the problem with the tests in the end?

They were using invalid GUIDs. The mocked Utils.makeGUID would return 11 character guids the first 10 times it ran (and times past 100). "fake-guid-0", etc. needed to be changed due to this since they're 11 characters, and the GUIDs in test_bookmark_order.js were 2 or 3 characters and not 12. This caused the failures from nsNavBookmarks that I was asking about in the earlier comment.

Comment on attachment 8737314 [details] MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak https://reviewboard.mozilla.org/r/43889/#review41711 Looks good, thank you!