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)

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.