Closed Bug 1394525 Opened 7 years ago Closed 7 years ago

Don't reset the tracker score after syncing

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

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 kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/439bc21d3947
Reset tracker scores before syncing. r=markh
https://hg.mozilla.org/mozilla-central/rev/439bc21d3947
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.