965606 - Give users the choice to only sync home panel data when on wifi

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect, P1)

Description

[:Margaret Leibovic]

In bug 964447, we're giving add-ons the ability to register to receive notifications when it's a good time to sync their data. I created a pref to only fire these notifications when the user is connected to a local network, but we should create a user-visible way to control that.

I think it's reasonable to default this pref to false (syncing over cell network okay), but it could be a nice feature for bandwidth-sensitive users.

Comment 1

[Ian Barlow (:ibarlow)]

I might start by adding a new section to the "Home" settings panel:


------------------------------------
CONTENT SETTINGS
------------------------------------
Automatic updates
Enabled / Only over Wifi / Disabled

Comment 2

[Chenxia Liu [:liuche]]

I think the intent of the pref is "Update over Wi-Fi only" versus not, which is just two states. At the moment, it doesn't really make sense to have "disabled" updates, since we don't have a manual way to refresh (bug 970707).

If we want to support the three states, we'll have to change how the pref is used. Do we actually want to offer "Never update" as an option though?

I'd go with just supporting this pref as a checkbox, with a string like "Update over Wi-Fi only", and changing that once we support manual refreshes.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Wifi option under content settings

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Standalone wifi option

I have a slight a preference for the wifi option being standalone - the double category doesn't look so strange, and it matches our Search settings as well (where there is a single stand-alone preference).

On the other hand, it might not be clear what is being updated over wifi if we're not putting the pref under "Content settings".

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Wifi option under content settings

Moved category to the bottom, where it makes more sense.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chenxia Liu [:liuche] from comment #5)
> Created attachment 8383985 [details]
> Screenshot: Wifi option under content settings
> 
> Moved category to the bottom, where it makes more sense.

I do like having the setting at the bottom, under the list of panels.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't know exactly why, but I like the "chooser" approach over a "checkbox" approach. The "chooser" gives us more text so we can convey the context better. The "checkbox" is binary, and sometimes the opposite of "checked" is not easy to figure out. Especially with localization.

I'd be happy even using a "chooser" with two choices:

Automatic updates
Enabled / Only over Wifi

We can add "Disabled" or "Manual" later and it won't break anything.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Example of confusing "checkbox":

If "Update only over Wi-Fi" is not checked, does it mean:
1. Always update, regardless of Wi-Fi
2. Never update

Comment 9

[Ian Barlow (:ibarlow)]

Agree with Mark about the popup chooser.

> Automatic updates
> Enabled / Only over Wifi

It may be "more work" for the user but it explains the options much more clearly than a checkbox

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Part 0: Change home.sync.wifiOnly to int pref

Our Java prefs code handles prefs differently based on their type so we fake the types of certain prefs as strings; keeping this pref as a boolean adds even more complexity, so I'm just going to switch this to ints.

Once we implement pull-to-refresh, we can consider adding "Disabled" to the pref options as well.

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Part 1: Expose home panels wifi pref in Settings

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Wifi option under content settings

Comment 13

[:Margaret Leibovic]

Comment on attachment 8386253 [details] [diff] [review]
Part 0: Change home.sync.wifiOnly to int pref

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

::: mobile/android/modules/HomeProvider.jsm
@@ +134,5 @@
>     * @return boolean Whether or not we were able to sync.
>     */
>    requestSync: function(datasetId, callback) {
>      // Make sure it's a good time to sync.
> +    if ((Services.prefs.getIntPref(PREF_SYNC_WIFI_ONLY) === 1) && !isUsingWifi()) {

My loose-type JS self says we could just leave this as:

if (Services.prefs.getIntPref(PREF_SYNC_WIFI_ONLY) && !isUsingWifi())

But it's more explicit to do it the way you're doing it :)

Comment 14

[:Margaret Leibovic]

Comment on attachment 8386254 [details] [diff] [review]
Part 1: Expose home panels wifi pref in Settings

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +612,5 @@
>              PrefsHelper.setPref(prefName, newValue);
>          }
>  
>          if (preference instanceof ListPreference) {
> +            Log.e(LOGTAG, "AOEU setting new listPref value " + prefName);

Extraneous debug logging?

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Part 2: tests

This adds testing capability for arbitrarily deep preference panels, instead of just top level ones - whereas before there wasn't a way to click on dialogs multiple preference screens down (there weren't any) this makes it work.

Comment 16

[Chenxia Liu [:liuche]]

(In reply to :Margaret Leibovic from comment #13)
> Comment on attachment 8386253 [details] [diff] [review]
> Part 0: Change home.sync.wifiOnly to int pref
> 
> Review of attachment 8386253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/HomeProvider.jsm
> @@ +134,5 @@
> >     * @return boolean Whether or not we were able to sync.
> >     */
> >    requestSync: function(datasetId, callback) {
> >      // Make sure it's a good time to sync.
> > +    if ((Services.prefs.getIntPref(PREF_SYNC_WIFI_ONLY) === 1) && !isUsingWifi()) {
> 
> My loose-type JS self says we could just leave this as:
> 
> if (Services.prefs.getIntPref(PREF_SYNC_WIFI_ONLY) && !isUsingWifi())
> 
> But it's more explicit to do it the way you're doing it :)

Let's leave it with the explicit check, because then we won't have to change it if we add another option for this pref, e.g., if we want to add "Disabled" syncing once pull to refresh is implemented.

> 
> ::: mobile/android/base/preferences/GeckoPreferences.java
> @@ +612,5 @@
> >              PrefsHelper.setPref(prefName, newValue);
> >          }
> >  
> >          if (preference instanceof ListPreference) {
> > +            Log.e(LOGTAG, "AOEU setting new listPref value " + prefName);
> 
> Extraneous debug logging?

Oops, removed!

Comment 17

[:Margaret Leibovic]

Comment on attachment 8386497 [details] [diff] [review]
Part 2: tests

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

Looks reasonable to me. Make a try run!

::: mobile/android/base/tests/testSettingsMenuItems.java
@@ +186,4 @@
>          // Check the items within each category.
> +        String section = null;
> +        for (Entry<String[], List<String[]>> e : settingsMap.entrySet()) {
> +            String[] menuPath = e.getKey();

Nit: final

@@ +241,5 @@
> +
> +            // Navigate back if on a phone. Tablets shouldn't do this because they use headers and fragments.
> +            int menuDepth = menuPath.length;
> +            while (menuDepth > 0) {
> +                if (mDevice.type.equals("phone")) {

It looks like this if statement go around this whole while loop, since this while loop doesn't actually do anything useful if this condition isn't met.

Comment 18

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #16)
> (In reply to :Margaret Leibovic from comment #13)
> > Comment on attachment 8386253 [details] [diff] [review]
> > Part 0: Change home.sync.wifiOnly to int pref
> > 
> > Review of attachment 8386253 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/HomeProvider.jsm
> > @@ +134,5 @@
> > >     * @return boolean Whether or not we were able to sync.
> > >     */
> > >    requestSync: function(datasetId, callback) {
> > >      // Make sure it's a good time to sync.
> > > +    if ((Services.prefs.getIntPref(PREF_SYNC_WIFI_ONLY) === 1) && !isUsingWifi()) {
> > 
> > My loose-type JS self says we could just leave this as:
> > 
> > if (Services.prefs.getIntPref(PREF_SYNC_WIFI_ONLY) && !isUsingWifi())
> > 
> > But it's more explicit to do it the way you're doing it :)
> 
> Let's leave it with the explicit check, because then we won't have to change
> it if we add another option for this pref, e.g., if we want to add
> "Disabled" syncing once pull to refresh is implemented.

If we are planning to implement another state, maybe we should rename this to not be "wifi only". Something more like home.sync.networkLevel? I don't know that I really like that, though, so I would be fine leaving it as-is until we actually have a new use for it.

Comment 19

[Chenxia Liu [:liuche]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=4a7a5ca19c8e

(In reply to :Margaret Leibovic from comment #18)
> > 
> > Let's leave it with the explicit check, because then we won't have to change
> > it if we add another option for this pref, e.g., if we want to add
> > "Disabled" syncing once pull to refresh is implemented.
> 
> If we are planning to implement another state, maybe we should rename this
> to not be "wifi only". Something more like home.sync.networkLevel? I don't
> know that I really like that, though, so I would be fine leaving it as-is
> until we actually have a new use for it.

It's probably easier to rename the pref now rather than later because we'll be exposing it to through the UI.

I believe Ian had wanted to model this after automatic updates; how about something like home.sync.autodownload, home.sync.updates?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/preferences_customize.xml#49

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Part 0: Rename home sync pref, use int instead of bool

Changed pref name to home.sync.updateMode, carrying over r+.

Comment 21

[Chenxia Liu [:liuche]]

Attachment: Part 1: Expose home panels wifi pref in Settings

Unbitrot from pref name change, carrying over r+.

Comment 22

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/cb8398060bce
https://hg.mozilla.org/integration/fx-team/rev/505001868157
https://hg.mozilla.org/integration/fx-team/rev/5c01409eccd5

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cb8398060bce
https://hg.mozilla.org/mozilla-central/rev/505001868157
https://hg.mozilla.org/mozilla-central/rev/5c01409eccd5

Comment 24

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.