Closed Bug 1274108 Opened 8 years ago Closed 8 years ago

Add a "PlacesSyncUtils" module and refactor the bookmarks engine to use it

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

(Whiteboard: [sync-tracker])

Attachments

(2 files, 5 obsolete files)

Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.
58 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1274108 - Add a `PlacesSyncUtils` module.
58 bytes, text/x-review-board-request
markh
: review+
Details 
Sync currently uses the synchronous methods exposed by `nsINavBookmarksService` and friends. In bug 1258127, we're looking to add a "syncChangeCounter" field to Places, and increment it for every tracked change. We don't want for Sync to track its own changes, though, except when we reconcile an incoming bookmark with existing local changes.

I'm thinking we could add a "PlacesSyncUtils" module that provides queries specifically for Sync. These could use transactions to update multiple tables at once: for example, to set tags or annos. Sync would pass an "info" object (similar to Bookmarks.jsm), and Places would take care of the queries.

There is precedent for this in https://wiki.mozilla.org/index.php?title=Places/AsyncAPIsForSync; in fact, I think that was one of the reasons for creating Bookmarks.jsm! I imagine these would share queries; the only differences are: 1) additional fields, like keywords, tags, and annos, and 2) no change counter increments.

This work can be done independently of bug 1258127. If we do this first, bug 1258127 would change these calls to skip the change counter, without changing the public API. That would make the migration a lot easier than trying to do everything in that bug.

I'm definitely open to other, less invasive suggestions, too. :-) Maybe some kind of "cookie" that we can pass to the Places methods, that would tell it to skip the counter? I'm only suggesting this approach because it's come up before, so maybe now's the right time for it.
Flags: firefox-backlog+
(In reply to Kit Cambridge [:kitcambridge] from comment #0)
> Sync currently uses the synchronous methods exposed by
> `nsINavBookmarksService` and friends. In bug 1258127, we're looking to add a
> "syncChangeCounter" field to Places, and increment it for every tracked
> change. We don't want for Sync to track its own changes, though, except when
> we reconcile an incoming bookmark with existing local changes.
> 
> I'm thinking we could add a "PlacesSyncUtils" module that provides queries
> specifically for Sync. These could use transactions to update multiple
> tables at once: for example, to set tags or annos. Sync would pass an "info"
> object (similar to Bookmarks.jsm), and Places would take care of the queries.

I'm not sure I like the idea, I need more details about what you need to set and what are the current problems with doing that.
Off-hand I'd prefer making propert changes to existing Places APIs and add the remaining stuff as PlacesUtils helpers. Providing multiple write points for the same data would complicate the code a lot and open it to far more bugs.

That said, I need more details about this, cause you are saying you want a way to track changes that cannot fail, but you want to introduce special methods to make it fail.  It sounds like a footgun.
> That said, I need more details about this, cause you are saying you want a way to track changes that cannot fail, but you want to introduce special methods to make it fail.

Sync needs a way to download bookmarks and make changes without incrementing the counter. Otherwise, it would bump the counter for its own changes (for example, bookmarks created on another device, title changed on another device, and so on), and we'd be on an express train to infinite-sync-loop-land. ;-)

In the other bug, you said:

> I'd like to reach a point where Sync communicates with Places through an API,
> rather than executing raw queries. Indeed I'd like Sync to make NO SQL QUERIES
> at all, and collect all the needed APIs in that interface.

Which is what I'm proposing here. You also asked:

> And how would yo prevent consumers from abusing your module?

I think naming the module something like "PlacesSyncUtils" and adding docs clarifying that it's a Sync-specific API is sufficient. If a caller wants to abuse it, they do so at the risk of having their Places changes ignored. It's a similar problem to custom SQL queries: we allow them, with a warning that they can and will break.
OK, I'm fine with the idea, provided all the SQL querying moves from Sync to Places.
Basically, I don't want to expose an addBookmarkDontUpdate API, but I'd be fine with applySyncRecord or such.
this API should be clearly dedicated to sync and of no use for other consumers.

But still, I'd like to understand what are "changes".
> Basically, I don't want to expose an addBookmarkDontUpdate API, but I'd be fine with applySyncRecord or such.

Yep! That's what I'm suggesting.

> But still, I'd like to understand what are "changes".

Basically, everything highlighted here. :-) https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/services/sync/modules/engines/bookmarks.js#474,574-576,603,613-614,669-671,677-679,685,687-689,693-695,700-702,707-709,733-745,759-760,776,781,785,789,854,857,869,873-875,877,882-884,886,890-892,918,1266-1269,1299,1464,1468-1472,1476,1523

Another benefit of having a Sync-specific API is batching some of those calls. For example, I think there should be one call that Sync makes to "insert this bookmark with these properties", instead of "insert this bookmark, then set its tags, then set annos, then...", which is what it does now.
See Also: → 1068054
Priority: -- → P2
Comment on attachment 8759277 [details]
Fix clang build failure.

This doesn't need to land; it just fixes a local build failure because I haven't upgraded clang yet.
Comment on attachment 8759280 [details]
Bug 1274108 - Sketch out a bookmarks API for Sync.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57260/diff/1-2/
Getting test_bookmark_store closer to passing...
https://reviewboard.mozilla.org/r/57260/#review54278

I'm afraid I didn't get very far here in terms of looking at the new implementations, but overall I agree it is the direction we should head - far less places hacks in Sync and back in places where they belong :)

I'll look further next week - in the meantime I suggest you consider getting feedback from Mak on the earlier patches when you think they are ready, just to make sure he is happy with the refactoring you are proposing. If he is happy, it might even make sense to split them into a different bug - refactors like that lingering for a long time run the risk of losing other changes made to those modules between the first version of your patch and it finally landing.

::: services/sync/modules/bookmark_utils.js:111
(Diff revision 2)
> +    return Async.promiseSpinningly(PlacesUtils.promiseItemGuid(mobileRootId));
> +  },
> +};
> +
> +let BookmarkSpecialGuids = {
> +  normalize(guid) {

I think we should come up with a better name here (and possibly for BookmarkSpecialIds too) - I don't have a concrete solution, but it would be ideal if a casual reader didn't need to effectively guess what these functions are doing.

Off the top of my head though, functions like syncIDToPlacesGUID and placesGUIDToSyncID seem clear.

::: services/sync/modules/engines/bookmarks.js:503
(Diff revision 2)
>    },
>  
>    /**
>     * Find all ids of items that have a given value for an annotation
>     */
>    _findAnnoItems: function BStore__findAnnoItems(anno, val) {

It looks like we can kill this too?

::: services/sync/modules/engines/bookmarks.js:510
(Diff revision 2)
> -        PlacesUtils.annotations.removeItemAnnotation(orphan, BookmarkAnnos.PARENT_ANNO);
> -      }
> -    }, this);
> -  },
> -
>    _reparentItem: function _reparentItem(itemId, parentId) {

and this?

::: toolkit/components/places/Bookmarks.jsm:92
(Diff revision 2)
>  const DB_TITLE_LENGTH_MAX = 4096;
>  
>  const MATCH_ANYWHERE_UNMODIFIED = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE_UNMODIFIED;
>  const BEHAVIOR_BOOKMARK = Ci.mozIPlacesAutoComplete.BEHAVIOR_BOOKMARK;
>  
> +var LivemarkSyncUtils = Object.freeze({

I'm a little surprised to see this livemark function in Bookmarks.jsm rather than in one of the LiveMark modules - but more generally, I'm also surprised to see *any* Sync utils in here at all. I assume that is because you need access to some of the internal/private functions - do you think there is scope to expose the ones we need so the sync utils can stand alone?

::: toolkit/components/places/Bookmarks.jsm:2293
(Diff revision 2)
>                                               entry.guid, entry.parentGuid,
>                                               "" ]);
>        }
>      }
>    }
>  });

ouch - this already complex module clocking in at ~2300 lines might be a problem :)

::: toolkit/components/places/PlacesSyncUtils.jsm:14
(Diff revision 2)
> +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +/**
> + * This module exports functions for Sync to use. The update queries are similar

IIUC, you are hoping to land this before the tracker changes - if so, this comment will be wrong.

And at face value, this module existing just to grab already public symbols from other modules doesn't seem to have much value - see above comments - it seems like it would be ideal to move functionality into here.
Comment on attachment 8759278 [details]
Bug 1274108 - Move the "Livemark" class into a separate module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57256/diff/1-2/
Comment on attachment 8759279 [details]
Bug 1274108 - Expose annotation service observers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57258/diff/1-2/
Comment on attachment 8759280 [details]
Bug 1274108 - Sketch out a bookmarks API for Sync.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57260/diff/2-3/
Attachment #8759280 - Flags: review?(markh)
Review commit: https://reviewboard.mozilla.org/r/57696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57696/
Attachment #8759280 - Attachment description: Bug 1274108 - Sketching out a bookmarks API for Sync. → Bug 1274108 - Sketch out a bookmarks API for Sync.
Attachment #8759280 - Flags: review?(markh)
Comment on attachment 8759280 [details]
Bug 1274108 - Sketch out a bookmarks API for Sync.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57260/diff/3-4/
Attachment #8759280 - Flags: review?(markh)
Comment on attachment 8759280 [details]
Bug 1274108 - Sketch out a bookmarks API for Sync.

Hey Mak,

When you have time, I'd love your feedback on the general approach. I sketched out an API for inserting bookmarks and livemarks; will wait for your comments before working on updating, deleting, and reordering.

The interesting additions are in attachment 8759280 [details]; the first few patches just move a lot of code around.

* I moved the "Livemark" constructor and the livemark cache into a new JSM because I didn't see a way to export them from `nsLivemarkService`. Alternatively, I could extend `nsIAnnotationService` with a Sync-specific method, but that seems messy.
* I exposed `nsIAnnotationService.getObservers()` so that `PlacesSyncUtils` could notify annotation observers when it changes descriptions and reparents orphans.
* Finally, I moved the internal implementations and helpers from `Bookmarks.jsm` into a new module, so that `Bookmarks.jsm` and `PlacesSyncUtils.jsm` could share them.

The goal is to (eventually) have this patch land first, then add the change counter columns in bug 1258127.
Attachment #8759280 - Flags: feedback?(mak77)
Attachment #8759847 - Flags: feedback?(mak77)
Attachment #8759278 - Flags: review?(mak77)
Attachment #8759279 - Flags: review?(mak77)
Attachment #8759279 - Flags: feedback?
Attachment #8759279 - Flags: feedback?
Whiteboard: [sync-data-integrity]
I will be away till monday, when I'll be in London. I hope to find some time there to give you some feedback. Eventually we could even meetup there to discuss in person the problems with this Sync rewrite.
(In reply to Marco Bonardo [::mak] - Away 9-13 June from comment #20)
> Eventually we could even meetup there to
> discuss in person the problems with this Sync rewrite.

Talking about this in person sounds great. See you in London!
Attachment #8759278 - Flags: review?(mak77)
Attachment #8759279 - Flags: review?(mak77)
Attachment #8759280 - Flags: feedback?(mak77)
Attachment #8759847 - Flags: feedback?(mak77)
Discussed in person, we should take a slightly more compartmental approach and modify existing APIs instead of trying to move them out, exposing possibly error-prone code paths to add-ons.
Comment on attachment 8759277 [details]
Fix clang build failure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57254/diff/1-2/
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/1-2/
Attachment #8759278 - Attachment is obsolete: true
Attachment #8759279 - Attachment is obsolete: true
Attachment #8759847 - Attachment is obsolete: true
Attachment #8759280 - Attachment is obsolete: true
Priority: P2 → P1
Whiteboard: [sync-data-integrity] → [data-integrity]
https://reviewboard.mozilla.org/r/61076/#review58016

This is looking good - some notes from a quick look below. I'm also not sure on what the status of the sync patch is here - it's relatively old and I suspect isn't quite up-to-date with this patch, so I haven't gone over that.

::: toolkit/components/places/Bookmarks.jsm:236
(Diff revision 1)
>      // The info object is first validated here to ensure it's consistent, then
>      // it's compared to the existing item to remove any properties that don't
>      // need to be updated.
>      let updateInfo = validateBookmarkObject(info,
>        { guid: { required: true }
> -      , index: { requiredIf: b => b.hasOwnProperty("parentGuid")
> +      , index: { validIf: b => b.index >= 0 || b.index == this.DEFAULT_INDEX }

this change (and the similar one below) look like ones we should chat more with Mak about (ie, specifically call out this requirement in bug comments rather than waiting for patch feedback)

::: toolkit/components/places/Bookmarks.jsm:1379
(Diff revision 1)
> +
> +function validateBookmarkObject(input, behavior) {
> +  return validateBookmarkProperties(VALIDATORS, input, behavior);
> +}
> +
> +function validateSyncBookmarkObject(input, behavior) {

It seems a little wrong to have a validator here for objects that this module never actually sees.

Do you think it might be reasonable to have a single public function validateBookmarkObject(input, behavior, validator = null) or similar and have the SYNC_VALIDATORS in the new module? It may require a small amount of duplication of, eg, VALIDATORS.url, but that might be OK - 

The other alternative would probably be to just copy/paste the validator function itself and require no public changes for validation in this module.

::: toolkit/components/places/PlacesSyncUtils.jsm:22
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");
> +
> +/**
> + * This module exports functions for Sync to use. The update queries are similar
> + * to those in `Bookmarks.jsm` and `nsINavBookmarksService`, but do not

Is this comment true in this version of the patch?

::: toolkit/components/places/PlacesSyncUtils.jsm:28
(Diff revision 1)
> + * increment the change counter. Otherwise, Sync would track its own changes as
> + * it applied incoming records, causing an infinite sync loop.
> + */
> +var PlacesSyncUtils = {};
> +
> +// TODO(kitcambridge): Move BookmarkAnnos from bookmark_utils.js here.

For our information, do you intend doing these TODOs in a later version of this patch?

::: toolkit/components/places/PlacesSyncUtils.jsm:98
(Diff revision 1)
> +
> +  /**
> +   * Inserts a synced bookmark into the tree. Only Sync should call this
> +   * method; other callers should use `Bookmarks.insert`.
> +   *
> +   * Unlike `Bookmarks.insert`, this call does not increment the sync change

ditto here re changeCounter comment

::: toolkit/components/places/PlacesSyncUtils.jsm:170
(Diff revision 1)
> +    return item;
> +  }));
> +}
> +
> +function* ensureParent(info, func) {
> +  // Ensure the parent exists; default to "unfiled" if it doesn't.

I think we should flesh this comment out a little to indicate that (a) it will actually replace item.parentGuid with unfiled and (b) that if it does that, it will write the original parent to an annotation, which will (hopefully) end up being used to correct this change and (c) it will perform that correction for any items previously annotated as having this as the "real" parent.

You should also indicate what "func" is for in some detail - it seems a little odd at first glance, but probably does make sense once you understand what is going on (and maybe a better name, such as "withSyncParentSemantics" or something might also make the intent clearer)

And add the comment before the function - probably no real need to use docstring style comments though for an internal function)

::: toolkit/components/places/PlacesSyncUtils.jsm:215
(Diff revision 1)
> +
> +  return item;
> +}
> +
> +function* insertSyncBookmark(insertInfo) {
> +  let item = yield ensureParent(insertInfo, function* (insertInfo) {

can you just return here? The 2 close |item| vars looks odd.

::: toolkit/components/places/PlacesSyncUtils.jsm:329
(Diff revision 1)
> +    // record; we must remove and reinsert.
> +    (oldKind == BookmarkSyncUtils.KIND_LIVEMARK &&
> +                (updateInfo.hasOwnProperty("site") ||
> +                updateInfo.hasOwnProperty("feed"))) ||
> +    // Similarly, if the items aren't the same kind, we need to reinsert.
> +    (updateInfo.hasOwnProperty("kind") && updateInfo.kind != oldKind);

I wonder if this can happen in practice - it almost sounds like a logic error in Sync. I guess I'm wondering if it might be worth logging here?

::: toolkit/components/places/PlacesSyncUtils.jsm:337
(Diff revision 1)
> +    let newItem = validateNewBookmark(updateInfo);
> +    yield PlacesUtils.bookmarks.remove(oldItem.guid);
> +    return yield insertSyncBookmark(newItem);
> +  }
> +
> +  let item = yield ensureParent(updateInfo, function* (updateInfo) {

similarly, returning here seems to make sense and make it clearer
Thanks for looking at this, Mark! I addressed some of your suggestions in this round, but focused on getting the tests passing. Good news: all the test_bookmark_*.js tests pass now.

I'll work on cleaning this up and resolving the remaining TODOs next.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/1-2/
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/2-3/
Attachment #8759277 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/61076/#review58016

> It seems a little wrong to have a validator here for objects that this module never actually sees.
> 
> Do you think it might be reasonable to have a single public function validateBookmarkObject(input, behavior, validator = null) or similar and have the SYNC_VALIDATORS in the new module? It may require a small amount of duplication of, eg, VALIDATORS.url, but that might be OK - 
> 
> The other alternative would probably be to just copy/paste the validator function itself and require no public changes for validation in this module.

Agreed, I think it makes sense to expose it.

> For our information, do you intend doing these TODOs in a later version of this patch?

I do, yes.

> I wonder if this can happen in practice - it almost sounds like a logic error in Sync. I guess I'm wondering if it might be worth logging here?

It's definitely worth logging. I see this came up in bug 632287, and the test covers replacing a folder with a livemark. (In that case, it's worse because both are stored as "folders"). That's why the Sync record "kind" leaks in to PlacesSyncUtils: the `nsINavBookmarkService` type doesn't give us enough information about the item.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/2-3/
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/3-4/
Looks like services/sync/tests/unit/test_places_guid_downgrade.js and test_syncscheduler.js still fail.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/3-4/
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/4-5/
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Whew, all the tests pass, and I think I've accurately preserved the current behavior. Ready for a round of feedback. There are a couple of TODOs around logging that I haven't addressed yet.

Mak, this follows the approach we discussed in London, using a mix of sync and async APIs. I expect us to move more code from Sync into Places: in particular, I'm not happy with exposing `clear`, and there's complexity in `ensureGuidForId` that I'm not sure we need anymore. But quite a few of the Sync tests exercise this behavior, and I wanted to avoid changing existing semantics in this round.

I also exposed the bookmark validators, and moved the validator function into `PlacesUtils`, because the PlacesSyncUtils validators are a superset of Bookmarks.jsm's.

I'm sorry for the size of the patch. Please let me know if you'd like me to break it up further to make it easier to review. Thanks for taking the time to discuss this with me in London, and I'm looking forward to your feedback!
Attachment #8765998 - Flags: feedback?(markh)
Attachment #8765998 - Flags: feedback?(mak77)
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Lots of moving stuff around. :-)

I'm kind of unhappy about the Sync kind leaking in to Places, but there are several tests for an item changing type, prompted by bug 632287. I'm also not happy about the "SpecialGUIDToPlacesGUID" map. It's a shame we're doing all this converting back and forth, so maybe it makes sense to just go all the way and have PlacesSyncUtils convert from special sync IDs to GUIDs.

There's also a fair bit of complexity around the mobile root. (I didn't touch `_ensureMobileQuery`, for example). Bug 647605 should clean that up, eventually, so I opted to punt on that for now.
Attachment #8759858 - Flags: feedback?(markh)
https://reviewboard.mozilla.org/r/61076/#review59194

::: toolkit/components/places/PlacesSyncUtils.jsm:66
(Diff revision 4)
> +      if (!existingChildren.has(childGuid)) {
> +        delta++;
> +        continue;
> +      }
> +      let newIndex = index - delta;
> +      yield PlacesUtils.bookmarks.update({

In a follow-up, it might make sense to copy the relevant query from `PlacesUtils.bookmarks.update` here, so that we're not doing extra work re-fetching the parent and validating properties again.

::: toolkit/components/places/PlacesSyncUtils.jsm:107
(Diff revision 4)
> +  }),
> +
> +  /**
> +   * Removes a folder's children. This is a temporary method that can be
> +   * replaced by `eraseEverything` once Places supports the Sync-specific
> +   * mobile root.

I think we can break this down into several parts:

* Move the mobile root creation into Places. We can do that at startup, and move existing mobile bookmarks under the new root during migration.
* Change Sync's reconciliation logic to use the new root, instead of creating its own. We'd need to make sure older clients that still create their own roots handle this gracefully. This is a case where Sync's "special GUIDs" help us.
* Move the mobile query creation into `PlacesUIUtils`. Currently, it's created lazily. We can either make it non-lazy, following Richard's suggestion, or add an observer that creates it as soon as we add a bookmark to the mobile root.

::: toolkit/components/places/PlacesSyncUtils.jsm:150
(Diff revision 4)
> +
> +    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: changeGuid", Task.async(function* (db) {
> +      return db.executeTransaction(function* () {
> +        let results = yield db.executeCached(`SELECT id FROM moz_bookmarks
> +          WHERE guid = :oldGuid LIMIT 1`, { oldGuid });
> +        if (!results.length) {

Add logging if we try to change the GUID for a nonexistent item.

::: toolkit/components/places/PlacesSyncUtils.jsm:317
(Diff revision 4)
> +    return item;
> +  }));
> +}
> +
> +function* annotateOrphan(item, requestedParentGuid) {
> +  let itemId = yield PlacesUtils.promiseItemId(item.guid);

This is an extra DB call. Might make sense to factor out if we already have the ID, even though it'll become unnecessary if we add an async annotations API that's GUID-aware.

::: toolkit/components/places/PlacesSyncUtils.jsm:397
(Diff revision 4)
> +  let parentId = yield PlacesUtils.promiseItemId(insertInfo.parentGuid);
> +  let parentIsLivemark = PlacesUtils.annotations.itemHasAnnotation(parentId,
> +    PlacesUtils.LMANNO_FEEDURI);
> +  if (parentIsLivemark) {
> +    // A livemark can't be a descendant of another livemark.
> +    return null;

Add logging here, or throw and have Sync log. (Sync doesn't currently throw for this case).

::: toolkit/components/places/PlacesSyncUtils.jsm:502
(Diff revision 4)
> +  let shouldReinsert =
> +    // If we're changing a livemark's site or feed URL, we can't update the
> +    // record; we must remove and reinsert.
> +    (oldKind == BookmarkSyncUtils.KIND_LIVEMARK &&
> +                (updateInfo.hasOwnProperty("site") ||
> +                updateInfo.hasOwnProperty("feed"))) ||

I don't think changing a livemark's URL is possible in the UI...but the same item also shouldn't change its kind, and yet here we are. :-)

::: toolkit/components/places/PlacesSyncUtils.jsm:520
(Diff revision 4)
> +    if (requestedParentGuid != oldItem.parentGuid) {
> +      let oldId = yield PlacesUtils.promiseItemId(oldItem.guid);
> +      if (isRootId(oldId)) {
> +        // Make sure we aren't trying to move a Places root. We check the ID
> +        // instead of the GUID because it's possible for reconciliation to
> +        // erroneously change a root's GUID, then try to reparent it.

Stepping back and thinking about this some more, it doesn't make sense to enforce this here. Changing a root's GUID during reconciliation is a bug, and we shouldn't be working around it.

Instead, let's do something like this:

* In `changeGuid`, check if we're changing a root. (I think we should check the ID, not the GUID, because it's possible for Sync to have changed it before).
* Throw if Sync tries to change a root GUID to a non-root GUID, a different root GUID (for example, changing `menu________` to `toolbar_____`), or a non-root to a root (the opposite of "test_misreconciled_root").

The case that worries me is this:

* A user has a profile where Sync changed a root's GUID. To use the example from the test, let's say Sync renamed `toolbar_____` to `zzzzzzzzzzzz`.
* User then adds a bookmark to the toolbar. Sync marks the new bookmark and the parent as changed.

::: toolkit/components/places/PlacesUtils.jsm:416
(Diff revision 4)
> +   *
> +   * @return a validated and normalized item.
> +   * @throws if the object contains invalid data.
> +   * @note any unknown properties are pass-through.
> +   */
> +  validateItemProperties(validators, input, behavior={}) {

JFTR, I named this `validateItemProperties` because it seems like we can use this validation function for other APIs, not just bookmarks. (If we ever add `History.jsm`, for example).
https://reviewboard.mozilla.org/r/61076/#review59210

::: toolkit/components/places/Bookmarks.jsm:259
(Diff revision 4)
>        }
>  
>        let time = (updateInfo && updateInfo.dateAdded) || new Date();
>        updateInfo = validateBookmarkObject(updateInfo,
>          { url: { validIf: () => item.type == this.TYPE_BOOKMARK }
> +        , index: { defaultValue: this.DEFAULT_INDEX

I changed this because I'm not sure it makes sense to require the index if we're not reparenting. But I don't think we need `defaultValue` here, that'll do the wrong thing.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/4-5/
Attachment #8765998 - Flags: feedback?(markh)
Attachment #8765998 - Flags: feedback?(mak77)
Attachment #8759858 - Flags: feedback?(markh)
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/5-6/
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/5-6/
Attachment #8765998 - Attachment description: Bug 1274108 - Sketch out a bookmarks API for Sync. → Bug 1274108 - Add a `PlacesSyncUtils` module for Sync.
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/6-7/
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Made the changes Mark and I discussed over Vidyo yesterday and added logging.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b800de610085
Attachment #8765998 - Flags: feedback?(markh)
Attachment #8765998 - Flags: feedback?(mak77)
Attachment #8759858 - Flags: feedback?(markh)
https://reviewboard.mozilla.org/r/61076/#review59628

I think this is in great shape - but I suspect we will want some tests for the new module (but OTOH we could possibly argue that Sync has the tests)

::: toolkit/components/places/PlacesSyncUtils.jsm:150
(Diff revision 6)
> +
> +  /**
> +   * Changes the GUID of an existing item.
> +   *
> +   * @return {Promise} resolved once the GUID has been changed.
> +   * @resolves to the new GUID, or |null| if the item doesn't exist.

should this maybe resolve without a value and reject instead of returning null? (ie, if it resolves, we know newGuid is the new guid, and failing to change the GUID sounds like an error at the API level)

::: toolkit/components/places/PlacesSyncUtils.jsm:245
(Diff revision 6)
> +  let log = Log.repository.getLogger("BookmarkSyncUtils");
> +  // Use the bookmark engine's log level so that `BookmarkSyncUtils` operations
> +  // show up in the Sync logs.
> +  let level = Preferences.get("services.sync.log.logger.engine.bookmarks");
> +  if (level) {
> +    let appender = new Log.DumpAppender();

I don't think we will want a new DumpAppender here - I think Sync is going to want to add this log to its logs (so they end up in log files etc) and it will also use a DumpAppender - the end result being that messages from here are likely to end up being dump'd twice.

::: toolkit/components/places/PlacesSyncUtils.jsm:474
(Diff revision 6)
> +
> +  try {
> +    item.tags = tagItem(item, insertInfo.tags);
> +  } catch (ex) {
> +    BookmarkSyncLog.warn(`insertBookmarkMetadata: Error tagging item ${
> +      item.guid}: ${ex}`);

you should pass exceptions as the 2nd arg to log functions so you get a stack trace.

::: toolkit/components/places/PlacesSyncUtils.jsm:609
(Diff revision 6)
> +  updateInfo = yield updateTagQueryFolder(updateInfo);
> +
> +  let newItem = yield PlacesUtils.bookmarks.update(updateInfo);
> +  let itemId = yield PlacesUtils.promiseItemId(newItem.guid);
> +
> +  newItem = yield* updateBookmarkMetadata(itemId, oldItem, newItem, updateInfo);

I don't understand why "yield*" is necessary in some places here but not others - if they are, do you mind educating me? :)
https://reviewboard.mozilla.org/r/57696/#review59670

I think this is looking good too - I'm a little worried me are changing semantics in some subtle ways, but I need to spend more time on this before I can make an informed comment re that.

::: services/sync/tests/unit/test_bookmark_engine.js
(Diff revision 7)
> -
> -  run_next_test();
> -});
> -
> -add_test(function test_bookmark_tag_but_no_uri() {
> -  _("Ensure that a bookmark record with tags, but no URI, doesn't throw an exception.");

this test seems worthile to keep

::: services/sync/tests/unit/test_bookmark_livemarks.js
(Diff revision 7)
>  
>  add_test(function test_livemark_invalid() {
>    _("Livemarks considered invalid by nsLivemarkService are skipped.");
>  
> -  _("Parent is 0, which is invalid. Will be set to unfiled.");
> -  let noParentRec = makeLivemark(record631361.payload, true);

ditto here
https://reviewboard.mozilla.org/r/61076/#review59666

::: toolkit/components/places/PlacesSyncUtils.jsm:716
(Diff revision 6)
> +                             , BookmarkSyncUtils.KIND_QUERY
> +                             , BookmarkSyncUtils.KIND_FOLDER
> +                             , BookmarkSyncUtils.KIND_LIVEMARK ].includes(b.kind) }
> +    , query: { requiredIf: b => b.kind == BookmarkSyncUtils.KIND_QUERY }
> +    , folder: { validIf: b => b.kind == BookmarkSyncUtils.KIND_QUERY }
> +    , tags: { validIf: b => [ BookmarkSyncUtils.KIND_BOOKMARK

I think we have a semantic change here (although I don't know how important it is). IIUC, an incoming "folder" item with tags will still be updated, but it looks to me like this new structure will just cause it to fail without applying anything?
Blocks: 1285408
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/6-7/
Attachment #8765998 - Flags: feedback?(markh)
Attachment #8765998 - Flags: feedback?(mak77)
Attachment #8759858 - Flags: feedback?(markh)
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/7-8/
I'll clean this up a bit more tonight and tomorrow before re-requesting feedback.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/7-8/
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/8-9/
https://reviewboard.mozilla.org/r/61076/#review59628

> I don't think we will want a new DumpAppender here - I think Sync is going to want to add this log to its logs (so they end up in log files etc) and it will also use a DumpAppender - the end result being that messages from here are likely to end up being dump'd twice.

Oh, nice, I didn't realize. I see there's a LogManager object that we create in policies.js; it looks like all I need to do is add "BookmarkSyncUtils" to the `logs` array.

> I don't understand why "yield*" is necessary in some places here but not others - if they are, do you mind educating me? :)

We talked a bit about this on IRC, but, if I yield to a generator that's not wrapped in `Task.async`, and that generator yields, it'll yield the generator (`[object Generator]`) instead of a promise. I can change it so that `updateBookmarkMetadata` and friends are wrapped in `Task.async`; that'll do the trick, too.
https://reviewboard.mozilla.org/r/61076/#review59666

> I think we have a semantic change here (although I don't know how important it is). IIUC, an incoming "folder" item with tags will still be updated, but it looks to me like this new structure will just cause it to fail without applying anything?

Ideally, we'd be OK because `BookmarkFolder` inherits from `PlacesItem`, which I wouldn't expect to have tags. We also check the kind in the bookmark engine before applying. I've since learned not to say "ideally", "should" or "expect" when it comes to Sync, though, so I can make it more generic if you think it's valuable.
https://reviewboard.mozilla.org/r/57696/#review59670

> this test seems worthile to keep

Agreed! Added it back.

> ditto here

I removed this part of the test because we don't use `_parent` anymore, and the "missing GUID" case is already handled. "Invalid GUID" will throw. I'll add a test for the previous behavior and verify it holds.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/8-9/
Attachment #8765998 - Attachment description: Bug 1274108 - Add a `PlacesSyncUtils` module for Sync. → Bug 1274108 - Add a `PlacesSyncUtils` module. f?mak
Attachment #8759858 - Attachment description: Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`. → Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`. f?mak
Attachment #8765998 - Flags: review?(markh)
Attachment #8759858 - Flags: review?(markh)
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/9-10/
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

OK, I think this is ready! Mark and Mak, please take a look when it's convenient.

This is the first part of the bookmarks tracker refactor, which moves all mutation methods used by Sync into a separate "PlacesSyncUtils" module. I started on some tests, though they'll need to be expanded to cover all the methods and branches exposed by the new module. The Sync bookmarks engine tests also help provide some test coverage.

I'll be out for the next couple of weeks, but I look forward to reading your feedback once I get back from holiday!
Attachment #8765998 - Flags: feedback?(mak77)
https://reviewboard.mozilla.org/r/61076/#review62500

::: toolkit/components/places/Bookmarks.jsm:250
(Diff revision 9)
>            updateInfo.dateAdded.getTime() != item.dateAdded.getTime())
>          throw new Error("The bookmark dateAdded cannot be changed");
> +      if (updateInfo.hasOwnProperty("parentGuid") &&
> +          updateInfo.parentGuid != item.parentGuid &&
> +          !updateInfo.hasOwnProperty("index"))
> +        throw new Error("An index is required for moving to another folder");

off-hand I'd say the opposite.

The reason to force an index when a parent is defined, is to avoid coding mistakes where one could forget about the index and then the bookmark would be appended. We are enforcing the caller to specify the behavior (append or insert).

The worst case of mistake is moving a bookmark into the same parent it is already, but not specify an index. In such a case you are basically taking the bookmark from its position and appending it at the bottom of its parent...
I honestly don't see a reason one would like to do that, and if he really should, would make sense to force him being explicit.

so, if we really need to relax this, at a maximum I would make the index optional only when moving to a different container.

::: toolkit/components/places/Bookmarks.jsm:1294
(Diff revision 9)
> -/**
>   * List of validators, one per each known property.
>   * Validators must throw if the property value is invalid and return a fixed up
>   * version of the value, if needed.
>   */
> -const VALIDATORS = Object.freeze({
> +XPCOMUtils.defineLazyGetter(this, "VALIDATORS", () => {

would moving the validators to PlacesUtils simplify things?
They seem to be generic enough we could share them.

::: toolkit/components/places/PlacesSyncUtils.jsm:52
(Diff revision 9)
> +
> +  /**
> +   * Fetches a folder's children, ordered by their position within the folder.
> +   */
> +  fetchChildGuids(parentGuid) {
> +    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: fetchChildGuids",

I'd like to clarify the difference between PlacesUtils.promiseDBConnection and withConnectionWrapper.

promiseDBConnection returns a read-only connection, that in general is faster, but clearly can only be used with SELECT.

withConnectionWrapper return a read-write connection, and enforces the write to happen BEFORE shutdown (spinning the events loop).

In general you may want to use the former for speed. There is one downside though, the 2 connections are concurrent, so promiseDBConnection reads from a snapshot of the db that does not include the current transaction. That means if you do a write and then immediately read from promiseDBConnection, you may not see the change.

what to use depends on the situation, when you need to make changes and read in a serialized way, withConnectionWrapper is better. Otherwise when you just need to read a bunch of rows, promiseDBConnection is better.

::: toolkit/components/places/PlacesSyncUtils.jsm:53
(Diff revision 9)
> +  /**
> +   * Fetches a folder's children, ordered by their position within the folder.
> +   */
> +  fetchChildGuids(parentGuid) {
> +    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: fetchChildGuids",
> +      db => db.executeTransaction(function* () {

I'd open the transaction later when yo uactually need to write. large transactions cause performance problems in general.

Note: we could extend Bookmarks.fetch, so that if a parentGuid is specified, but no index, it would return all the children in position order (fetchBookmarksByParent already has an implementation)

::: toolkit/components/places/PlacesSyncUtils.jsm:68
(Diff revision 9)
> +        return Promise.all(rows.map(row => {
> +          let guid = row.getResultByName("guid");
> +          if (guid) {
> +            return guid;
> +          }
> +          // Give the child a GUID if it doesn't have one.

It is my assumption this is just an emergency case, in general it should never happen.

Since you are moving a bunch of code, it would be nice to have more inline documentation

::: toolkit/components/places/PlacesSyncUtils.jsm:87
(Diff revision 9)
> +   * incoming records.
> +   *
> +   */
> +  order: Task.async(function* (parentGuid, childGuids) {
> +    let parentId = yield PlacesUtils.promiseItemId(parentGuid);
> +    let existingChildren = yield getChildrenToOrder(parentGuid);

You could probably factor out the querying for  fetchChildGuids and getChildrenToOrder and just do something on the results of the querying.

::: toolkit/components/places/PlacesSyncUtils.jsm:102
(Diff revision 9)
> +      }
> +      let newIndex = index - delta;
> +      yield PlacesUtils.bookmarks.update({
> +        guid: childGuid,
> +        index: newIndex,
> +      });

There is an important difference between this and reorder, and it's efficiency.

This runs an update call for each item, that is a bunch of queries.
The main reason to introduce reorder was that people were using update (setItemIndex previously) to reorder children, and that was a perf problem.

maybe we could add code to pre-calculate position for all the guids and then just pass that to reorder? We seem to have all the information (or we can fetch it from the query that gets children)

::: toolkit/components/places/PlacesSyncUtils.jsm:114
(Diff revision 9)
> +  remove: Task.async(function* (guid) {
> +    VALIDATORS.guid(guid);
> +
> +    let db = yield PlacesUtils.promiseDBConnection();
> +    let results = yield db.executeCached(`SELECT type FROM moz_bookmarks
> +      WHERE guid = :guid`, { guid });

Why not bookmarks.fetch?

::: toolkit/components/places/PlacesSyncUtils.jsm:137
(Diff revision 9)
> +
> +      default:
> +        BookmarkSyncLog.error(`remove: Unknown item type ${type} for ${guid}`);
> +        return false;
> +    }
> +    yield PlacesUtils.bookmarks.remove(guid);

note that remove returns the removed object and would throw if it's not found, so to me looks like this could avoid fetching before the removal just by inverting the code?

::: toolkit/components/places/PlacesSyncUtils.jsm:148
(Diff revision 9)
> +   * replaced by `eraseEverything` once Places supports the Sync-specific
> +   * mobile root.
> +   */
> +  clear: Task.async(function* (folderGuid) {
> +    let folderId = yield PlacesUtils.promiseItemId(folderGuid);
> +    PlacesUtils.bookmarks.removeFolderChildren(folderId);

So we could eraseEverything and THEN removeFolderChildren for the mobile root?

::: toolkit/components/places/PlacesSyncUtils.jsm:167
(Diff revision 9)
> +        }
> +        // Use the existing GUID if it exists.
> +        let guid = results[0].getResultByName("guid");
> +        if (guid) {
> +          return guid;
> +        }

Most of this method could be replaced by promiseItemGuid

::: toolkit/components/places/PlacesSyncUtils.jsm:181
(Diff revision 9)
> +   *
> +   * @return {Promise} resolved once the GUID has been changed.
> +   * @resolves to the new GUID.
> +   * @rejects if the old GUID does not exist.
> +   */
> +  changeGuid: Task.async(function* (oldGuid, newGuid) {

I find confusing that there's 2 identically named function "changeGuid", I'd suggest changing one of the 2 to avoid confusion...

::: toolkit/components/places/PlacesSyncUtils.jsm:190
(Diff revision 9)
> +      return db.executeTransaction(function* () {
> +        let results = yield db.executeCached(`SELECT id FROM moz_bookmarks
> +          WHERE guid = :oldGuid LIMIT 1`, { oldGuid });
> +        if (!results.length) {
> +          throw new Error(`Item ${oldGuid} does not exist`);
> +        }

and most of this could be replaced by promiseItemId

::: toolkit/components/places/PlacesSyncUtils.jsm:319
(Diff revision 9)
> +  return PlacesUtils.validateItemProperties(VALIDATORS, input, behavior);
> +}
> +
> +// Tag queries use a `place:` URL that refers to the tag folder ID. When we
> +// apply a synced tag query from a remote client, we need to update the URL to
> +// point to the local tag folder.

we should probably file a bug to change any existing place:folder=X&type=RESULTS_AS_TAG_CONTENTS to a place:tags=name and remove RESULTS_AS_TAG_CONTENTS...

::: toolkit/components/places/PlacesSyncUtils.jsm:355
(Diff revision 9)
> +                 (SELECT count(*) FROM moz_bookmarks WHERE parent = :parent),
> +                 :title, :dateAdded, :lastModified, :guid)
> +      `, { type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +           parent: PlacesUtils.bookmarks.tagsFolder, title: item.folder,
> +           dateAdded: now, lastModified: now, guid });
> +      return PlacesUtils.promiseItemId(guid);

woo, this completely breaks the internal tagging service cache since it doesn't even notify the bookmark change...

::: toolkit/components/places/PlacesSyncUtils.jsm:765
(Diff revision 9)
> +  let annos = PlacesUtils.annotations;
> +  return annos.getItemsWithAnnotation(anno, {}).filter(id =>
> +    annos.getItemAnnotation(id, anno) == val);
> +}
> +
> +function tagItem(item, tags) {

let's make this fakeAsync, so it's more future-proof
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

I concentrated mostly on major issues and direct db changes
Attachment #8765998 - Flags: feedback?(mak77)
Whiteboard: [data-integrity] → [tracker]
Whiteboard: [tracker] → [sync-tracker]
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Clearing review until I address Mak's comments.
Attachment #8759858 - Flags: review?(markh)
Attachment #8765998 - Flags: review?(markh)
https://reviewboard.mozilla.org/r/61076/#review62500

> off-hand I'd say the opposite.
> 
> The reason to force an index when a parent is defined, is to avoid coding mistakes where one could forget about the index and then the bookmark would be appended. We are enforcing the caller to specify the behavior (append or insert).
> 
> The worst case of mistake is moving a bookmark into the same parent it is already, but not specify an index. In such a case you are basically taking the bookmark from its position and appending it at the bottom of its parent...
> I honestly don't see a reason one would like to do that, and if he really should, would make sense to force him being explicit.
> 
> so, if we really need to relax this, at a maximum I would make the index optional only when moving to a different container.

Thinking about this some more, you're totally right. Thanks for explaining the intent for enforcing this.

> would moving the validators to PlacesUtils simplify things?
> They seem to be generic enough we could share them.

Yes, it would. I like that better than the lazy getters.

> I'd like to clarify the difference between PlacesUtils.promiseDBConnection and withConnectionWrapper.
> 
> promiseDBConnection returns a read-only connection, that in general is faster, but clearly can only be used with SELECT.
> 
> withConnectionWrapper return a read-write connection, and enforces the write to happen BEFORE shutdown (spinning the events loop).
> 
> In general you may want to use the former for speed. There is one downside though, the 2 connections are concurrent, so promiseDBConnection reads from a snapshot of the db that does not include the current transaction. That means if you do a write and then immediately read from promiseDBConnection, you may not see the change.
> 
> what to use depends on the situation, when you need to make changes and read in a serialized way, withConnectionWrapper is better. Otherwise when you just need to read a bunch of rows, promiseDBConnection is better.

That's a very helpful explanation, thanks! In some cases, I use `withConnectionWrapper` where I read-then-write...but writing isn't expected to be the common case here, so I changed it as you suggested.

> I'd open the transaction later when yo uactually need to write. large transactions cause performance problems in general.
> 
> Note: we could extend Bookmarks.fetch, so that if a parentGuid is specified, but no index, it would return all the children in position order (fetchBookmarksByParent already has an implementation)

Extending `fetch` seems like a good idea. I didn't do that in the latest version of this patch, but maybe worthwhile for a follow-up.

> There is an important difference between this and reorder, and it's efficiency.
> 
> This runs an update call for each item, that is a bunch of queries.
> The main reason to introduce reorder was that people were using update (setItemIndex previously) to reorder children, and that was a perf problem.
> 
> maybe we could add code to pre-calculate position for all the guids and then just pass that to reorder? We seem to have all the information (or we can fetch it from the query that gets children)

Indeed, we do have all the info we need to sort all the children and pass an array to `reorder`. Oddly, I found `reorder` leaves holes in the tree. I pasted a dump into `services/sync/tests/unit/test_bookmark_order.js` to illustrate what's going on.

I think we should fix `reorder`, but I don't understand its queries well enough yet to know where it's gone wrong, and I'm still recovering from traveling. :-) So I copy-pasted a modified version of its query into this method.

> So we could eraseEverything and THEN removeFolderChildren for the mobile root?

We could. I'd like to keep the existing mobile root code in the bookmarks engine and avoid having it leak into PlacesSyncUtils. Bug 647605 should clean up the complexity around it, and then Sync could just use `eraseEverything`.

> woo, this completely breaks the internal tagging service cache since it doesn't even notify the bookmark change...

Fixed to use public methods that fire observer notifications properly. Not sure what I was thinking before.
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61076/diff/9-10/
Attachment #8765998 - Attachment description: Bug 1274108 - Add a `PlacesSyncUtils` module. f?mak → Bug 1274108 - Add a `PlacesSyncUtils` module.
Attachment #8759858 - Attachment description: Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`. f?mak → Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.
Attachment #8765998 - Flags: review?(markh)
Attachment #8759858 - Flags: review?(markh)
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57696/diff/10-11/
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

https://reviewboard.mozilla.org/r/61076/#review67070

::: services/sync/tests/unit/test_bookmark_order.js:126
(Diff revision 10)
>  
>    _("Moving 20 behind 42 in f40 -> update 50");
>    apply(bookmark(id20, id40));
>    check([id10, [id31], [id41, id42, id20]]);
>  
> +  /* Using `PlacesUtils.bookmarks.reorder` in `PlacesSyncUtils.bookmarks.order`

let's get a bug on file for this and reference it in the comment.

::: toolkit/components/places/PlacesSyncUtils.jsm:61
(Diff revision 10)
> +    for (let child of children) {
> +      let guid = child.guid;
> +      if (!guid) {
> +        // Give the child a GUID if it doesn't have one. This shouldn't happen,
> +        // but the old bookmarks engine code does this, so we'll match its
> +        // behavior until we're sure this can be removed.

I don't quite understand this block - guidsToSet doesn't seem used. I'd have thought ensureGuidForId() below would be used?

::: toolkit/components/places/PlacesSyncUtils.jsm:116
(Diff revision 10)
> +            updateChildIndex(children, child, newIndex);
> +          }
> +          children.sort((a, b) => a.index - b.index);
> +
> +          /*
> +          let orderedChildrenGuids = children.map(({ guid }) => guid);

add that bug # here too.

::: toolkit/components/places/PlacesSyncUtils.jsm:176
(Diff revision 10)
> +    // Use the existing GUID if it exists.
> +    let guid = yield PlacesUtils.promiseItemGuid(itemId);
> +    if (guid) {
> +      return guid;
> +    }
> +    // Give the item a GUID if it doesn't have one.

I think you should copy the comment above re "this shouldn't happen", and throw in a log.warn for good measure.

::: toolkit/components/places/PlacesSyncUtils.jsm:271
(Diff revision 10)
> +  return Log.repository.getLogger("BookmarkSyncUtils");
> +});
> +
> +function validateSyncBookmarkObject(input, behavior) {
> +  return PlacesUtils.validateItemProperties(
> +    PlacesUtils.SYNC_BOOKMARK_VALIDATORS, input, behavior);

We earlier discussed the possibility of putting the Sync validators in this module.

::: toolkit/components/places/PlacesUtils.jsm:258
(Diff revision 10)
> +    return v;
> +  }
> +});
> +
> +// Sync bookmark records can contain additional properties.
> +const SYNC_BOOKMARK_VALIDATORS = Object.freeze(Object.assign({

I've forgotten the details when we discussed this, but it looks to me like these validators could go into the new module, with the only cost being we need to copy/paste the small simpleValidateFunc function?

(I'm not too bothered by this, but I do think it makes sense to keep the sync validators in the sync module)
Attachment #8765998 - Flags: review?(markh) → review+
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

https://reviewboard.mozilla.org/r/61076/#review67070

> let's get a bug on file for this and reference it in the comment.

Filed bug 1293365.

> I've forgotten the details when we discussed this, but it looks to me like these validators could go into the new module, with the only cost being we need to copy/paste the small simpleValidateFunc function?
> 
> (I'm not too bothered by this, but I do think it makes sense to keep the sync validators in the sync module)

I went back and forth on this a bit. I think Mak is right in that they're generic enough to where they can live in `PlacesUtils`, and it avoids the lazy getter to work around the PlacesUtils <-> Bookmarks import cycle. OTOH, we did discuss keeping the validators local to the module in London. In the end, I went with the former, but I don't have a strong preference. If you're not too bothered more than I'm not too bothered, I'll put them back. ;-)
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

https://reviewboard.mozilla.org/r/61076/#review62500

> Extending `fetch` seems like a good idea. I didn't do that in the latest version of this patch, but maybe worthwhile for a follow-up.

Bug 1293458.

> It is my assumption this is just an emergency case, in general it should never happen.
> 
> Since you are moving a bunch of code, it would be nice to have more inline documentation

Added some logging for this, too, per Mark's suggestion.

> we should probably file a bug to change any existing place:folder=X&type=RESULTS_AS_TAG_CONTENTS to a place:tags=name and remove RESULTS_AS_TAG_CONTENTS...

Filed bug 1293445.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3994eef4db59

Just pushed a follow-up that fixes the ESLint failure.
Comment on attachment 8759858 [details]
Bug 1274108 - Migrate the bookmarks engine to `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/57696/#review68190

::: services/sync/tests/unit/test_bookmark_engine.js:67
(Diff revision 14)
>    _("All IDs: " + JSON.stringify(store.getAllIDs()));
>  
>    let mobileID = store.idForGUID("mobile");
>    _("Change the GUID for that item, and drop the mobile anno.");
> -  store._setGUID(mobileID, "abcdefghijkl");
> +  let mobileRoot = BookmarkSpecialIds.specialIdForGUID("mobile", false);
> +  let mobileGUID = Async.promiseSpinningly(PlacesUtils.promiseItemGuid(mobileRoot));

I'd say we should change the test to a task and yield these promises (not a big deal, but should be fairly easy to do)

::: services/sync/tests/unit/test_bookmark_engine.js:438
(Diff revision 14)
> -  do_check_false(store.isTaggable(""));
> -
> -  run_next_test();
> -});
> -
>  add_test(function test_bookmark_tag_but_no_uri() {

ditto here re task
Attachment #8759858 - Flags: review?(markh) → review+
Comment on attachment 8765998 [details]
Bug 1274108 - Add a `PlacesSyncUtils` module.

https://reviewboard.mozilla.org/r/61076/#review68184

Still looks great - thanks!

::: toolkit/components/places/PlacesSyncUtils.jsm:199
(Diff revision 13)
> +    try {
> +      // Use the existing GUID if it exists. `promiseItemGuid` caches the GUID
> +      // as a side effect, and throws if it's invalid.
> +      guid = yield PlacesUtils.promiseItemGuid(itemId);
> +    } catch (ex) {
> +      if (!isInvalidCachedGuidError(ex)) {

looking at placesutils, it seems fairly clear that it really does expect every item to have a GUID. Checking the exception message seems a little fragile, but we have a test change exercises this, so that's probably OK. Please add a log.warn() here, and maybe add a comment mentioning this block can die once bug 1294291 lands.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56d25249618a
Add a `PlacesSyncUtils` module. r=markh
https://hg.mozilla.org/integration/autoland/rev/b38ecfc83b29
Migrate the bookmarks engine to `PlacesSyncUtils`. r=markh
https://hg.mozilla.org/mozilla-central/rev/56d25249618a
https://hg.mozilla.org/mozilla-central/rev/b38ecfc83b29
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1295518
Blocks: 1295519
Blocks: 1295521
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: