Don't reset the tracker score after syncing

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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 hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
mozreview-review
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 4

2 months ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 6

2 months ago
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
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.