Closed Bug 1433178 Opened 3 years ago Closed 3 years ago

Write weakly uploaded records back to the mirror

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: lina, Assigned: eoger, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1433178 p1 - Add public domain license headers to test files.
59 bytes, text/x-review-board-request
lina
: review+
Details
Bug 1433178 p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads.
59 bytes, text/x-review-board-request
lina
: review+
Details 
`BufferedBookmarksEngine#_onRecordsWritten` currently writes successfully uploaded records back to the mirror, using the cleartexts in the strong changeset.

However, it doesn't write weakly uploaded records back, because `_createRecord` doesn't keep those cleartexts around. This isn't a big deal for date added, but probably more important if repair uploaded missing records.

It might also be worth explicitly passing the weakly uploaded GUIDs to `apply`, and having it inflate records for everything. The distinction between strong and weak uploads is somewhat blurry (records with date added are weakly uploaded, but they're in the "strong" changeset), anyway, now that we don't use the changeset to resolve conflicts.
Priority: -- → P2
Mentor: kit
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8954833 [details]
Bug 1433178 p1 - Add public domain license headers to test files.

https://reviewboard.mozilla.org/r/223990/#review230078

Oh, I'm so sorry, I wasn't clear earlier. :-( Test code should use the public domain header from https://www.mozilla.org/en-US/MPL/headers/, not MPL.
Attachment #8954833 - Flags: review?(kit) → review-
Comment on attachment 8954834 [details]
Bug 1433178 p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads.

https://reviewboard.mozilla.org/r/223992/#review230082

Excellent!

::: toolkit/components/places/SyncedBookmarksMirror.jsm:307
(Diff revision 1)
>     * @param  {Number} [options.localTimeSeconds]
>     *         The current local time, in seconds.
>     * @param  {Number} [options.remoteTimeSeconds]
>     *         The current server time, in seconds.
> +   * @param  {String[]} [options.weakUpload]
> +   *         GUID of bookmarks to weakly upload.

Nit: GUIDs, here and above `stageItemsToUpload`.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:316
(Diff revision 1)
>     *         succeeds.
>     */
>    async apply({ localTimeSeconds = Date.now() / 1000,
> -                remoteTimeSeconds = 0 } = {}) {
> -    let hasChanges = await this.hasChanges();
> +                remoteTimeSeconds = 0,
> +                weakUpload = [] } = {}) {
> +    let hasChanges = weakUpload.length > 0 || (await this.hasChanges());

This looks good to me. If we don't have any changes, but do have weak uploads, we might be able to just run https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/toolkit/components/places/SyncedBookmarksMirror.jsm#427-428,430-431,440-441,443. However, I don't know the future of repair, and we'll probably remove weak uploads for dates eventually, so I think it's perfectly OK to run the merge for the case.

::: toolkit/components/places/tests/sync/test_bookmark_explicit_weakupload.js:34
(Diff revision 1)
> +
> +  let changesToUpload = await buf.apply({
> +    weakUpload: ["mozBmk______"]
> +  });
> +
> +  ok("mozBmk______" in changesToUpload);

Let's check that the `counter` for `mozBmk______` is 0, too.
Attachment #8954834 - Flags: review?(kit) → review+
Comment on attachment 8954833 [details]
Bug 1433178 p1 - Add public domain license headers to test files.

https://reviewboard.mozilla.org/r/223990/#review230118

Thanks!
Attachment #8954833 - Flags: review?(kit) → review+
Comment on attachment 8954834 [details]
Bug 1433178 p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads.

https://reviewboard.mozilla.org/r/223992/#review230122

::: services/sync/modules/engines/bookmarks.js:730
(Diff revision 2)
>        let buf = await this._store.ensureOpenMirror();
>        let recordsToUpload = await buf.apply({
>          remoteTimeSeconds: Resource.serverTime,
> +        weakUpload: [...this._needWeakUpload.keys()],
>        });
> +      this._needWeakUpload.clear();

Mind removing https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/services/sync/modules/engines/bookmarks.js#739-741, too, please?
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 24ec853fb430587e77bf4b969847be311738e4c8 -d e9d7a3f92bcc: rebasing 450070:24ec853fb430 "Bug 1433178 p1 - Add public domain license headers to test files. r=kitcambridge"
merging toolkit/components/places/tests/sync/head_sync.js
warning: conflicts while merging toolkit/components/places/tests/sync/head_sync.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ccd9765a817
p1 - Add public domain license headers to test files. r=kitcambridge
https://hg.mozilla.org/integration/autoland/rev/ae1084525659
p2 - Allow callers to SyncedBookmarksMirror.apply() to pass explicit weak uploads. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/4ccd9765a817
https://hg.mozilla.org/mozilla-central/rev/ae1084525659
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.