Closed
Bug 1323867
Opened 8 years ago
Closed 7 years ago
Bump bookmark validation threshold to 1k
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
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.
|
Assignee | |
Comment 1•8 years ago
|
||
We may also want to have validation ride all the way to Release.
|
|
Assignee | |
Comment 2•7 years ago
|
||
Bump first, measure, then make a decision to ride the trains later.
Priority: -- → P1
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → kit
|
|
Assignee | |
Comment 3•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c3b1968e183 Bump bookmark validation threshold to 1k. r=tcsc
|
|
||
Comment 14•7 years ago
|
||
Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/074d6b728c5c followup, placate eslint
|
||
Comment 15•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/2c3b1968e183 https://hg.mozilla.org/mozilla-central/rev/074d6b728c5c
Status: NEW → RESOLVED
Closed: 7 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.
Description
•