Closed Bug 1295521 Opened 8 years ago Closed 8 years ago

Remove `BookmarkSpecialIds` and move sync GUID handling into PlacesSyncUtils

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

(Whiteboard: [sync-tracker])

Attachments

(3 files, 1 obsolete file)

Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.
58 bytes, text/x-review-board-request
tcsc
: review+
markh
: review+
Details
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.
58 bytes, text/x-review-board-request
markh
: review+
tcsc
: review+
Details
Bug 1295521 - Add a `toSyncBookmark` method and clean up `BookmarksStore`.
58 bytes, text/x-review-board-request
markh
: review+
Details 
The conversions back and forth between Places GUIDs and Sync record IDs (which are also Places GUIDs, except for the "special" ones) make it too easy for confused code to pass one where another is expected.

I think PlacesSyncUtils should take over management of these special GUIDs, just like it already manages "kinds" instead of item types.
Priority: -- → P3
Whiteboard: [sync-quality]
This is a ton of artificial (afaict) complexity in the bookmark sync code so yes, please do this. 

Worth noting that the validator will need to be fixed to do the correct mapping if you do so (I mention it because I don't think we have tests in place to cover this, although if you got rid of the specialids code entirely you would have to fix it).
Assignee: nobody → kcambridge
Nominating this for Monday's triage meeting. Converting back and forth adds noise to bug 1274496 and bug 1258127, which are already complicated enough.
Priority: P3 → --
Whiteboard: [sync-quality] → [sync-tracker]
Priority: -- → P2
Rank: 1
A first cut at this on a Friday afternoon. Tests don't pass yet, but the goal is to tease apart the difference between Places and Sync bookmark objects, and make sure `PlacesSyncUtils` only returns the latter.
Status: NEW → ASSIGNED
Rank: 1 → 0
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review76514

A quick drive-by and I didn't get to the end, but this seems a massive patch which introduces a significant amount of complexity, with the stated aim being to just simplify the special casing of ~5 "special" GUIDs that correspond to roots. I'm really quite confused by the additional abstractions introduced here and how they relate to each other.

::: toolkit/components/places/PlacesSyncUtils.jsm:58
(Diff revision 1)
> +
> +// A map of conversion functions from Places bookmark properties to Sync
> +// bookmark properties. In this module, we use `bookmarkInfo` to refer to
> +// objects passed to Places methods, and `bookmarkItem` for objects
> +// returned by those methods.
> +const BOOKMARK_CONVERSIONS = {

The names BOOKMARK_CONVERSIONS and SYNC_BOOKMARK_CONVERSIONS aren't very clear to me, and don't seem to match the (duplicated) comment above them - ie, I'd expect one of the 2 to have "info" in the name and the other to have "item"

::: toolkit/components/places/PlacesSyncUtils.jsm:95
(Diff revision 1)
> +  },
> +};
> +
> +// A map of conversion functions from Sync bookmark object properties to
> +// Places bookmark properties. In this module, `syncBookmarkInfo` refers to
> +// objects passed to `BookmarkSyncUtils` methods, and `syncBookmarkItem`

I'm quite confused by this - we now have *four* different ways to represent a bookmark? And Sync itself passes syncBookmarkInfo objects in to this module, but gets syncBookmarkItems back? That seems odd, as I'd expect it to send and receive the same types of objects.

::: toolkit/components/places/PlacesSyncUtils.jsm:156
(Diff revision 1)
>      SEPARATOR: "separator",
>    },
>  
> +  ROOT_IDS: ["menu", "places", "tags", "toolbar", "unfiled", "mobile"],
> +
> +  /**

What's a "bookmark object"? As per the comment above, I'd expect it to be either "bookmark item" or "bookmark info"

::: toolkit/components/places/PlacesSyncUtils.jsm:583
(Diff revision 1)
> -  let isOrphan = yield GUIDMissing(insertInfo.parentGuid);
> +  // folder ID to refer to the local tag folder.
> +  syncBookmarkInfo = yield updateTagQueryFolder(syncBookmarkInfo);
> +
> +  // Convert the Sync bookmark object to a bookmark object that we can use
> +  // with the `PlacesUtils.bookmarks` methods.
> +  let bookmarkInfo = yield syncBookmarkToBookmark(syncBookmarkInfo);

The naming here seems confusing too - we are starting with a "syncBookmarkInfo" then converting it to a "bookmarkInfo" via a function named "syncBookmarkToBookmark" - casual reading of this function makes me quite confused what's going on.

::: toolkit/components/places/PlacesSyncUtils.jsm:1016
(Diff revision 1)
> -  // Sync doesn't track modification times, so use the default.
> -  let time = new Date();
> -  insertInfo.dateAdded = insertInfo.lastModified = time;
> +  return syncBookmarkInfo;
> +}
> +
> +var convertBookmarkObject = Task.async(function* (input, output, converters) {
> +  for (let prop in input) {
> +    if (converters.hasOwnProperty(prop)) {

It seems odd that we'd just silently drop elements without a converter. I'd say we should throw.
Thanks for taking a look at this! I think you're right. The core of this patch is pretty small, but the other parts add a lot of noise. I can break them out into a separate bug, assuming we want them at all. Let me try to explain my thinking, and you can tell me if that makes sense, or if I'm totally wrong.

* We need to convert incoming record IDs to Places GUIDs. They're identical except for the roots, but I don't want to call them `guid` and `parentGuid` because that's what Bookmarks.jsm uses for real GUIDs. I think all our `PlacesSyncUtils` functions should take these record IDs and convert them. Once mobile becomes a real root, `syncIdToGuid` and `guidToSyncId` become one-line synchronous functions.

* I changed the argument names because it's sometimes unclear whether we're working with incoming sync info, Places bookmark info that we pass to Bookmarks.jsm, an item that we get back from Bookmarks.jsm, and a Sync item that we return with additional properties. But "syncBookmarkInfo" and "syncBookmarkItem" are really verbose, and the renaming makes the patch bigger than it needs to be. I settled on using item/info for Sync objects, and bookmarkInfo/bookmarkItem for Places objects. This also helps with `PlacesSyncUtils` only returning Sync bookmark objects (IOW, the engine only sees `syncId` `parentSyncId`, etc.), not "Places bookmark objects with stuff tacked on."

* The conversions would help with moving `BookmarksStore.prototype.createRecord` into `PlacesSyncUtils`. My plan was to have `PlacesSyncUtils.bookmarks.fetch` return a Sync item with all the right properties set...but `fetch` and the `populate*Metadata` calls can be in a follow-up.

WDYT?
Priority: P2 → P1
Depends on: 1302286
Blocks: 1302288
Blocks: 1302286
No longer depends on: 647605, 1302286
Blocks: 1302289
This is a pretty big patch, though most of it is renaming and moving stuff around. I'd love for both of you to have a look. It'd be nice to get this in for 51, but no pressure if you'd prefer to take your time. Some general comments:

* I simplified the conversion functions: we call `syncBookmarkToPlacesBookmark` whenever we're passing a Sync bookmark to a Places method, and `placesBookmarkToSyncBookmark` when we have a Places bookmark that we want to return. (In a nutshell: PlacesSyncUtils only takes and returns Sync bookmark objects). I'll also use `placesBookmarkToSyncBookmark` in bug 1302288.

* I'm not really happy with exposing `createMobileRoot`, but `idForGUID` needs it. The only remaining callers of `GUIDForId` and `idForGUID` are `createRecord`, `exists`, and tests, so this can be removed once they're converted over.

* It's unfortunate that these functions are async. Once bug 1302901 lands, we can make them sync, and remove the `Async.promiseSpinningly` calls.
Now I'm a little nervous about all the `Async.promiseSpinningly` calls...

> 12:45 <mconley> https://pastebin.mozilla.org/8910432
> 12:46 <mconley> I just caught the end of it it looks like, but I was inside makeSpinningCallback
> 12:47 <kitcambridge> ugh, yeah, sync uses that heavily
> 12:48 <kitcambridge> looks like that's within _refreshReconcilerState, if i'm reading that right?
> 12:50 <kitcambridge> ...which fetches all addons, and all installs with another spinning callback
> 12:53 <mconley> kitcambridge: you’re inside _refreshReconcilerState, correct
> 12:54 <mconley> though I’ll have to trust you on what that does. :)
> 12:55 <kitcambridge> mconley: would spinning the event loop like makeSpinningCallback does with `Services.tm.currentThread.processNextEvent(true)` cause jank like that?
> 12:55 <kitcambridge> for long-running operations?
> 12:55 <mconley> nested event loops, eh?
> 12:55 <mconley> Uuuuusually a bad idea, from my experience
> 12:55 <kitcambridge> yeah
> 12:56 <kitcambridge> sync is built on those :|
> 12:56 <mconley> and my answer is “maybe”, because as soon as you have nested event loops, you get all sorts of weird re-entry possibilities

For posterity, the full Cleopatra URL is https://cleopatra.io/#report=cdb0449548c62c409258fafeec83fc2d7a915581&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A35352895,%22end%22%3A35353106%7D%5D&selection=0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,29,30,35,36,37,38,39,30,31,32,33,34,29,30,35,40,38,39,30,31,32,33,34,29,30,41,29,30,35,42,43,38,39,30,31,32,33,34,29,30,35,44,38,39,30,41,29,30,35,45,38,39,30,31,32,33,34,29,30,41,29,30,35,46,38,39,30,31,32,33,34,29,30,35,38,39,47,48,49,50,51,52,53,30,35,54,38,39,30,31,32,33,34,29,30,35,55,38,39,30,31,32,33,34,29,30,35,45,38,39,30,31,32,33,34,29,30,41,29,30,35,56,38,39,30,31,32,33,34,29,30,35,57,58,38,39,30,31,32,33,34,29,30,35,38,59,48,49,60,61,62,63,64,65,66,24,67,68,69,70,71,72,73,74,75,85,86.
(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> Now I'm a little nervous about all the `Async.promiseSpinningly` calls...
...
> > 12:55 <kitcambridge> mconley: would spinning the event loop like makeSpinningCallback does with `Services.tm.currentThread.processNextEvent(true)` cause jank like that?
> > 12:55 <kitcambridge> for long-running operations?
> > 12:55 <mconley> nested event loops, eh?
> > 12:55 <mconley> Uuuuusually a bad idea, from my experience
> > 12:55 <kitcambridge> yeah
> > 12:56 <kitcambridge> sync is built on those :|
> > 12:56 <mconley> and my answer is “maybe”, because as soon as you have nested event loops, you get all sorts of weird re-entry possibilities

There are a few facets to this. Sync itself is built on nested event loops, and promiseSpinningly is just one such hack. I've no reason to believe promiseSpinningly is any worse than all the other places Sync turns async code into sync code by spinning. IOW, I don't think promiseSpinningly will have additional problems, but just highlighting existing problems.

Further, sadly, and counter-intuitively, truly promise based code can still be *extremely* janky - eg, see bug 1186714 - no nested event loops, but still enough jank that we ended up adding "sleeps" into the main loop. That bug came out of some work I did a year or so ago to identify jank in Sync - the instrumentation I did found a few candidates (such as that bug) but not many others. I'm not sure if that was due to the methodology (in terms of measurement) or due to the test load (ie, I didn't manage to force Sync into the truly janky parts). At some point we should consider resurrecting those jank instrumentations and see what they say today (I didn't end up checking them in as they were really quite ugly.) My work was mainly focused around bookmarks, so there's a reasonable chance we could still provoke jank with history or forms.
I also meant to say - bug 1168428 is the meta bug for Sync jank - but it has no open deps.
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review77748

Sorry, but I did't get to the second patch today - but I think this is looking great and is a nice improvement to the engine.

::: services/sync/modules/bookmark_validator.js:126
(Diff revision 3)
>    createClientRecordsFromTree(clientTree) {
>      // Iterate over the treeNode, converting it to something more similar to what
>      // the server stores.
>      let records = [];
>      function traverse(treeNode) {
> -      let guid = BookmarkSpecialIds.specialGUIDForId(treeNode.id) || treeNode.guid;
> +      let guid = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.guidToSyncId(treeNode.guid));

I'm not too bothered by this, but I'd rather see the validator implemented as promises and have the promiseSpinningly moved back into Sync. IOW, I think many of these functions could consider using Task.jsm, and we should theoretically only need the promiseSpinningly call where Sync actually calls the validator.

At a minimum, I think we should get a bug on file to convert the validators to promises, just so it reduces the work we need to do when we can finally convert sync to fully async.

::: services/sync/modules/engines/bookmarks.js:474
(Diff revision 3)
>        let statement = db.createAsyncStatement(query);
>        try {
>          for (let i = 0; i < chunkLength; i++) {
>            statement.bindByIndex(i, modifiedGUIDs[startIndex + i]);
>          }
> -        let results = Async.querySpinningly(statement, ["id", "guid"]);
> +        let results = Async.querySpinningly(statement, ["guid"]);

I wonder if we can make this promise based so we only end up with 1 event-loop spin for the entire function?

::: services/sync/modules/engines/bookmarks.js:499
(Diff revision 3)
>          // The `===` check also filters out old persisted timestamps,
>          // which won't have a `deleted` property.
>          continue;
>        }
> -      let guid = BookmarkSpecialIds.syncIDToPlacesGUID(syncID);
> +      let guid = Async.promiseSpinningly(
> +        PlacesSyncUtils.bookmarks.syncIdToGuid(syncID));

ISTM that maybe the tracker should do this conversion? ie, once your tracker patch lands, it seems natural it would give you a "guid" rather than a "syncId"?

(also re naming - we tend to use "ID" and "GUID" rather than "Id", so maybe SyncIdToGuid should be SyncIDToGUID? Not too bothered by this, and TBH I can't remember what PlacesSyncUtils already does here)

::: services/sync/modules/engines/bookmarks.js:856
(Diff revision 3)
> -      ex => -1));
> +          createMobileRoot: !noCreate,
> +        });
> +        if (placesGuid) {
> +          id = yield PlacesUtils.promiseItemId(placesGuid);
> +        }
> +      } catch (ex) {}

I think a log.warn/error here might make sense (or log.trace or so if it really is expected)

::: services/sync/modules/engines/bookmarks.js:901
(Diff revision 3)
>        FROM changeRootContents
>        JOIN moz_bookmarks USING (id)
>      `;
>  
>      let statement = this._getStmt(query);
> -    let results = Async.querySpinningly(statement, ["id", "guid"]);
> +    let results = Async.querySpinningly(statement, ["guid"]);

similarly to the above, can we arrange for this to be a single spin?
Attachment #8789964 - Flags: review?(markh)
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review77802

Looks pretty good, happy to see this mess gone. Couple of issues which might just be personal confusion.

::: services/sync/modules/bookmark_validator.js:126
(Diff revision 3)
>    createClientRecordsFromTree(clientTree) {
>      // Iterate over the treeNode, converting it to something more similar to what
>      // the server stores.
>      let records = [];
>      function traverse(treeNode) {
> -      let guid = BookmarkSpecialIds.specialGUIDForId(treeNode.id) || treeNode.guid;
> +      let guid = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.guidToSyncId(treeNode.guid));

Is it possible for us to arrange for promiseBookmarksTree to pass this in? IIUC it would let you remove the promiseSpinningly's here and in \_buildGUIDMap. (It would probably make it easier to test and for aboutsync to use)

If doing so is nontrivial, I'd be fine with doing it in a followup.

::: services/sync/modules/bookmark_validator.js:265
(Diff revision 3)
> -        //
> -        // To make things worse, this doesn't even work for root________, which has
> -        // the special id 'places'.
>          record.childGUIDs = record.children;
>          record.children = record.children.map(childID => {
> -          let match = childID.match(/_+$/);
> +          return Async.promiseSpinningly(PlacesSyncUtils.bookmarks.guidToSyncId(childID));

Can this result in a different value on different clients (in the case of the mobile root -- which AFAICT is the only time it has to do a DB hit in order to figure out the value)?

If so I think we could have a situation where the id we get here isn't the same as the record's id from the server.
Attachment #8789964 - Flags: review?(tchiovoloni)
Comment on attachment 8791556 [details]
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/78946/#review77822

Looks good to me.

r=me with nits fixed

::: toolkit/components/places/PlacesSyncUtils.jsm:110
(Diff revision 1)
> +  }),
> +
>    /**
>     * Fetches a folder's children, ordered by their position within the folder.
>     */
> -  fetchChildGuids: Task.async(function* (parentGuid) {
> +  fetchChildren: Task.async(function* (parentSyncId) {

nit: unless I'm missing something, this isn't actually returning children, it's returning their sync id's, so the name is somewhat unclear now (as well as the comment, although that's not new). Something like fetchChildSyncIds seems more accurate.

::: toolkit/components/places/PlacesSyncUtils.jsm:133
(Diff revision 1)
>     *
>     */
> -  order: Task.async(function* (parentGuid, childGuids) {
> -    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(parentGuid);
> -    for (let guid of childGuids) {
> -      PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(guid);
> +  order: Task.async(function* (parentSyncId, childSyncIds) {
> +    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(parentSyncId);
> +    if (!childSyncIds.length) {
> +      return Promise.resolve();

nit: You should just be able to `return;` since you're in a task (here and elsewhere).
Attachment #8791556 - Flags: review?(tchiovoloni) → review+
Depends on: 1303405
Comment on attachment 8791556 [details]
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/78946/#review77822

> nit: unless I'm missing something, this isn't actually returning children, it's returning their sync id's, so the name is somewhat unclear now (as well as the comment, although that's not new). Something like fetchChildSyncIds seems more accurate.

Good call! Renamed.

> nit: You should just be able to `return;` since you're in a task (here and elsewhere).

Sadly, we have an eslint rule ("consistent-return") that complains if I do this. Alternatively, I could change the last return to `yield PlacesUtils.withConnectionWrapper(...)`, but I think this reads a bit better.
Comment on attachment 8791556 [details]
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/78946/#review77822

> Sadly, we have an eslint rule ("consistent-return") that complains if I do this. Alternatively, I could change the last return to `yield PlacesUtils.withConnectionWrapper(...)`, but I think this reads a bit better.

I see. Well, I guess I understand that, although Promise.resolve() will unnecessarially create extra promise objects (which probably doesn't matter much in practice)
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review77748

Thanks for reviewing!

> I'm not too bothered by this, but I'd rather see the validator implemented as promises and have the promiseSpinningly moved back into Sync. IOW, I think many of these functions could consider using Task.jsm, and we should theoretically only need the promiseSpinningly call where Sync actually calls the validator.
> 
> At a minimum, I think we should get a bug on file to convert the validators to promises, just so it reduces the work we need to do when we can finally convert sync to fully async.

Thinking a bit about our conversation yesterday...since there's no rush getting this in, we might as well wait for the mobile root, and then we can avoid spinning entirely. It removes a ton of complexity from converting back and forth, and making the validator async for a function that'll become synchronous just adds code churn.

> I wonder if we can make this promise based so we only end up with 1 event-loop spin for the entire function?

It's synchronous now, so no spinning needed, yay! I think it's OK to keep `querySpinningly`...we'll chunk after 999 records; I don't expect the tracker to accumulate that many changes between syncs. (And the first sync uses a different query).

> ISTM that maybe the tracker should do this conversion? ie, once your tracker patch lands, it seems natural it would give you a "guid" rather than a "syncId"?
> 
> (also re naming - we tend to use "ID" and "GUID" rather than "Id", so maybe SyncIdToGuid should be SyncIDToGUID? Not too bothered by this, and TBH I can't remember what PlacesSyncUtils already does here)

Agreed! The tracker patch moves this entire query into `PlacesSyncUtils.bookmarks.pull{All, New}Changes`, and that'll return sync IDs so that the bookmarks engine doesn't have to convert between them. The difference in naming is my fault: Places seems to use `Id` and `Guid` consistently, so `PlacesSyncUtils` does the same at the cost of inconsistency with Sync.

> similarly to the above, can we arrange for this to be a single spin?

Fixed by making `guidToSyncId` synchronous. ;-)
No longer blocks: 1302286, 1302289
Depends on: 1302901
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review77802

Thanks, Thom! I rebased this on top of the mobile root, so we can ditch `promiseSpinningly` and the DB lookups.
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review77802

> Can this result in a different value on different clients (in the case of the mobile root -- which AFAICT is the only time it has to do a DB hit in order to figure out the value)?
> 
> If so I think we could have a situation where the id we get here isn't the same as the record's id from the server.

Addendum: I think this should work fine with the new mobile root. Locally, it has a new GUID, but we map that to "mobile", and it's still called "mobile" on the server. Older clients without a real root will still map it based on the anno.
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review78002

::: services/sync/modules/engines/bookmarks.js:580
(Diff revision 4)
> -        parentGuid: BookmarkSpecialIds.syncIDToPlacesGUID(record.parentid),
> +        parentSyncId: record.parentid,
>          title: record.title,
> -        guid: BookmarkSpecialIds.syncIDToPlacesGUID(record.id),
> +        syncId: record.id,
>          tags: record.tags,
>          keyword: record.keyword,
> +        loadInSidebar: record.loadInSidebar,

Isn't this going to report "ReferenceError: reference to undefined property..."?

::: services/sync/modules/engines/bookmarks.js:661
(Diff revision 4)
>    update: function BStore_update(record) {
>      let info = {
> -      parentGuid: BookmarkSpecialIds.syncIDToPlacesGUID(record.parentid),
> -      guid: BookmarkSpecialIds.syncIDToPlacesGUID(record.id),
> +      parentSyncId: record.parentid,
> +      syncId: record.id,
>        kind: record.type,
> +      title: record.title,

ditto here re "property is undefined"?

::: services/sync/tests/unit/test_bookmark_engine.js:497
(Diff revision 4)
>    // We're simply checking that no exception is thrown, so
>    // no actual checks in this test.
>  
>    yield PlacesSyncUtils.bookmarks.insert({
>      kind: PlacesSyncUtils.bookmarks.KINDS.BOOKMARK,
> -    guid: Utils.makeGUID(),
> +    syncId: Utils.makeGUID(),

ISTM these new property names should end with "ID" too?
Attachment #8789964 - Flags: review?(markh)
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review78002

> ISTM these new property names should end with "ID" too?

Oops - I see your comment about keeping it consistent with places, which is fine.
Comment on attachment 8791556 [details]
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/78946/#review78008

This looks good to me, although I'm a little confused by some of the validation changes.

Please also wait for Thom's review, as I admit I skimmed over a few of the changes :)

::: toolkit/components/places/PlacesSyncUtils.jsm:154
(Diff revision 2)
> -        for (let i = 0; i < childGuids.length; ++i) {
> -          let guid = childGuids[i];
> +        for (let i = 0; i < childSyncIds.length; ++i) {
> +          let guid = BookmarkSyncUtils.syncIdToGuid(childSyncIds[i]);
>            let child = findChildByGuid(children, guid);
>            if (!child) {
>              delta++;
> -            BookmarkSyncLog.trace(`order: Ignoring missing child ${guid}`);
> +            BookmarkSyncLog.trace(`order: Ignoring missing child ${

minor style comment: I think the wrapping of the line here hurts rather than helps readability.

::: toolkit/components/places/PlacesSyncUtils.jsm:624
(Diff revision 2)
>  
>  var updateSyncBookmark = Task.async(function* (updateInfo) {
> -  let oldItem = yield PlacesUtils.bookmarks.fetch(updateInfo.guid);
> -  if (!oldItem) {
> -    throw new Error(`Bookmark with GUID ${updateInfo.guid} does not exist`);
> +  let guid = BookmarkSyncUtils.syncIdToGuid(updateInfo.syncId);
> +  let oldBookmarkItem = yield PlacesUtils.bookmarks.fetch(guid);
> +  if (!oldBookmarkItem) {
> +    throw new Error(`Bookmark with sync ID ${

ditto :)

::: toolkit/components/places/PlacesSyncUtils.jsm:637
(Diff revision 2)
>      // we must remove and reinsert.
>      shouldReinsert = true;
> +    if (BookmarkSyncLog.level <= Log.Level.Warn) {
> +      let oldSyncId = BookmarkSyncUtils.guidToSyncId(oldBookmarkItem.guid);
> -    BookmarkSyncLog.warn(`updateSyncBookmark: Local ${
> +      BookmarkSyncLog.warn(`updateSyncBookmark: Local ${
> -      oldItem.guid} kind = (${oldKind}); remote ${
> +        oldSyncId} kind = ${oldKind}; remote ${

even more so here and below!

::: toolkit/components/places/PlacesSyncUtils.jsm
(Diff revision 2)
>  function validateNewBookmark(info) {
>    let insertInfo = validateSyncBookmarkObject(info,
>      { kind: { required: true }
> -    // Explicitly prevent callers from passing types.
> +    , syncId: { required: true }
> -    , type: { validIf: () => false }
> -    // Because Sync applies bookmarks as it receives them, it doesn't pass

I don't understand the removal of these attributes?

::: toolkit/components/places/PlacesSyncUtils.jsm
(Diff revision 2)
>  
> -  // Sync doesn't track modification times, so use the default.
> -  let time = new Date();
> -  insertInfo.dateAdded = insertInfo.lastModified = time;
> -
> -  insertInfo.type = getTypeForKind(insertInfo.kind);

nor these?
Attachment #8791556 - Flags: review?(markh) → review+
(In reply to Mark Hammond [:markh] from comment #27)
> Please also wait for Thom's review, as I admit I skimmed over a few of the
> changes :)

Oops - I got confused - Thom's already looked at this, so go for it (assuming there's a good story for those validation changes ;)
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

I don't know if I let reviewboard down, or it let me down :)
Attachment #8789964 - Flags: review+
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review78144

Looks great!
Attachment #8789964 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8791556 [details]
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/78946/#review78008

> I don't understand the removal of these attributes?

I separated the two in this patch, so SYNC_BOOKMARK_VALIDATORS doesn't inherit from BOOKMARK_VALIDATORS. "guid", "parentGuid", "type", and "index" aren't valid properties anymore, and the validator will throw if you forget to pass "syncId", "parentSyncId", or "kind".

> nor these?

This is set in `syncBookmarkToPlacesBookmark` now, since both insert and update need it.
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review78002

> Isn't this going to report "ReferenceError: reference to undefined property..."?

It'll print a warning if you have `javascript.options.strict` enabled, but it's harmless. I didn't realize that the validator treats undefined the same as if you didn't pass the property at all when I wrote the original code...so there's no need for `RECORD_PROPS_TO_BOOKMARK_PROPS` or the loop. I can add them back if you'd prefer, though!
(In reply to Kit Cambridge [:kitcambridge] from comment #32)
> It'll print a warning if you have `javascript.options.strict` enabled, but
> it's harmless.

It's noise we should try and avoid though IMO. If most incoming records cause the warning to be generated a number of times, a first sync of a new device could be very noisy indeed.

> so there's no need for `RECORD_PROPS_TO_BOOKMARK_PROPS` or the loop.
> I can add them back if you'd prefer, though!

I'm perfectly fine with the restructure so long as it doesn't spew those warnings.
Comment on attachment 8791556 [details]
Bug 1295521 - Replace GUIDs with sync IDs in `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/78946/#review78008

> even more so here and below!

The funky like breaking is unfortunate. I did this because multiline strings include newlines and whitespace, so we'd end up with logs that look like this:

> 1474365611836 BookmarkSyncUtils DEBUG insertSyncBookmark: Item"
>                                    bgrwEPoc3bkV is an orphan:"
>                                    parent vNxp_JVHzDBY"
>                                    doesn't exist; reparenting to unfiled"
> 1474365611880 BookmarkSyncUtils DEBUG insertSyncLivemark:
>                                    8C7jCXln2QHg missing feed URL

...But we can use a template tag to collapse the whitespace. Or, if you think it's OK, we can just keep it.
Blocks: 1258127
Comment on attachment 8789964 [details]
Bug 1295521 - Remove `BookmarkSpecialIds` from Sync.

https://reviewboard.mozilla.org/r/77984/#review78946
Attachment #8789964 - Flags: review?(markh) → review+
Comment on attachment 8792840 [details]
Bug 1295521 - Add a `toSyncBookmark` method and clean up `BookmarksStore`.

https://reviewboard.mozilla.org/r/79730/#review78948

LGTM
Attachment #8792840 - Flags: review?(markh) → review+
Comment on attachment 8792841 [details]
Bug 1295521 - Clean up multiline log messages.

https://reviewboard.mozilla.org/r/79732/#review78954

TBH I don't care enough about those long messages to change every one to use string concatenation, and I get the impression this patch was made just because I raised the issue. In general, I'd tend to prefer a 100 char line without funky wrapping than a line artificially split at 80, but that's personal and YMMV.

So r+ as it looks fine, but feel free to just drop this patch.
Attachment #8792841 - Flags: review?(markh) → review+
Attachment #8792841 - Attachment is obsolete: true
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f49d6c536351
Replace GUIDs with sync IDs in `PlacesSyncUtils`. r=markh,tcsc
https://hg.mozilla.org/integration/autoland/rev/be575ae455e8
Remove `BookmarkSpecialIds` from Sync. r=markh,tcsc
https://hg.mozilla.org/integration/autoland/rev/c902b27afaa4
Add a `toSyncBookmark` method and clean up `BookmarksStore`. r=markh
Blocks: 1309332
https://hg.mozilla.org/mozilla-central/rev/f49d6c536351
https://hg.mozilla.org/mozilla-central/rev/be575ae455e8
https://hg.mozilla.org/mozilla-central/rev/c902b27afaa4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1309647
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: