Bump bookmark validation threshold to 1k

RESOLVED FIXED in Firefox 54

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
We may also want to have validation ride all the way to Release.
(Assignee)

Comment 2

2 years ago
Bump first, measure, then make a decision to ride the trains later.
Priority: -- → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → kit
(Assignee)

Comment 3

2 years ago
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 hidden (mozreview-request)

Comment 5

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

Comment 7

2 years ago
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 8

2 years ago
mozreview-review
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 9

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

Comment 13

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c3b1968e183
Bump bookmark validation threshold to 1k. r=tcsc

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c3b1968e183
https://hg.mozilla.org/mozilla-central/rev/074d6b728c5c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.