Closed Bug 1131414 Opened 5 years ago Closed 5 years ago

Change sync UI to pretend there is a "reading list" engine

Categories

(Firefox :: Sync, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

There are a couple of places where we list the "sync engines" in the UI.  We need to add "reading list" to this even though it will not be implemented as a real sync engine.
Flags: qe-verify-
Flags: firefox-backlog+
I had not realised we listed this anywhere other than the Sync pane in preferences... will we need any strings other than what I mentioned in bug 1129288 comment 5?
Flags: needinfo?(mhammond)
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #1)
> I had not realised we listed this anywhere other than the Sync pane in
> preferences... will we need any strings other than what I mentioned in bug
> 1129288 comment 5?

AFAIK, that's it!
Flags: needinfo?(mhammond)
Blocks: 1132074
Priority: -- → P1
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
This is roughly the minimum we can do today to "disable" sync if none of its engines are enabled, and I think roughly reflects what Richard and I chatted about in IRC.

It adds a new pref for the global state of Sync. Service.enabled is now a getter that reflects the state of the pref. The pref has a default of true and is only set to false by the UI bits (in part 2). I also needed to add one more check for the enabled state as enginesync.js threw when .enabled was false, which I suspect is simply due to it being rarely tested that way. Note that we *do* still need to initialize the identity object even when disabled as browserid_identity has magic we need but can't reasonably refactor in time for reading-list.
Attachment #8575817 - Flags: feedback?(rnewman)
Hopefully somewhat obvious - new checkboxes for the "reading list" engine, which is hidden by default and only shown when both the browser.readinglist.enabled pref is true and FxA sync is used.

Note that the dialog-based prefs changes are not complete yet, which should be simple to fix, but I think it's still worth getting early feedback.
Attachment #8575818 - Flags: feedback?(adw)
Attachment #8575818 - Flags: feedback?(adw) → feedback+
Comment on attachment 8575817 [details] [diff] [review]
0006-Bug-1131414-part-1-add-a-pref-to-indicate-if-sync-is.patch

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

::: services/sync/modules/service.js
@@ +305,5 @@
>      return false;
>    },
>  
> +  // The global "enabled" state comes from prefs, and will be set to false
> +  // whenever the UI that exposes what to Sync finds all Sync engines disabled.

First "sync" should be lowercase.
Attachment #8575817 - Flags: feedback?(rnewman) → feedback+
Comment on attachment 8575817 [details] [diff] [review]
0006-Bug-1131414-part-1-add-a-pref-to-indicate-if-sync-is.patch

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

Thanks for the feedback, and I think this is ready to roll, so requesting review (and please assume I'll fix that typo)
Attachment #8575817 - Flags: review?(rnewman)
Comment on attachment 8575818 [details] [diff] [review]
0007-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

Thanks Drew!
Attachment #8575818 - Flags: review?(adw)
Comment on attachment 8575818 [details] [diff] [review]
0007-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

Ack, sorry, getting ahead of myself - I need to address those XXX comments.
Attachment #8575818 - Flags: review?(adw)
Same patch as before, but with the RL engine's checkbox in the correct place.
Attachment #8575818 - Attachment is obsolete: true
Attachment #8578320 - Flags: review?(adw)
Comment on attachment 8575817 [details] [diff] [review]
0006-Bug-1131414-part-1-add-a-pref-to-indicate-if-sync-is.patch

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

See the nit comment from the feedback. Otherwise, if it works, FEEL GOOD.
Attachment #8575817 - Flags: review?(rnewman) → review+
Comment on attachment 8578320 [details] [diff] [review]
0006-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

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

It looks like the XXX comment in onPreferenceChanged is still there, but r+ with that addressed however you think it should be.
Attachment #8578320 - Flags: review?(adw) → review+
Thanks for the review, but I had to make significant changes to the dialog-based prefs, due to them being "instant apply" on Mac and "modal" on Windows (and preferences.xml doesn't really support that).  So I just hook up a preference observer, and take the action whenever the dialog gets around to actually setting the prefs.  Given these changes I think it's work an additional look over.
Attachment #8578320 - Attachment is obsolete: true
Attachment #8579520 - Flags: review?(adw)
Comment on attachment 8579520 [details] [diff] [review]
0006-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

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

Looks fine to me!  Will be nice when we don't have to worry about the dialog-based prefs anymore...
Attachment #8579520 - Flags: review?(adw) → review+
You need to log in before you can comment on or make changes to this bug.