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

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 2

[Mark Hammond [:markh] [: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!

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0006-Bug-1131414-part-1-add-a-pref-to-indicate-if-sync-is.patch

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.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0007-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

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.

Comment 5

[Richard Newman [:rnewman]]

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.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

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)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8575818 [details] [diff] [review]
0007-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

Thanks Drew!

Comment 8

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0006-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

Same patch as before, but with the RL engine's checkbox in the correct place.

Comment 10

[Richard Newman [:rnewman]]

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.

Comment 11

[Drew Willcoxon :adw]

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.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0006-Bug-1131414-part-2-add-the-reading-list-engine-to-Sy.patch

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.

Comment 13

[Drew Willcoxon :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...

Comment 14

[Mark Hammond [:markh] [:mhammond]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/933b795a1fc1
remote:   https://hg.mozilla.org/integration/fx-team/rev/ac91a79c4cab

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/933b795a1fc1
https://hg.mozilla.org/mozilla-central/rev/ac91a79c4cab

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf356118c79e
https://hg.mozilla.org/releases/mozilla-aurora/rev/395fa1ecf377