59 bytes, text/x-review-board-request
I noticed two issues in `test_engine_changes_during_sync.js#test_bookmark_change_during_sync`: * The Bugzilla bookmark is in `menu` locally, but has a `parentid` of `toolbar` remotely. We should either change its `parentid`, or add a remote `toolbar` record. This isn't currently an issue because we treat the `parentid` as canonical, but it's not technically correct, and will cause validation failures once we switch to using the parent's `children` in bug 1305563. * We reset the score after syncing, meaning we'll sometimes clobber increments made during the sync. Since we already set `ignoreAll` before applying changes for the other engines, I think it's safe to do this at the same time we clear the changed IDs. * `_uploadOutgoing` is probably a better place to make changes during the sync than `recordHandler`, especially once we apply merged bookmark trees in a transaction, and pull changes before we're ready to upload.
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #0) > * `_uploadOutgoing` is probably a better place to make changes during the > sync than `recordHandler` s/recordHandler/applyIncomingBatch
Comment on attachment 8901924 [details] Bug 1394525 - Reset tracker scores before syncing. https://reviewboard.mozilla.org/r/173350/#review178776 ::: services/sync/tests/unit/test_engine_changes_during_sync.js:384 (Diff revision 1) > await assertChildGuids(folder1.guid, [tbBmk.guid], > "Folder should have 1 child before first sync"); > > let pingsPromise = wait_for_pings(2); > > - let changes = await engine.pullNewChanges(); > + let changes = await PlacesSyncUtils.bookmarks.pullChanges(); These are currently equivalent, but will be different in bug 1305563, hence the change. :-)
Comment on attachment 8901924 [details] Bug 1394525 - Reset tracker scores before syncing. https://reviewboard.mozilla.org/r/173350/#review178838 looks great, thanks!
Attachment #8901924 - Flags: review?(markh) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/439bc21d3947 Reset tracker scores before syncing. r=markh
You need to log in before you can comment on or make changes to this bug.