Closed Bug 1146068 Opened 5 years ago Closed 5 years ago

Update reading list scheduler to match requirements of the sync engine

Categories

(Firefox Graveyard :: Reading List, defect)

defect
Not set

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(2 files)

Now we have a real RL sync engine, the scheduler needs some tweaks.

Drew, you gave r+-over-the-shoulder of part of this, but I added some other things, in particular, that if an item is added/changed/etc while a sync is in progress we start a new sync immediately after the existing one completes - it may have missed that change and thus may be a long time before it is actually synced.  This includes a test. We also had a preference for "how long after the RL is dirty until we sync" with a default of 2 mins, but talking with rnewman we decided this should be immediate - there's a good chance the user might add an item then shut their laptop - so that's been removed too.
Attachment #8581217 - Flags: review?(adw)
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Here are the log manager fixes we discussed - these are almost identical to what you have in the sync patch, but I also clarified an unrelated comment as I found it difficult to read.  I figure we may as well keep it in this bug as it is part of the scheduler if you squint enough (ie, we rely on the scheduler to take care of RL logs)
Attachment #8581218 - Flags: review?(adw)
Hi Mark, can you provide a point value.
Iteration: --- → 39.2 - 23 Mar
Flags: needinfo?(mhammond)
Points: --- → 2
Flags: needinfo?(mhammond)
Comment on attachment 8581217 [details] [diff] [review]
0005-fix-scheduler-to-match-readinglist-sync-engine-imple.patch

Review of attachment 8581217 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/readinglist/Scheduler.jsm
@@ +122,5 @@
>      this._setupTimer();
>    },
>  
> +  _setupRLListener() {
> +    let maybeSync = () => {

Just a nit, but this doesn't have to be inside _setupRLListener.  IMO it should be a method on the scheduler.  Just a suggestion, feel free to leave it as is.
Attachment #8581217 - Flags: review?(adw) → review+
Attachment #8581218 - Flags: review?(adw) → review+
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
https://hg.mozilla.org/mozilla-central/rev/c12a9bd2cc2b
https://hg.mozilla.org/mozilla-central/rev/497bcc0a0143
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.