`PlacesSyncUtils` could grow a `fetch` method that gives us an item with all the right properties set on it. This would remove _isLoadInSidebar, _getDescription, _getTags, another caller of GUIDForId, and idForGUID.

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. Good news: `idForGUID` and `GUIDForId` can go away, too. \o/ Bad news: We have a lot of tests that use them. http://searchfox.org/mozilla-central/search?q=(idForGUID%7CGUIDForId)&case=true&regexp=true&path=services%2Fsync (╯°□°）╯︵ ┻━┻

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. https://reviewboard.mozilla.org/r/79620/#review78256 ::: services/sync/modules/engines/bookmarks.js:95 (Diff revision 1) > }, > > __proto__: CryptoWrapper.prototype, > _logName: "Sync.Record.PlacesItem", > + > + populateFromSyncBookmark(item) { I kind of like having a method that populates itself from a `PlacesSyncUtils` bookmark. Do you think it makes sense to go the other way, too (i.e., a `toSyncBookmark` method that returns an object we can pass to `PlacesSyncUtils.bookmarks.insert` or `PlacesSyncUtils.bookmarks.update`)? That would take care of the "reference to undefined property" warning you pointed out in bug 1295521, and the switches in `create`. Or is that going too far into abstraction kool-aid land?

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. https://reviewboard.mozilla.org/r/79620/#review78972

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. stoopid mozreview - this looks fine to me.

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. MozReview didn't reset the flag, for some reason. Mark, could you take another look, please?

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. https://reviewboard.mozilla.org/r/79620/#review83974 Looks great! ::: toolkit/components/places/PlacesSyncUtils.jsm:1143 (Diff revision 3) > + let folder = null; > + let params = new URLSearchParams(bookmarkItem.url.pathname); > + let tagFolderId = +params.get("folder"); > + if (tagFolderId) { > + try { > + // Throws if the tag query points to a nonexistent folder. This seems more "unexpected" than the other empty catch handlers - do you think it is worth logging here?

Comment on attachment 8792705 [details] Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`. https://reviewboard.mozilla.org/r/79620/#review83974 > This seems more "unexpected" than the other empty catch handlers - do you think it is worth logging here? Good call, done!

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2ab327c2a30a Implement `PlacesSyncUtils.bookmarks.fetch`. r=markh