Lina Butler [:lina] Assignee
	Description • 8 years ago

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

	Lina Butler [:lina] Assignee
	Comment 2 • 8 years ago

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 (╯°□°）╯︵ ┻━┻

	Lina Butler [:lina] Assignee
	Comment 3 • 8 years ago
mozreview-review

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?

	Mark Hammond [:markh] [:mhammond]
	Comment 4 • 8 years ago
mozreview-review

Comment on attachment 8792705 [details]
Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`.

https://reviewboard.mozilla.org/r/79620/#review78972

	Mark Hammond [:markh] [:mhammond]
	Comment 5 • 8 years ago

Comment on attachment 8792705 [details]
Bug 1302288 - Implement `PlacesSyncUtils.bookmarks.fetch`.

stoopid mozreview - this looks fine to me.

	Lina Butler [:lina] Assignee
	Comment 8 • 8 years ago

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?

	Mark Hammond [:markh] [:mhammond]
	Comment 9 • 8 years ago
mozreview-review

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?

	Lina Butler [:lina] Assignee
	Comment 11 • 8 years ago
mozreview-review-reply

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!

	Pulsebot
	Comment 12 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 13 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2ab327c2a30a