997328 - SharedPreferences.jsm and SharedPreferencesHelper are not using same prefs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Josh Dover [:jdover]]

While updating the Pocket home panel, I noticed that setting the authentication as completed wasn't working and found that this is due to the fact that [1] PanelAuthCache was listening to changes to the per-profile prefs, but SharedPreferences.jsm uses the per-app prefs.

SharedPreferences should allow for both per-app and per-profile.

Comment 1

[Josh Dover [:jdover]]

Attachment: patch

This allows the user of SharedPreferences.jsm to specify if they would like to use per-profile prefs, defaulting to per-app prefs (current behavior).

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8407861 [details] [diff] [review]
patch

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

Tell me what you think about re-working the API; I'm open to discussion on this point.  No tests that exercise the new functionality is a blocker, though.

::: mobile/android/base/tests/testSharedPreferences.js
@@ -32,1 @@
>  

This makes the tests pass, but doesn't test any of the new things you have added.

::: mobile/android/modules/SharedPreferences.jsm
@@ +19,5 @@
> + * options {Object} with the following valid keys:
> + *   - branch {String} (optional) should be a string describing a preferences branch,
> + *     like "UpdateService" or "background.data", or null to access the
> + *     default preferences branch for the application.
> + *   - forProfile {boolean} indicates whether or not to use the profile-specific

I see that this change is minimal, but I'm not a fan of the resulting API.

What we're doing here is aligning SharedPreferences with GeckoSharedPrefs.  GSP has the concept of "scope"; right now, there is APP scope and PROFILE scope.  To this mixed, SP adds ANDROID scope.

So rather than having flags, etc, I'd rather have explicit constructors that mirror GSP:

29  * forApp()
30  *     Use it for app-wide, cross-profile pref keys.
31  * forProfile()
32  *     Use it to fetch and store keys for the current profile.
33  * forProfileName()
34  *     Use it to fetch and store keys from/for a specific profile.

and then have another "forAndroid(branch)" that acts like the old code.  I suppose that SharedPreferences is no longer a constructor, but that's fine.

We could conceivably do away with "forAndroid" -- it's almost always better to use Fennec's scopes -- but it's handy for testing that your other code does what it should, and in future for add-ons.

Comment 3

[:Margaret Leibovic]

Bumping to P2, since this isn't necessary for our feed add-on release for Fx30.

Comment 4

[Josh Dover [:jdover]]

Attachment: 01 - Add Scope enum to describe SharedPreferences

Comment 5

[Josh Dover [:jdover]]

Attachment: 02 - Update SharedPreferences.jsm to use scopes and mirror GeckoSharedPrefs

As per our discussion in IRC, this changes SharedPreferences.jsm to have the same API as GeckoSharedPrefs in Java by utilizing the Scope enum.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8411404 [details] [diff] [review]
01 - Add Scope enum to describe SharedPreferences

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

Rather than modifying GSP, fold the Scope-parsing into SharedPrefsHelper.  Have the Helper delegate App/Profile to GSP, and handle Android scope itself.

That's the behaviour I think is best: pass-through where possible; back-door to facilitate testing.

::: mobile/android/base/GeckoSharedPrefs.java
@@ +76,5 @@
> +    public enum Scope {
> +        APP("app"),
> +        PROFILE("profile"),
> +        PROFILE_NAME("profileName"),
> +        GLOBAL("global");

I don't want to add this global scope to GSP.  Fennec shouldn't be using this global scope at all (it should be using App scope).

And profile vs. profile name is not a distinction I want to make either.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8411405 [details] [diff] [review]
02 - Update SharedPreferences.jsm to use scopes and mirror GeckoSharedPrefs

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

The JS side of this is looking great.  I've asked for Java changes because I don't want the global scope to leak to GSP.  I think that will simplify the branch-for-scope logic, too.

::: mobile/android/base/SharedPreferencesHelper.java
@@ +68,5 @@
> +                return GeckoSharedPrefs.forApp(mContext);
> +            case PROFILE:
> +                return GeckoSharedPrefs.forProfile(mContext);
> +            case PROFILE_NAME:
> +                final String profileName = message.getString("profileName");

nit: comment that null is allowed (if that is the case).  Should this be optString?

@@ +202,5 @@
>       */
>      private void handleObserve(JSONObject message) throws JSONException {
> +        final SharedPreferences prefs = getSharedPreferences(message);
> +        final boolean enable = message.getBoolean("enable");
> +        final Scope scope = Scope.forKey(message.getString("scope"));

this can be null :(

@@ +209,5 @@
> +        String profileName = null;
> +        switch (scope) {
> +            case GLOBAL:
> +                final String globalBranch = message.optString("branch", null);
> +                branch = GeckoSharedPrefs.getBranchForScope(mContext, scope, globalBranch);

Further to the overall comment on previous patch, do this branch parsing in the helper.

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +26,5 @@
>  const EVENT_HEALTH_RESPONSE = "HealthReport:Response";
>  
>  // about:healthreport prefs are stored in Firefox's default Android
>  // SharedPreferences.
> +let sharedPrefs = SharedPreferences.forAndroid();

I've filed Bug 1001037 to address this, but it's not a priority.

@@ +189,5 @@
>    },
>  };
>  
>  window.addEventListener("load", healthReportWrapper.init.bind(healthReportWrapper), false);
> +window.addEventListener("unload", healthReportWrapper.uninit.bind(healthReportWrapper), false);

nit: it looks like your editor is borking last line newlines.  Don't commit these bad hunks.

::: mobile/android/modules/SharedPreferences.jsm
@@ +23,5 @@
> +/**
> + * Public API to getting a SharedPreferencesAdapter instance. These scopes mirror GeckoSharedPrefs.
> + */
> +let SharedPreferences = {
> +  forApp: function() {

This ended up just as I envisioned.  Good work!

@@ +53,2 @@
>   */
> +function SharedPreferencesAdapter(options = {}) {

nit: it's traditional to call this private inner delegate thing the "implementation" (the above being the interface).  So consider SharedPreferencesImpl.

@@ +213,5 @@
>      }
>  
>      let msg = JSON.parse(data);
> +    if (msg.scope != this._scope ||
> +        this._scope === PROFILE_NAME && msg.profileName != this._profileName ||

nit: parenthesize inner comparisons for E-Z-Reading (patent pending).

Comment 8

[Josh Dover [:jdover]]

Attachment: 01 - Add Scope enum to describe SharedPreferences

Comment 9

[Josh Dover [:jdover]]

Attachment: 02 - Update SharedPreferences.jsm to use scopes and mirror GeckoSharedPrefs

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8412975 [details] [diff] [review]
02 - Update SharedPreferences.jsm to use scopes and mirror GeckoSharedPrefs

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

Looks good!  Fold these two patches into one for landing.  Also, have you manually tested about:healthreport?  (My guess is that it's broken and this patch corrects it, but we should verify.)  We can ask QA to verify.  And sorry for the slow and multiple review cycle.

::: mobile/android/base/SharedPreferencesHelper.java
@@ +250,5 @@
>  
>          // mListeners is only modified in this one observer, which is called
>          // from Gecko serially.
>          if (enable && !this.mListeners.containsKey(branch)) {
> +            SharedPreferences.OnSharedPreferenceChangeListener listener

let the long lines ride unless you're really changing them.

Comment 11

[Josh Dover [:jdover]]

Attachment: 02 - Update SharedPreferences.jsm to use scopes and mirror GeckoSharedPrefs

I realized that about:healthreport was actually using app-level prefs rather than global prefs, so I fixed that. Ready to land!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/a7b80bdc8a7c
https://hg.mozilla.org/integration/fx-team/rev/2b5863a3f0ae

Comment 13

[Carsten Book [:Tomcat]]

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=39043245&tree=Fx-Team

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Carsten Book [:Tomcat] from comment #13)
> sorry had to backout this changes for test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=39043245&tree=Fx-Team

Okay, I can confirm this NPE locally.  Digging in.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #14)
> (In reply to Carsten Book [:Tomcat] from comment #13)
> > sorry had to backout this changes for test failures like
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=39043245&tree=Fx-Team
> 
> Okay, I can confirm this NPE locally.  Digging in.

Bug found; it's actually in the test; fix tomorrow.

Comment 16

[Nick Alexander :nalexander [he/him]]

Relanded with small fixes.  Sorry for the delay, jdover!

https://hg.mozilla.org/integration/fx-team/rev/5e1bdfb23c17

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5e1bdfb23c17

Comment 18

[:Margaret Leibovic]

Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.