Closed Bug 1068388 Opened 10 years ago Closed 10 years ago

Make Stumbler preference a SharedPreference, not a Gecko pref

Categories

(Android Background Services Graveyard :: Geolocation, defect)

Firefox 35
All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: garvan, Assigned: rnewman)

References

Details

Attachments

(5 files, 1 obsolete file)

+++ 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!
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Summary: Fennec stumbler pref: Android SharedPreferences may not be in sync with Gecko setting → Make Stumbler preference a SharedPreference, not a Gecko pref
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;
                }
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.
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.
Attachment #8490966 - Flags: review?(gkeeley)
Attachment #8490961 - Flags: superreview?(nalexander)
Attachment #8490961 - Flags: feedback?(gkeeley)
Attachment #8490961 - Flags: superreview?(nalexander) → review?(nalexander)
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;
>                 }
Attachment #8490966 - Flags: review?(gkeeley) → review+
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.
Attachment #8490961 - Flags: feedback?(gkeeley) → feedback+
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.
Attachment #8490961 - Flags: review?(nalexander) → review+
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?
(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".
Removed use of app.geo.reportdata in Gecko, based on
http://lxr.mozilla.org/mozilla-central/search?string=app.geo.reportdata
Attachment #8491103 - Flags: review?(rnewman)
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.
Attachment #8491105 - Flags: review?(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!
Attachment #8491105 - Flags: review?(rnewman) → review+
Attachment #8491103 - Flags: review?(rnewman) → review+
(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.
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)
Attachment #8491631 - Flags: review?(nalexander)
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.
Attachment #8491631 - Flags: review?(nalexander) → review+
Thanks Nick, updated, waiting for try server results.
Attachment #8491631 - Attachment is obsolete: true
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
Attachment #8491758 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: