1068388 - Make Stumbler preference a SharedPreference, not a Gecko pref

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[:garvan]

+++ This bug was initially created as a clone of Bug #1064677 +++

From https://bugzilla.mozilla.org/show_bug.cgi?id=1064677#c50

What you broadcast is read from SharedPreferences.
The pref that's shown in Settings is read from Gecko.
See the use of not_a_pref in GeckoPreferences to indicate "nope, this isn't a Gecko pref".
See also how "locale" isn't prefixed that way, and so is specially handled elsewhere.
Right now we assume false on both sides for stumbling, but that's just coincidence!

Comment 1

[Richard Newman [:rnewman]]

Question: is this still true?

                else if (AppConstants.RELEASE_BUILD &&
                            (PREFS_GEO_REPORTING.equals(key) ||
                             PREFS_GEO_LEARN_MORE.equals(key))) {
                    // We don't build wifi/cell tower collection in release builds, so hide the UI.
                    preferences.removePreference(pref);
                    i--;
                    continue;
                }

Comment 2

[Richard Newman [:rnewman]]

Attachment: Clarify that Stumbler pref is Android-only. v1

This patch:

  * Renames PREFS_STUMBLER_ENABLED to ACTION_STUMBLER_UPLOAD_PREF, 'cos that's what it is.
  * Prefixes PREFS_GEO_REPORTING with the NON_PREF_PREFIX so we look only in SharedPreferences, rather than asking Gecko.
  * Eliminates the Gecko lookup branch for that pref.

Note that we do not migrate existing Gecko pref values into SharedPreferences.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Hide stumbler settings items if stumbler isn't built. v1

This should really be a separate bug, but this is good enough.

We no longer use release conditionalization, so this should use the build flag. And now it does.

Comment 4

[:garvan]

The following is no longer true, thanks for addressing that. It should be build time conditional, as you did.

(In reply to Richard Newman [:rnewman] from comment #1)
> Question: is this still true?
> 
>                 else if (AppConstants.RELEASE_BUILD &&
>                             (PREFS_GEO_REPORTING.equals(key) ||
>                              PREFS_GEO_LEARN_MORE.equals(key))) {
>                     // We don't build wifi/cell tower collection in release
> builds, so hide the UI.
>                     preferences.removePreference(pref);
>                     i--;
>                     continue;
>                 }

Comment 5

[:garvan]

Comment on attachment 8490961 [details] [diff] [review]
Clarify that Stumbler pref is Android-only. v1

Thanks Richard, looks good to me, patching locally and building now to test hands-on.

ACTION_STUMBLER_UPLOAD_PREF is most accurately named ACTION_STUMBLER_ENABLE_AND_UPLOAD_PREF. 

The Intent extra sets the enable/disable behaviour, and when enabled, it also schedules an upload check.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8490961 [details] [diff] [review]
Clarify that Stumbler pref is Android-only. v1

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

Have a rubbery stamp.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +1321,5 @@
>  
>              @Override
>              public void prefValue(String prefName, final int value) {
>                  final Preference pref = getField(prefName);
> +                Log.w(LOGTAG, "Unhandled int value for pref [" + pref + "]");

Well, isn't this a nice simplification.

Comment 7

[:garvan]

On this line
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#1060

android.not_a_preference.app.geo.reportdata != app.geo.reportdata

Should this be a String.contains() match?

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Garvan Keeley [:garvank] from comment #7)
> On this line
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> preferences/GeckoPreferences.java#1060
> 
> android.not_a_preference.app.geo.reportdata != app.geo.reportdata
> 
> Should this be a String.contains() match?

No, these two should be the same thing.  rnewman's patch deprecates 'app.geo.reportdata' and it should no longer be referenced.  The point is that 'app.geo.reportdata' is a Gecko pref; by adding the prefix, we're opting out of the Gecko pref handling and saying, "Just right this to Shared Preferences".

Comment 9

[:garvan]

Attachment: Remove gecko use of geo.reportdata

Removed use of app.geo.reportdata in Gecko, based on
http://lxr.mozilla.org/mozilla-central/search?string=app.geo.reportdata

Comment 10

[:garvan]

Attachment: Add the "not a gecko" prefix to the UI for geo.reportdata

Renamed app.geo.reportdata to have android.not_a_preference prefix

With all the patches in this bug to this point applied, tested, everything working a-ok.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8491105 [details] [diff] [review]
Add the "not a gecko" prefix to the UI for geo.reportdata

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

Thanks, I missed this!

Comment 12

[Richard Newman [:rnewman]]

(In reply to Garvan Keeley [:garvank] from comment #7)
> On this line
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> preferences/GeckoPreferences.java#1060

This also shows a bit that can be amended. We can short-circuit here, so I'll land as such.

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/920b201d30e9
https://hg.mozilla.org/integration/fx-team/rev/cf6f1223b6b7
https://hg.mozilla.org/integration/fx-team/rev/e85caca8529f
https://hg.mozilla.org/integration/fx-team/rev/29d39909222d

Comment 14

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/099d0ec27cd9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Still failing robocop. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/d6df7c9e58f5

https://tbpl.mozilla.org/php/getParsedLog.php?id=48334667&tree=Fx-Team

Comment 16

[:garvan]

Attachment: Testing patch: if stumbler built off, test should ignore stumbler prefs

Updated the robocop test for when stumbler is built off. 

(Tried to get a fully successful robocop run locally, but it fails in unrelated ways on my system)

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8491631 [details] [diff] [review]
Testing patch: if stumbler built off, test should ignore stumbler prefs

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

::: mobile/android/base/tests/testSettingsMenuItems.java
@@ +169,5 @@
>              if (NewTabletUI.isEnabled(getActivity())) {
>                  settingsMap.get(PATH_DISPLAY).remove(TITLE_BAR_LABEL_ARR);
>              }
>  
> +          // Anonymous cell tower/wifi collection.

nit: indentation looks off.  Also, this is not conditional on AppConstants.RELEASE_BUILD: move it out of this block.

Comment 18

[:garvan]

Attachment: Patch for test case: if stumbler built off, test should ignore stumbler prefs

Thanks Nick, updated, waiting for try server results.

Comment 19

[:garvan]

Comment on attachment 8491758 [details] [diff] [review]
Patch for test case: if stumbler built off, test should ignore stumbler prefs

Try green
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0b0ba8783b2

Carrying over r+ from prev review

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e2e8d945b53b
https://hg.mozilla.org/integration/fx-team/rev/6d4380faf42c
https://hg.mozilla.org/integration/fx-team/rev/81fe068f3c0b
https://hg.mozilla.org/integration/fx-team/rev/5629a09d7c3f
https://hg.mozilla.org/integration/fx-team/rev/0a4ce94e4658

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/e2e8d945b53b
https://hg.mozilla.org/mozilla-central/rev/6d4380faf42c
https://hg.mozilla.org/mozilla-central/rev/81fe068f3c0b
https://hg.mozilla.org/mozilla-central/rev/5629a09d7c3f
https://hg.mozilla.org/mozilla-central/rev/0a4ce94e4658