Make Stumbler preference a SharedPreference, not a Gecko pref

RESOLVED FIXED in Firefox 35

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: garvan, Assigned: rnewman)

Tracking

Firefox 35
Firefox 35
All
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
+++ 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

Updated

5 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Summary: Fennec stumbler pref: Android SharedPreferences may not be in sync with Gecko setting → Make Stumbler preference a SharedPreference, not a Gecko pref
Assignee

Comment 1

5 years ago
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;
                }
Assignee

Comment 2

5 years ago
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.
Assignee

Comment 3

5 years ago
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)
Assignee

Updated

5 years ago
Attachment #8490961 - Flags: superreview?(nalexander)
Attachment #8490961 - Flags: feedback?(gkeeley)
Assignee

Updated

5 years ago
Attachment #8490961 - Flags: superreview?(nalexander) → review?(nalexander)
Reporter

Comment 4

5 years ago
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;
>                 }
Reporter

Updated

5 years ago
Attachment #8490966 - Flags: review?(gkeeley) → review+
Reporter

Comment 5

5 years ago
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+
Reporter

Comment 7

5 years ago
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".
Reporter

Comment 9

5 years ago
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)
Reporter

Comment 10

5 years ago
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)
Assignee

Comment 11

5 years ago
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+
Assignee

Updated

5 years ago
Attachment #8491103 - Flags: review?(rnewman) → review+
Assignee

Comment 12

5 years ago
(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.
Reporter

Comment 16

5 years ago
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+
Reporter

Comment 18

5 years ago
Thanks Nick, updated, waiting for try server results.
Attachment #8491631 - Attachment is obsolete: true
Reporter

Comment 19

5 years ago
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+
Reporter

Updated

5 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.