Closed Bug 965606 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: Margaret, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

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.
Flags: needinfo?(ibarlow)
Priority: -- → P1
Assignee: nobody → liuche
I might start by adding a new section to the "Home" settings panel:


------------------------------------
CONTENT SETTINGS
------------------------------------
Automatic updates
Enabled / Only over Wifi / Disabled
Flags: needinfo?(ibarlow)
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.
Attached image Screenshot: Standalone wifi option (obsolete) —
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".
Flags: needinfo?(ibarlow)
Moved category to the bottom, where it makes more sense.
Attachment #8383976 - Attachment is obsolete: true
(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.
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.
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
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
Flags: needinfo?(ibarlow)
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.
Attachment #8383983 - Attachment is obsolete: true
Attachment #8383985 - Attachment is obsolete: true
Attachment #8386253 - Flags: review?(margaret.leibovic)
Attachment #8386254 - Flags: review?(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 :)
Attachment #8386253 - Flags: review?(margaret.leibovic) → review+
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?
Attachment #8386254 - Flags: review?(margaret.leibovic) → review+
Attached patch Part 2: testsSplinter Review
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.
Attachment #8386497 - Flags: review?(margaret.leibovic)
(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 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.
Attachment #8386497 - Flags: review?(margaret.leibovic) → review+
(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.
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
Changed pref name to home.sync.updateMode, carrying over r+.
Attachment #8386253 - Attachment is obsolete: true
Attachment #8387935 - Flags: review+
Unbitrot from pref name change, carrying over r+.
Attachment #8386254 - Attachment is obsolete: true
Attachment #8387937 - Flags: review+
Target Milestone: --- → Firefox 30
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.