I think it would be nice to have an option, that makes Firefox Sync operations work only when the phone is connected through a WiFi connection. It would be extra useful for users not on a huge data plan (something not so rare here in EU). Similar features can be found on most Google applications (namely Drive). Moreover, I don't think that it needs great programmatic effort to be implemented.

(In reply to Teo X Greasidis from comment #0) > I think it would be nice to have an option, that makes Firefox Sync > operations work only when the phone is connected through a WiFi connection. > > It would be extra useful for users not on a huge data plan (something not so > rare here in EU). > Similar features can be found on most Google applications (namely Drive). > Moreover, I don't think that it needs great programmatic effort to be > implemented. Teo, this is an excellent suggestion, but I'm not certain this is the kind of thing an individual Android Sync provider is intended to manage. A quick Google suggested http://tasker.wikidot.com/autosyncwifionly and http://www.addictivetips.com/android/enable-android-auto-sync-only-on-power-andor-wifi-connection/ (Aside: why is this not just part of Android?) We're busy enough right now that this won't be a priority, but it's a good idea and an excellent opportunity for a community fix. I've marked that I'll mentor it if you or anyone is interested in addressing it.

Might be also useful to see how the new updater toggles between connections (cc: snorp)

I see this ticket just got some movement. The interesting part of this is providing the UI for the checkbox, which is not really easy or awesome on Android. Contact me (nalexander) with a sketch of your approach, please, and any questions you might have.

(In reply to Nick Alexander :nalexander from comment #3) > I see this ticket just got some movement. The interesting part of this is > providing the UI for the checkbox, which is not really easy or awesome on > Android. Contact me (nalexander) with a sketch of your approach, please, > and any questions you might have. It's really not too bad, I just added some pref stuff for the updater. One of the folks on Finkle's team should be able to help you (or me).

I was thinking of doing this by adding a spinner element, to choose between wifi/mobile data/both

(In reply to Gary from comment #5) > I was thinking of doing this by adding a spinner element, to choose between > wifi/mobile data/both Bear in mind that Android exposes a whole set of different connection types: wimax, Bluetooth, etc. Let's not over-complicate this, but let's also not make it wrong!

Gary, could you confirm that you're still working on this?

Where to start?

Start here: https://github.com/mozilla-services/android-sync You'll need a checkout of android-sync, and to be able to run tests, in order to do the back-end part of this bug, which is to make Sync do the right thing on various network types. (A follow-up will be to do that for other background services, too.) Depending on where the UI element for this lives, you will probably also need to get a checkout of mozilla-central, and build Firefox for Android: https://wiki.mozilla.org/Mobile/Fennec/Android Let us know when you've made it that far! See also: Bug 895991.

shubham: any word on this? Building Fennec can be tricky; you can join #mobile on irc.mozilla.org for help.

I would like to take this up. I have a Fennec build all set up and ready to go. Is the link in rnewman's post still valid? It's from May 2014 and when I clicked on the link for the Android Sync project, the description on the page said "An outdated mirror of services and related code for Firefox for Android. See gecko-dev/mozilla-central."

(In reply to swaroop.rao from comment #12) > I would like to take this up. I have a Fennec build all set up and ready to > go. > > Is the link in rnewman's post still valid? It's from May 2014 and when I > clicked on the link for the Android Sync project, the description on the > page said "An outdated mirror of services and related code for Firefox for > Android. See gecko-dev/mozilla-central." Yeah, the instructions are out of date here: but it's good, things have gotten easier, not harder! Since you have a Fennec build, you're all ready to go. There are several parts to this work: 1) First, add a pref to enable syncing only on WiFi. Since this is Sync-only, let's add it to the FxAccountStatusFragment, maybe above or below the existing checkboxes: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/res/xml/fxaccount_status_prefscreen.xml#73. 2) You'll want to read and write a SharedPreference flag (per-Account) to store the flag. To get the per-Account SharedPreferences, use https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java#356. You'll need to read the value at https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java#215 and you'll need to add a case to update the value around https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java#277. 3) Now, check the value of the SharedPreference flag using the sharedPrefs in the sync adapter here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java#436 You'll want to test the network type -- not quite sure how to do this, you'll need to investigate -- and abort the Sync (with diagnostic warnings) if the network type isn't the right one. If you discover that a simple toggle doesn't make sense (i.e., too many different types of networks), let me know.

Still valuable, still not very difficult to do, still an excellent "next" project.

Awesome! Thanks, Nick/Grisha. I'll start working on it today and reach out to one of you on IRC if I have any questions.

(In reply to swaroop.rao from comment #15) > Awesome! Thanks, Nick/Grisha. I'll start working on it today and reach out > to one of you on IRC if I have any questions. Sounds great, ping me on IRC (grisha) or over email (grisha@mozilla.com) if you run into problems. Also, feel free to post WIP patches for feedback!

Anthony, Ryan, pinging you folks for UX feedback. Anything we should be particularly weary of while adding this pref?

I do limit my offline downloads to wi-fi in music streaming apps, but I have never seen a request for this feature in Firefox Sync. From a UI point of view, I imagine that it is easy to implement (a checkbox in sync prefs) but it would complicate the product somewhat. Would we sync this setting to other Android devices? I think we should check in with Alex before adding a feature. He's the product manager!

(In reply to Ryan Feeley [:rfeeley] from comment #18) > From a UI point of view, I imagine that it is easy to implement (a checkbox > in sync prefs) but it would complicate the product somewhat. Would we sync > this setting to other Android devices? We're looking at this through a lens of a community contribution, so let's not over-complicate things. Providing a local-only option is likely to be "good enough" and will add value, and we can easily measure its use to see if it's a valuable addition to consider for other platforms - specifically, iOS, since I don't think this has a lot of meaning on desktop.

The obvious part of the implementation: - Add checkbox UI to Sync Preferences - Default this pref to "allow syncing over cell", since that's our current behaviour and we need to carry it over - Consider persisting value of this checkbox to SharedPreferences accessed via AndroidFxAccount.getSyncPrefs, as you will need to read this value during a sync - Early on in onPerformSync, check the network status, and make the obvious decisions - don't sync if you're not in the correct network state. You'll need to notify syncDelegate via rejectSync call or similar. The non-obvious part: - what to do _during_ a sync? Imagine device is on wi-fi, and user set the checkbox to "sync only on wi-fi". Device starts syncing, but then it switches to a cell network. Provided that sync doesn't fail due to a network problem as the switch-over happens, should it continue to sync? Should it stop? - Look at GeckoNetworkManager for inspiration for maintaining an accurate representation of the current network state. Things get a bit tricky there. - We need to be mindful of user's intent here. It's likely that they're using this feature to minimize bandwidth usage over cellular networks. On one hand, we're going >80% of the way there if we ignore these considerations. On another hand, if a user wouldn't even turn on sync knowing that we _might_ sync over a cell network, we're knowingly putting them in a tight spot by ignoring these edge cases - Overall, I think it's OK for the first implementation to be very simple and simply concern itself with the very beginning of a sync. We're going to hit most of the value for majority of the users this way. - However, it's worth adding a note in the UI warning users that we _might_ partially sync over cellular in rare circumstances when this feature is enabled. I think a confirmation dialog when user interacts with the checkbox should be sufficient.

(In reply to :Grisha Kruglov from comment #20) > The obvious part of the implementation: > - Add checkbox UI to Sync Preferences > - Default this pref to "allow syncing over cell", since that's our current > behaviour and we need to carry it over > - Consider persisting value of this checkbox to SharedPreferences accessed > via AndroidFxAccount.getSyncPrefs, as you will need to read this value > during a sync > - Early on in onPerformSync, check the network status, and make the obvious > decisions - don't sync if you're not in the correct network state. You'll > need to notify syncDelegate via rejectSync call or similar. > > The non-obvious part: > - what to do _during_ a sync? Imagine device is on wi-fi, and user set the > checkbox to "sync only on wi-fi". Device starts syncing, but then it > switches to a cell network. Provided that sync doesn't fail due to a network > problem as the switch-over happens, should it continue to sync? Should it > stop? iOS has a "green light" concept here: a thunk that's threaded around through the sync to allow us to stop gracefully when backgrounded or enough time has elapsed. It's probably reasonable to take a similar approach here: to wrap up the current download (for two-phase batching) or engine (for non-batching) as appropriate. > It's likely that they're > using this feature to minimize bandwidth usage over cellular networks. Think about metered connections instead of cellular. > - Overall, I think it's OK for the first implementation to be very simple > and simply concern itself with the very beginning of a sync. We're going to > hit most of the value for majority of the users this way. SGTM.

After syncing up with rfeeley, here are some additional things to keep in mind. First, where to place the new pref. We already have Settings->Advanced->Data Saver section, and so it makes sense to place the new toggle preference there. You'd need to show it conditionally, only if the user already has a Firefox Account. This should help: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java#36 Second, we essentially have two types of syncs - periodic background syncs, and user-initiated syncs. This pref should only control the background syncs. We want to allow people to tap "Sync Now" button and perform other actions that depend on Sync without restricting them in any way. User-initiated actions that will result in a sync: - tapping Sync Now - pull-to-refreshing a Synced Devices part of the history panel - sending a tab - incoming push messages that tell us to sync 'clients' collection - right now this will be just receiving a tab sent from another device - renaming your device in sync prefs, this also kicks off a sync All of these various user-initiated go through FirefoxAccounts#requestImmediateSync [0]. You'd want to either make use of the SYNC_EXTRAS_EXPEDITED flag, or introduce a new one, in order to ignore the "wi-fi only" pref. [0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java?q=FirefoxAccounts.java&redirect_type=direct#151

In addition to what Ryan has been saying and getting Product input here, I think messaging is really going need to be clarified too. Keeping this in the Sync prefs make the most sense to me, instead of having it in the Product. Michelle, have you been looking at that part of FxA? What's your suggestion for copy here? Thanks!

to be clear, are we thinking this would be under Manage account > About:accounts?action=manage ?

(In reply to Anthony Lam (:antlam) from comment #24) > to be clear, are we thinking this would be under Manage account > > About:accounts?action=manage ? This is a property of the device, not the account.

(In reply to Anthony Lam (:antlam) from comment #23) > Keeping this in the Sync prefs make the most sense to me, instead of having > it in the Product. Michelle, have you been looking at that part of FxA? > What's your suggestion for copy here? We'll definitely need to figure out the wording here. What do you mean by "in the Product"? As Richard said, this is a device property, and affects only the current device, and will live in the native prefs. We already have "data saver" options in Settings-Advanced, to me this seems like an obvious choice. That _might_ somewhat dictate the wording, if we'll want to be consistent with other data saver options. There's also a chance people will be looking for this pref under Sync preferences, of course.

Hi Swaroop - any updates on this work? Let me know if you're blocked on anything or have any questions. I'm available either here, on IRC (grisha) or over email (grisha@mozilla.com).

(In reply to :Grisha Kruglov from comment #27) > Hi Swaroop - any updates on this work? > > Let me know if you're blocked on anything or have any questions. I'm > available either here, on IRC (grisha) or over email (grisha@mozilla.com). Looks like Saroop is busy with other things. So, if it's not a problem I'd like to work on this bug. Would you please assign me this and mentor me on it?

Hi mehdisolamannejad, Between Comment 20 - Comment 22 you should have enough to get started. Given the scope and potential impact here, it's safe to say that this is more of an advanced bug, all things considered. With that in mind, read through the comments above, make sure you understand what needs to be done - feel free to ask questions here if you don't understand something, or reach out to me on IRC - and submit some "work in progress" patches for feedback. Thanks!

Fair enough. Before submitting anything, I have a few questions. 1. What key should I use for the preference? Does "sync.use_cellular" sound good to you? 2. Do we already have a method for checking WiFi availability? I feel this is something that can be generally useful in the future so having it as a method would be cool. If we don't have such a method, where should I implement it? 3. If I understand correctly there are some Android devices that can use an Ethernet connection. Should I also check for Ethernet availability or do you think checking WiFi is good enough? 4. While I won't be doing anything about aborting a sync in my initial submission, I think it'd be worth it to work on it. I think it is possible to implement listener for WiFi disconnection, but where should I implement it and how should I abort an ongoing sync? Can an aborted sync potentially damage data? What precautions are we supposed to take? If you think we should do something about this, I'd like to get your advice on how to do it properly. 5. This one might be a dumb question, but how should I test my code locally before submitting anything? Is there some way to trigger background syncing?

Grisha, I'm still interested in fixing this. Can you please help me by answering my questions?

(In reply to mehdisolamannejad from comment #30) > 3. If I understand correctly there are some Android devices that can use an > Ethernet connection. Should I also check for Ethernet availability or do you > think checking WiFi is good enough? You should consider using the 'metered' hint for this, instead of just checking for wifi versus cellular, which will allow us to avoid syncing when you tether to a friend's cellular connection over wifi. Good discussion here: https://stackoverflow.com/questions/23877476/android-why-connectivitymanager-isactivenetworkmetered-always-returning-true > 5. This one might be a dumb question, but how should I test my code locally > before submitting anything? Is there some way to trigger background syncing? The easiest thing to do is to drop the syncing intervals and rate limiting values: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java#79 https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSchedulePolicy.java#21

(In reply to mehdisolamannejad from comment #30) > 1. What key should I use for the preference? Does "sync.use_cellular" sound > good to you? Let's go with something like sync.use_metered. > 2. Do we already have a method for checking WiFi availability? I feel this > is something that can be generally useful in the future so having it as a > method would be cool. If we don't have such a method, where should I > implement it? We already have GeckoNetworkManager and NetworkUtils, which largely do what you want: https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/NetworkUtils.java#76 However, due to how background services code is separated out from the rest of the project, you won't be able to use these functions easily. You need a very simple check, so it's acceptable to duplicate it in the services package. Once you have a largely working work-in-progress patch, we can discuss this in more detail. I agree with Richard that we should think of this as "metered vs non-metered" connections. Luckily, isActiveNetworkMetered API is 16+, and recently we switched to only support 16+ devices. Let's see how far this gets us. https://developer.android.com/reference/android/net/ConnectivityManager.html#isActiveNetworkMetered() > 4. While I won't be doing anything about aborting a sync in my initial > submission, I think it'd be worth it to work on it. I think it is possible > to implement listener for WiFi disconnection, but where should I implement > it and how should I abort an ongoing sync? Can an aborted sync potentially > damage data? What precautions are we supposed to take? If you think we > should do something about this, I'd like to get your advice on how to do it > properly. Again, let's not step into that bog, and deal with this separately in a different bug once the core functionality is in place. For one, our experience with tracking network state (up/down) reliably across a wide range of devices, OS versions, custom ROMs, and features such as battery savers, etc suggests that it's trickier than you'd hope. Interrupting a sync should also be done with a good understanding of underlying data flow, and I highly suggest avoiding that work if you don't strictly need to do it. > 5. This one might be a dumb question, but how should I test my code locally > before submitting anything? Is there some way to trigger background syncing? Richard's suggestion in Comment 32 is a good one.

Thank you both. Got 2 more questions if you don't mind me asking. 1. How can I define a new string resource? When I want to edit strings.xml the IDE warns me that this file is build-generated and editing it is pointless. 2. How can I access the SharedPreferences that reference the 'Advanced' settings? Using getContext().getSharedPreferences("android.not_a_preference.advanced_screen", Context.MODE_PRIVATE) or PreferenceManager.getDefaultSharedPreferences(getContext()) did not work?

Not sure if Bugzilla is breaking with unicode comments, but here goes. As a checkbox label, I think we should go with: ☑ Background sync using cellular data

(In reply to mehdisolamannejad from comment #34) > Thank you both. Thank you for helping out! In the future, I recommend using "need more information from..." flag (NI) below the message box, and tagging someone. This way we get a person will get a notification, and will see a pending request for information. Otherwise, regular comments are easy to miss in the bugzilla email traffic. > Got 2 more questions if you don't mind me asking. > > 1. How can I define a new string resource? When I want to edit strings.xml > the IDE warns me that this file is build-generated and editing it is > pointless. You'll need to add new strings to: android_strints.dtd and to strings.xml.in files. For an example, take a look how 'remote_tabs_last_synced' string is defined: - https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#759 (notice the localization notes, provide a description for your string to make it easier to translate) - https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/strings.xml.in#595 After re-compiling, you'll be able to use the new string in code or xml files. > 2. How can I access the SharedPreferences that reference the 'Advanced' > settings? Using > getContext().getSharedPreferences("android.not_a_preference.advanced_screen", > Context.MODE_PRIVATE) or > PreferenceManager.getDefaultSharedPreferences(getContext()) did not work? You can figure this out by taking a look at how one of the existing prefs is used. Unfortunately, that logic is somehow convoluted by the fact that our front-end (java) and back-end (gecko) share some of the preferences, or rather - gecko prefs are set via the front-end. In this case, look for a front-end pref, and trace its usage. For example, tracing through how "Custom Tabs" switch pref is used leads you to its consumers doing something like: GeckoSharedPrefs.forApp(this).getBoolean(GeckoPreferences.PREFS_CUSTOM_TABS, false);

Thanks! I'm back online on September 4th, I'll take a look then.

Ryan, given that we're checking for "metered" connections explicitly - which might include certain wifi endpoints, not just cellular - perhaps we should revise that checkbox's wording? Do you think something like "Allow background syncing on metered connections" would be decipherable to an average user? I wonder if that might be too long for that screen, actually. Something like "Show web fonts" is much shorter and already takes up a third of the horizontal space. Also, do you think we should have a subtext for this settings? If so, what should it be? For reference, here is what this settings pane looks like now.

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review181468 This is a good first stab at this, but as always with syncing, "devil is in the detail". Please see my comments below on what else needs to be done. ::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:176 (Diff revision 1) > public static final String PREFS_DEFAULT_BROWSER = NON_PREF_PREFIX + "default_browser.link"; > public static final String PREFS_SYSTEM_FONT_SIZE = NON_PREF_PREFIX + "font.size.use_system_font_size"; > public static final String PREFS_SET_AS_HOMEPAGE = NON_PREF_PREFIX + "distribution.set_as_homepage"; > public static final String PREFS_DIST_HOMEPAGE = NON_PREF_PREFIX + "distribution.homepage"; > public static final String PREFS_DIST_HOMEPAGE_NAME = NON_PREF_PREFIX + "distribution.homepage.name"; > + public static final String PREFS_SYNC_METERED = "sync.use_metered"; I think `sync.allow_metered` is better name. ::: mobile/android/base/locales/en-US/android_strings.dtd:278 (Diff revision 1) > <!ENTITY pref_show_web_fonts_summary2 "Download remote fonts when loading a page"> > > +<!ENTITY pref_sync_use_metered "Allow sync over metered connections"> > +<!-- Localization note: Only affects background syncing, user initiated > + syncs will still be done regardless of the connection --> > +<!ENTITY pref_sync_use_metered_summary "Background syncing will be done over metered connections"> I have my doubts about wording of these fields. Let's wait for Ryan's feedback. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:503 (Diff revision 1) > * token implementation. > */ > @Override > public void onPerformSync(final Account account, final Bundle extras, final String authority, ContentProviderClient provider, final SyncResult syncResult) { > + > + // Check whether the sync is user-initiated or not Here and elsewhere: please use full sentences in comments, with correct grammar and punctuation. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:504 (Diff revision 1) > */ > @Override > public void onPerformSync(final Account account, final Bundle extras, final String authority, ContentProviderClient provider, final SyncResult syncResult) { > + > + // Check whether the sync is user-initiated or not > + if (!extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false)) { This needs to be carefully thought out. SYNC_EXTRAS_EXPEDITED flag is set whenever we go through `requestImmediateSync`. Users of this function varied, and should behave differently in light of the new user preference. In detail: - follow-up syncs. Follow-up syncs are requested if a sync failed due to hitting a sync deadline (taking too long to complete), or because we saw a 412 error (concurrent modification of server data by another client). The scenario we might hit is: one sync happened over a non-metered connection, sometime mid-sync user switched over to a metered connection, while sync also partially failed and requested a follow up. We have a choice: either ignore the user preference and sync anyway, or don't allow follow-up syncs over metered connections. I think the right decision is to respect user's choice. Actionable change here is to create a differentiation between "expedited syncs" (current behaviour of requestImmediateSync) and "syncs which ignore user settings". We should use the former for follow-up syncs, and the latter for syncing which supports product features (send tab) and user-triggered syncing. We can use SYNC_EXTRAS_MANUAL flag for this new sync request function. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java?q=path%3AFirefoxAccounts.java&redirect_type=single#151 - "account verified" push messages. When user first signs-in, they might be asked to verify their sign-in on the new device (via a 2fa-style email at the moment). Once sign-in is verified, device receives a push message which currently triggers an immediate sync. I think this should also respect the "allow metered syncing" pref, and so should use the "expedited sync" mechanism above. Actionable item here is to ensure "account verified" syncs don't happen if user disabled metered syncing. - "collection changed" push messages. Currently, this is used to support "Send Tab" feature. Tabs are "sent" as commands to clients, via the "clients" collection. To ensure speedy delivery, we send a push message to the recepient device telling it to sync its clients collection. Since this push message supports a core product feature, we should ignore the pref if asked to sync clients collection specifically. However, in theory this push message might be telling us to sync any other collection as well (history, bookmarks...). A whitelist of collections which are allowed to forgo a user settings seems like a good fit. However, we need to acknowledge here that such whitelist will force us to synchronize any feature deployments that involve push-based syncing with a Fennec release cycle, since this whitelist will need to be adjusted. Actionable item here is to create a whitelist of which collections we're allowed to sync while ignoring user settings after receiving a "sync collection" push message. - Device name changes. This needs to sync immediately. It will trigger a "clients" collection sync, and should ignore user preference. Use the "sync ignoring user settings" approach mentioned above. - ForcedClientCollectionSync runnable. We use it to populate the 'fxadeviceid' column in the clients table. It requests a sync of the "clients" collection after an application upgrade. This also supports core functionality (correct behaviour of client lists on different devices), and so we should ignore user preferences here. Use the "sync ignoring user settings" approach mentioned above. - Send Tab. Sending a tab requests an immediate sync of "clients" and "tabs" collections. It should also ignore the user pref and use the "sync ignoring user settings" approach mentioned above. - Refreshing the "Synced Devices" section of the History home panel via pull-to-refresh. This should be considered a manual user sync, so ignore the user preference as well. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:508 (Diff revision 1) > + // Check whether the sync is user-initiated or not > + if (!extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false)) { > + // If it's not user-initiated, we should check if we are allowed to sync on metered connections > + final boolean isMeteredAllowed = GeckoSharedPrefs.forApp(getContext()).getBoolean(GeckoPreferences.PREFS_SYNC_METERED, true); > + // Should check if the device is on a metered connection > + ConnectivityManager manager = (ConnectivityManager) getContext().getSystemService(Context.CONNECTIVITY_SERVICE); Here and elsewhere: `final` everywhere you can. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:513 (Diff revision 1) > + ConnectivityManager manager = (ConnectivityManager) getContext().getSystemService(Context.CONNECTIVITY_SERVICE); > + final boolean isMetered = manager.isActiveNetworkMetered(); > + > + // If the connection is metered and syncing over metered connections are > + // not permitted, we should bail > + if(!isMeteredAllowed && isMetered) { Missing space, `if (`. This _would_ have been caught be checkstyle, but unfortunately we don't currently run it for *services* code. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:519 (Diff revision 1) > + final AndroidFxAccount fxAccount = new AndroidFxAccount(getContext(), account); > + final BlockingQueue<Result> latch = new LinkedBlockingQueue<>(1); > + Collection<String> knownStageNames = SyncConfiguration.validEngineNames(); > + Collection<String> stageNamesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras); > + final SyncDelegate syncDelegate = new SyncDelegate(latch, syncResult, fxAccount, stageNamesToSync); > + syncDelegate.rejectSync(); Just move this logic lower, to where we do other `shouldPerformSync` checks. That will let you kill off most of this code and just do `syncDelegate.rejectSync()`.

Thank you for reviewing my code. So... to my understanding only follow-up syncs, "account verified" messages and some non-white-listed collections should be considered "background syncing" right? If so, 1. What collections should be white-listed? 2. Where does "account verified" push happen? Can it cause later bugs if we ignore it on metered connections? Or does it simply get postponed?

> So... to my understanding only follow-up syncs, "account verified" messages and some non-white-listed collections should be considered "background syncing" right? The whitelist applies only to "collection changed" push messages. However, while skimming that code I've misread it: we already have a de-facto whitelist in place. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java?q=path%3AFxAccountPushHandler&redirect_type=single#80 So for these push messages, we just need to ensure that a sync of a "clients" collection will happen regardless of the user preference. See my proposed flow below. > 2. Where does "account verified" push happen? Can it cause later bugs if we ignore it on metered connections? Or does it simply get postponed? IIRC, "account verified" push message is sent after a sign-in/sign-up attempt was confirmed. In order for features such as device lists and Send Tab to work correctly, on different devices/versions, we need to ensure that our local client record is uploaded to the clients collection. This would normally happen during a regular "first sync", so we need to ensure we still do this if the user disabled "metered syncing" prior to signing-in. However the "first sync" is triggered, it needs to _at least_ sync the clients collection. Let's follow this basic flow: - whenever we request an immediate sync which must be honored regardless of the user pref, ensure that we set the SYNC_EXTRAS_MANUAL flag (that includes tapping "sync now" button, "collection changed" push messages, sending a tab, refreshing the "synced devices" ui, etc. - in onPerformSync, determine if we're in a firstSync situation (never synced before, via fxAccount.getLastSyncedTimestamp()) - if metered syncing isn't allowed & we're on metered connection: -- if SYNC_EXTRAS_MANUAL is set, continue syncing as normal. otherwise: -- if we're attempting firstSync, sync the "clients" collection -- otherwise, reject a sync This lets us keep changes outside of 'onPerformSync' to the minimum, and concentrate all of the logic in that function. Another thing to investigate is: can we determine that a user triggered a sync from the Accounts section of Android Settings? I think that should be considered a manual sync. There's a good chance a flag will be set that we can use to figure this out - you can set a breakpoint in onPerformSync, trigger a sync from the system settings screen, and see what that sync request looks like.

Everything seems to be working as expected. Please do a review and see if I've missed and/or misunderstood anything.

Nick, I've just stumbled into Bug 1182206 where you removed SYNC_EXTRAS_MANUAL, which I propose to use in Comment 42. The stated reason at the time is: "The problem is that SYNC_EXTRAS_IGNORE_SETTINGS syncs even disabled (unchecked in Android Settings) SyncAdapters. We shouldn't do that." It's not clear to me that obeying Android Settings is desirable in case of a forced syncs, for two reasons: 1) user is clearly indicating their intent to sync by tapping "sync now" button. Granted, they could have also stated their desire to disable syncing in Android Settings, but I do think that a clear in-product action should override a system setting in this case. 1.1) our in-product UI for this state sucks. There is no indication of what's going on. 2) this is probably the more important point: our usage of Sync has expanded. It's not just "get data back and forth" any more. With the introduction of push-based "Send Tabs", Sync is now also supporting a core product feature which isn't obviously tied to it from user's point of view. 2.1) it's just not how most android apps work at this point in time. Most aren't using the account system, and expecting users to understand its intricacies, how it ties-in with in-app actions, and subtleties of how we implemented features is likely to cause confusion. What do you think?

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review182466 This is looking good, sans bunch of nits and some cleanup - unless Nick has any objections to our use of SYNC_EXTRAS_IGNORE_SETTINGS. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java:157 (Diff revision 2) > + requestImmediateSync(account, stagesToSync, stagesToSkip, true); > + } > + > + public static void requestImmediateSync(final Account account, String[] stagesToSync, String[] stagesToSkip, boolean overrideMeteredPrefs) { > final Bundle syncOptions = new Bundle(); > + if (overrideMeteredPrefs) House style: always use {}, even for one-line if statements. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java:159 (Diff revision 2) > + > + public static void requestImmediateSync(final Account account, String[] stagesToSync, String[] stagesToSkip, boolean overrideMeteredPrefs) { > final Bundle syncOptions = new Bundle(); > + if (overrideMeteredPrefs) > + syncOptions.putBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, true); > syncOptions.putBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, true); SYNC_EXTRAS_MANUAL = SYNC_EXTRAS_IGNORE_SETTINGS | SYNC_EXTRAS_IGNORE_BACKOFF, so there's some redundancy here. But, it's not clear to me what the behaviour will be if we omit SYNC_EXTRAS_IGNORE_BACKOFF and just use SYNC_EXTRAS_MANUAL. Will we receive SYNC_EXTRAS_IGNORE_BACKOFF flag in extras in onPerformSync? Perhaps we should be more explicit here, and instead of SYNC_EXTRAS_MANUAL just set SYNC_EXTRAS_IGNORE_SETTINGS. That seems like the most straightforward thing, and carries our intent better as well. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:565 (Diff revision 2) > * > * @param stagesToSync stage names to sync; can be null to sync <b>all</b> known stages. > * @param stagesToSkip stage names to skip; can be null to skip <b>no</b> known stages. > */ > public void requestImmediateSync(String[] stagesToSync, String[] stagesToSkip) { > - FirefoxAccounts.requestImmediateSync(getAndroidAccount(), stagesToSync, stagesToSkip); > + requestImmediateSync(stagesToSync, stagesToSkip, true); I'd prefer consumers of this function to make an explicit choice, as opposed to inferring what should happen. And so, I think we should only have one full-signature method here. Two reasons: 1) features requesting an immediate sync are better suited to determine if their request should ignore user preference 2) in order to avoid regressions from this patch, it would be useful to explicitly go through all of the uses ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:575 (Diff revision 2) > + * > + * @param stagesToSync stage names to sync; can be null to sync <b>all</b> known stages. > + * @param stagesToSkip stage names to skip; can be null to skip <b>no</b> known stages. > + * @param overrideMeteredPrefs whether we should check preferences for syncing over metered connections. > + */ > + public void requestImmediateSync(String[] stagesToSync, String[] stagesToSkip, boolean overrideMeteredPrefs) { `ignoreSettings` is a better name, since it's not _just_ the metered settings will be ignoring, but also system settings (see https://bugzilla.mozilla.org/show_bug.cgi?id=802749#c45) ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:504 (Diff revision 2) > */ > @Override > public void onPerformSync(final Account account, final Bundle extras, final String authority, ContentProviderClient provider, final SyncResult syncResult) { > + > + // This flag is used to conclude whether we should ignore syncing > + // based on their preference for syncing over metered connections "based on user preference". Also, missing period at the end of the sentence. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:505 (Diff revision 2) > @Override > public void onPerformSync(final Account account, final Bundle extras, final String authority, ContentProviderClient provider, final SyncResult syncResult) { > + > + // This flag is used to conclude whether we should ignore syncing > + // based on their preference for syncing over metered connections > + boolean shouldReject = false; A better name is something like `shouldRejectSyncViaSettings`, since we might ignore this choice later on. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:508 (Diff revision 2) > + // This flag is used to conclude whether we should ignore syncing > + // based on their preference for syncing over metered connections > + boolean shouldReject = false; > + // Check whether the sync is user-initiated or not and if we should > + // ignore user preference for syncing over metered connections. > + if (!extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false) I don't think SYNC_EXTRAS_EXPEDITED should play into this. We're particularly interested in SYNC_EXTRAS_IGNORE_SETTINGS, so let's just check for that flag here, and set it elsewhere. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:513 (Diff revision 2) > + if (!extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false) > + && !extras.getBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, false)) { > + // If it's not user-initiated, we should check if we are allowed to sync on metered connections. > + final boolean isMeteredAllowed = GeckoSharedPrefs.forApp(getContext()).getBoolean(GeckoPreferences.PREFS_SYNC_METERED, true); > + // Check if the device is on a metered connection or not. > + final ConnectivityManager manager = (ConnectivityManager) getContext().getSystemService(Context.CONNECTIVITY_SERVICE); This _could_ return null, but it's probably safe to assume CONNECTIVITY_SERVICE is always going to be present... ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:515 (Diff revision 2) > + // If it's not user-initiated, we should check if we are allowed to sync on metered connections. > + final boolean isMeteredAllowed = GeckoSharedPrefs.forApp(getContext()).getBoolean(GeckoPreferences.PREFS_SYNC_METERED, true); > + // Check if the device is on a metered connection or not. > + final ConnectivityManager manager = (ConnectivityManager) getContext().getSystemService(Context.CONNECTIVITY_SERVICE); > + final boolean isMetered = manager.isActiveNetworkMetered(); > + // If the connection is metered and syncing over metered connections are is not permitted ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:570 (Diff revision 2) > Collection<String> knownStageNames = SyncConfiguration.validEngineNames(); > Collection<String> stageNamesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras); > > + // If syncing should be rejected due to metered connection preferences > + // and we are doing the first sync ever, we should at least sync the > + // 'clients' stage to ensure the device shows up in the list. Add a reference to Bug 802749, so that it's easier to find relevant discussions in the future. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:570 (Diff revision 2) > Collection<String> knownStageNames = SyncConfiguration.validEngineNames(); > Collection<String> stageNamesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras); > > + // If syncing should be rejected due to metered connection preferences > + // and we are doing the first sync ever, we should at least sync the > + // 'clients' stage to ensure the device shows up in the list. "to ensure we upload our local client record." ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:570 (Diff revision 2) > Collection<String> knownStageNames = SyncConfiguration.validEngineNames(); > Collection<String> stageNamesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras); > > + // If syncing should be rejected due to metered connection preferences > + // and we are doing the first sync ever, we should at least sync the > + // 'clients' stage to ensure the device shows up in the list. s/stage/collection ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:571 (Diff revision 2) > Collection<String> stageNamesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras); > > + // If syncing should be rejected due to metered connection preferences > + // and we are doing the first sync ever, we should at least sync the > + // 'clients' stage to ensure the device shows up in the list. > + if (shouldReject && fxAccount.getLastSyncedTimestamp() == -1){ Check against `fxAccount.neverSynced()`, instead of relying on a constant. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:579 (Diff revision 2) > + } > + > final SyncDelegate syncDelegate = new SyncDelegate(latch, syncResult, fxAccount, stageNamesToSync); > Result offeredResult = null; > > + if (shouldReject && fxAccount.getLastSyncedTimestamp() != -1) { Something like this is cleaner: ``` final SyncDelegate syncDelegate = new SyncDelegate(latch, syncResult, fxAccount, stageNamesToSync); if (shouldRejectSyncViaSettings) { // We already synced at some point, so fully respect user preference and bail out. if (!fxAccount.neverSynced()) { syncDelegate.rejectSync(); return; } // This is our first sync, and user preferences indicate that we should skip this sync. // We depend on our local 'client' record to be available to other clients, and so // we partially ignore this user preference and just do a limited sync of one collection. // See Bug 802749. stageNamesToSync.clear(); stageNamesToSync.add("clients"); } ```

(In reply to :Grisha Kruglov from comment #46) > Something like this is cleaner: > > ``` > final SyncDelegate syncDelegate = new SyncDelegate(latch, syncResult, > fxAccount, stageNamesToSync); > > if (shouldRejectSyncViaSettings) { > // We already synced at some point, so fully respect user preference and > bail out. > if (!fxAccount.neverSynced()) { > syncDelegate.rejectSync(); > return; > } > // This is our first sync, and user preferences indicate that we should > skip this sync. > // We depend on our local 'client' record to be available to other > clients, and so > // we partially ignore this user preference and just do a limited sync of > one collection. > // See Bug 802749. > stageNamesToSync.clear(); > stageNamesToSync.add("clients"); > } > ``` While I agree with you, please note that stageNamesToSync in SyncDelegate is unmodifiable and calling add() or clear() after constructing an instance would throw an exception. So all in all, I'd say both snippets would be equally clean. I'm dropping this, feel free to re-open it if I'm mistaken or you simply disagree.

(In reply to :Grisha Kruglov from comment #45) > Nick, I've just stumbled into Bug 1182206 where you removed > SYNC_EXTRAS_MANUAL, which I propose to use in Comment 42. The stated reason > at the time is: "The problem is that SYNC_EXTRAS_IGNORE_SETTINGS syncs even > disabled (unchecked in Android Settings) SyncAdapters. We shouldn't do > that." At one time we wanted to do the "right thing" and split the SyncAdapter into a bunch of different adapters, one for each the different data type. Then users could turn on and off the Synced data types in the "natural" place. However, we didn't do this: mostly because product wanted (and still wants, AFAIK) to have the set of Synced data types be global, not local to each device. In addition, I don't think that is the trend in Android-land for anybody _except_ Google (which do use many SyncAdapter instances). > It's not clear to me that obeying Android Settings is desirable in case of a > forced syncs, for two reasons: > > 1) user is clearly indicating their intent to sync by tapping "sync now" > button. Granted, they could have also stated their desire to disable syncing > in Android Settings, but I do think that a clear in-product action should > override a system setting in this case. > 1.1) our in-product UI for this state sucks. There is no indication of > what's going on. > > 2) this is probably the more important point: our usage of Sync has > expanded. It's not just "get data back and forth" any more. With the > introduction of push-based "Send Tabs", Sync is now also supporting a core > product feature which isn't obviously tied to it from user's point of view. > 2.1) it's just not how most android apps work at this point in time. Most > aren't using the account system, and expecting users to understand its > intricacies, how it ties-in with in-app actions, and subtleties of how we > implemented features is likely to cause confusion. I think 2.1) is not that significant; "nobody" disables Sync using the check box in Android Settings (especially 'cuz the Android UI hid it ages ago). IK think 2) is really significant. If we had infinite resources, we'd decouple FxA/clients from Sync/SyncAdapters entirely, so that Send Tab would _be_ independent of Sync, and we wouldn't have this conflict. As it stands, I'm fine to violate the Android expectation -- for most users this will "do the right thing" for the product, and for power users, they can find this ticket... hahaha.

(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #49) > However, we didn't do this: mostly because product wanted (and still wants, > AFAIK) to have the set of Synced data types be global, not local to each > device. FWIW, I think we've back-tracked from that - bug 1215415 is to allow that selection per-device, but it's trickier than it seems.

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review185168 This looks good, thank you! I don't think the strings are quite correct though, and I have my doubts about pref placement - yes, we have "Data Saver", but it really feels like this should live under Sync Preferences... I'm happy to land this in Data Saver for now though, and we can always move these prefs afterwards. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:715 (Diff revision 3) > Logger.warn(LOG_TAG, "Got exception setting last synced time; ignoring.", e); > } > } > > public long getLastSyncedTimestamp() { > final long neverSynced = -1L; Can you move this into a constant, and use the new constant in `neverSynced`? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:506 (Diff revision 3) > public void onPerformSync(final Account account, final Bundle extras, final String authority, ContentProviderClient provider, final SyncResult syncResult) { > + > + // This flag is used to conclude whether we should ignore syncing > + // based on user preference for syncing over metered connections. > + boolean shouldRejectSyncViaSettings = false; > + // Check whether the sync is user-initiated or not and if we should nit: The comments here need some editing. We're really just checking if we've been asked to ignore settings; we don't explicitly check if sync is user-initiated or not.

Thank you for the R+ Grisha. Before I submit the last patch and fix the remaining open issues, can you please suggest what should I use for the strings so I can fix everything in one submission and add checkin-needed?

(In reply to :Grisha Kruglov from comment #51) I have my doubts about pref placement - yes, we have "Data > Saver", but it really feels like this should live under Sync Preferences... I mean, we can put it in both categories. Is there some rule strictly forbidding duplicate entries in preferences?

I'm not sure if metered connections is understood by most Android users, but it looks like the second line can provide more detail. How does this work? Legend - Background sync using metered connections Label - Allow usage of cellular data and metered wi-fi to sync in the background

Ryan, we'll need to shorten the title of that pref. See screenshot - it doesn't fit. Perhaps "Sync on metered connections"? That fits, and we mention "background" in the summary below.

Will "Sync over metered connections" fit?

OK, let's go with these strings: title=Sync over metered connections summary=Allow usage of cellular data and metered wi-fi to sync in the background Discussed this with Ryan, and ISTM that this new pref is best placed under Sync Preferences. It's only relevant if user is signed-in into Sync, and likely to be more discoverable there as well. mehdisolamannejad, could you please change the labels, and move the pref to the Sync Preferences screen? Likely, you'll need to manually toggle the shared pref via listeners in FxAccountStatusFragment. See attached image for placement. (screenshot is a WIP from Bug 1398283).

Given that we're very close to the merge date (57 goes beta on September 21st), I propose we land this after the merge and aim to have this functionality out with Firefox 58. A few specific reasons: - I won't have a lot of bandwidth during the beta cycle to address any issues that are likely to come up and will need an uplift - We need to follow-up with a telemetry probe to measure how much this is used. See Bug 1401380 - It would be good to let this "bake" on nightly for a bit: I have a general distrust of how Android's sync APIs behave across API levels, so it would be good to have a broad set of users exposed to this - We need to give QA some heads-up and request that this is goes through regular QA verification. It's too late for that now

(In reply to :Grisha Kruglov from comment #58) > - It would be good to let this "bake" on nightly for a bit: I have a general > distrust of how Android's sync APIs behave across API levels, so it would be > good to have a broad set of users exposed to this Specifically, I'm mostly concerned about quality of the "is this a metered connection" network API.

Grisha, should we land this patch now?

All I know is currently the user turns off all search hints, even turns off images, and then browses some itty bitty web page, but still is in danger of suddenly being smacked with one Megabyte of data usage just to browse some 3 KB webpage, especially if he hasn't used the browser for a while. Therefore please cordon off all the possible ways this could happen. Thanks.

(In reply to mehdisolamannejad from comment #61) > Grisha, should we land this patch now? Yeah, let's do it. First you'll need to update the patch, since most of it already landed in Bug 1401336, move the pref over to Sync Prefs and update the strings.

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review188008 Clearing the flag, since most of this needs to change - pref position, strings, and to account for Bug 1401336.

(In reply to :Grisha Kruglov from comment #65) > Comment on attachment 8903451 [details] > Bug 802749 - Make background sync over metered connections optional. Kruglov > > https://reviewboard.mozilla.org/r/175296/#review188008 > > Clearing the flag, since most of this needs to change - pref position, > strings, and to account for Bug 1401336. Ok. Open issues and I'll fix them. Thank you.

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review188114 My apologies, I didn't notice the updated version before clearing the flag; anyway, I've left some comments. ::: mobile/android/base/locales/en-US/android_strings.dtd:577 (Diff revision 4) > with the page title defined in aboutHome.dtd --> > <!ENTITY home_title "&brandShortName; Home"> > <!ENTITY home_history_title "History"> > <!ENTITY home_synced_devices_smartfolder "Synced devices"> > <!ENTITY home_synced_devices_number "&formatD; devices"> > +<!ENTITY pref_sync_use_metered "Allow sync over metered connections"> Delete these ::: mobile/android/base/locales/en-US/sync_strings.dtd:71 (Diff revision 4) > <!ENTITY fxaccount_status_bookmarks 'Bookmarks'> > <!ENTITY fxaccount_status_history 'History'> > <!ENTITY fxaccount_status_passwords2 'Logins'> > <!ENTITY fxaccount_status_tabs 'Open tabs'> > <!ENTITY fxaccount_status_additional_settings 'Additional Settings'> > +<!ENTITY pref_sync_use_metered "Allow sync over metered connections"> Sync over metered connections ::: mobile/android/base/locales/en-US/sync_strings.dtd:71 (Diff revision 4) > <!ENTITY fxaccount_status_bookmarks 'Bookmarks'> > <!ENTITY fxaccount_status_history 'History'> > <!ENTITY fxaccount_status_passwords2 'Logins'> > <!ENTITY fxaccount_status_tabs 'Open tabs'> > <!ENTITY fxaccount_status_additional_settings 'Additional Settings'> > +<!ENTITY pref_sync_use_metered "Allow sync over metered connections"> Rename these strings to follow format of other strings in the file. e.g. fxaccount_sync_use_metered ::: mobile/android/base/locales/en-US/sync_strings.dtd:74 (Diff revision 4) > <!ENTITY fxaccount_status_tabs 'Open tabs'> > <!ENTITY fxaccount_status_additional_settings 'Additional Settings'> > +<!ENTITY pref_sync_use_metered "Allow sync over metered connections"> > +<!-- Localization note: Only affects background syncing, user initiated > + syncs will still be done regardless of the connection --> > +<!ENTITY pref_sync_use_metered_summary "Background syncing will be done over metered connections"> Allow usage of cellular data and metered wi-fi to sync in the background ::: mobile/android/base/strings.xml.in:261 (Diff revision 4) > <string name="pref_restore">&pref_restore_tabs;</string> > <string name="pref_restore_always">&pref_restore_always;</string> > <string name="pref_restore_quit">&pref_restore_quit;</string> > <string name="pref_sync">&pref_sync2;</string> > <string name="pref_sync_summary">&pref_sync_summary2;</string> > + <string name="pref_sync_use_metered">&pref_sync_use_metered;</string> move these to mobile/android/services/strings.xml.in, and rename them as well (see other note)

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review188194

Pushed by gkruglov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/74b5dba16c23 Make background sync over metered connections optional. r=Grisha Kruglov

Backed out for Android bustage: https://hg.mozilla.org/integration/autoland/rev/8dcd0bcdf363f043a3bc8a551ecd4b1f31b01ba2 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74b5dba16c23380e18942085781234a1dfa32d05&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132978315&repo=autoland [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - /builds/worker/workspace/build/src/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java:27: error: cannot find symbol [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - import org.mozilla.gecko.GeckoSharedPrefs; [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - ^ [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - symbol: class GeckoSharedPrefs [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - location: package org.mozilla.gecko [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - /builds/worker/workspace/build/src/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:20: error: cannot find symbol [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - import org.mozilla.gecko.GeckoSharedPrefs; [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - ^ [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - symbol: class GeckoSharedPrefs [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - location: package org.mozilla.gecko [task 2017-09-24T22:37:50.203Z] 22:37:50 INFO - /builds/worker/workspace/build/src/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java:954: error: cannot find symbol ...

This is a good reminder to verify our work with ./mach build - gradle builds do not currently catch these issues. We can't access GeckoSharedPrefs from the services/* code. There are two options here: 1) just do something like context.getSharedPreferences("GeckoApp", 0). This will work, but it's potentially brittle and hacky. 2) store the new pref in "sync prefs", accessed via fxAccount.getSyncPrefs(). This will be consistent with how the rest of the account pref screen works. Let's go with option (2). Eventually we'll merge the sync pref activity into the regular settings flow, but for now it's good to be consistent. mehdisolamannejad, can you push an updated patch?

(In reply to :Grisha Kruglov from comment #72) > This is a good reminder to verify our work with ./mach build - gradle builds > do not currently catch these issues. > > We can't access GeckoSharedPrefs from the services/* code. There are two > options here: > 1) just do something like context.getSharedPreferences("GeckoApp", 0). This > will work, but it's potentially brittle and hacky. > 2) store the new pref in "sync prefs", accessed via > fxAccount.getSyncPrefs(). This will be consistent with how the rest of the > account pref screen works. > > Let's go with option (2). Eventually we'll merge the sync pref activity into > the regular settings flow, but for now it's good to be consistent. > > mehdisolamannejad, can you push an updated patch? Sorry for the trouble. Going to add checkin-needed.

Comment on attachment 8903451 [details] Bug 802749 - Make background sync over metered connections optional. Kruglov https://reviewboard.mozilla.org/r/175296/#review188592 ::: mobile/android/base/locales/en-US/sync_strings.dtd:74 (Diff revision 6) > <!ENTITY fxaccount_status_tabs 'Open tabs'> > <!ENTITY fxaccount_status_additional_settings 'Additional Settings'> > +<!ENTITY fxaccount_pref_sync_use_metered 'Sync over metered connections'> > +<!-- Localization note: Only affects background syncing, user initiated > + syncs will still be done regardless of the connection --> > +<!ENTITY fxaccount_pref_sync_use_metered_summary 'Allow usage of cellular data and metered wi-fi to sync in the background'> I see this went through copy review, but I'm pretty sure the proper English spelling of this is Wi-Fi, not wi-fi https://transvision.mozfr.org/?recherche=wi-fi&repo=central&sourcelocale=en-US&locale=en-US&search_type=strings_entities

Thanks flod, we did miss this.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5799b3ef8745 Make background sync over metered connections optional. r=Grisha Kruglov

Backed out in https://hg.mozilla.org/integration/autoland/rev/f1157b4dfa69fccb2e97ac61afff58fbaba83f9b until comment 75 can be addressed.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2f33884ae03d Make background sync over metered connections optional. r=Grisha Kruglov

Added release notes in 58.