Closed Bug 1323867 Opened 5 years ago Closed 5 years ago

Bump bookmark validation threshold to 1k

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: lina, Assigned: lina)

Details

Attachments

(1 file)

It's currently 100 bookmarks. We may want to yield to the event loop in the validation code, to minimize jank. Ride this to Aurora.
We may also want to have validation ride all the way to Release.
Bump first, measure, then make a decision to ride the trains later.
Priority: -- → P1
Assignee: nobody → kit
Bumping the pref is a one-line change, but I noticed a bit of jank with my profile of ~2.4k bookmarks. I suspect we'll want to periodically yield to the event loop, just like we discussed at triage.
Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

https://reviewboard.mozilla.org/r/104606/#review105984

Looks good, but the conversion of some of those functions to generators will break TPS, and [aboutsync](https://github.com/mhammond/aboutsync)... Not breaking TPS is required IMO for landing this, but keeping aboutsync working is a "nice to have".

If you do decide to handle aboutsync, it should be done in a way that won't break on older firefoxes. I believe you can check if a function is a generator via it's constructor property... e.g. `generator.constructor.name === "GeneratorFunction"`, although maybe there's a better way. This kind of sucks, admittedly.
Attachment #8826705 - Flags: review?(tchiovoloni)
Now that we can use async functions in production code, I changed the validator to use them instead of tasks. Still seeing a race in test_bookmark_conflict.js; I'll file a separate bug for that. The Sync ping schema change is also to fix TPS. I submitted a PR for About Sync here: https://github.com/mhammond/aboutsync/pull/23
Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

https://reviewboard.mozilla.org/r/104606/#review110430

lgtm
Attachment #8826705 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

https://reviewboard.mozilla.org/r/104606/#review110926

::: services/sync/modules/bookmark_validator.js:8
(Diff revision 2)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const Cu = Components.utils;
> +const Timer = Components.Constructor("@mozilla.org/timer;1", "nsITimer");

As a meta-comment (as I don't really care!), note there is a Timer.jsm new code tends to use
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c3b1968e183
Bump bookmark validation threshold to 1k. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/2c3b1968e183
https://hg.mozilla.org/mozilla-central/rev/074d6b728c5c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.