Closed Bug 997328 Opened 6 years ago Closed 6 years ago

SharedPreferences.jsm and SharedPreferencesHelper are not using same prefs

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
fennec 31+ ---

People

(Reporter: jdover, Assigned: jdover)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

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.
tracking-fennec: --- → ?
Assignee: nobody → jdover
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #8407861 - Flags: review?(nalexander)
Status: NEW → ASSIGNED
tracking-fennec: ? → 31+
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.
Attachment #8407861 - Flags: review?(nalexander) → review-
Bumping to P2, since this isn't necessary for our feed add-on release for Fx30.
Priority: P1 → P2
Attachment #8407861 - Attachment is obsolete: true
Attachment #8411404 - Flags: review?(nalexander)
As per our discussion in IRC, this changes SharedPreferences.jsm to have the same API as GeckoSharedPrefs in Java by utilizing the Scope enum.
Attachment #8411405 - Flags: review?(nalexander)
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.
Attachment #8411404 - Flags: review?(nalexander) → review-
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).
Attachment #8411405 - Flags: review?(nalexander) → feedback+
Attachment #8411404 - Attachment is obsolete: true
Attachment #8412974 - Flags: review?(nalexander)
Attachment #8411405 - Attachment is obsolete: true
Attachment #8412975 - Flags: review?(nalexander)
Attachment #8412974 - Flags: review?(nalexander) → review+
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.
Attachment #8412975 - Flags: review?(nalexander) → review+
I realized that about:healthreport was actually using app-level prefs rather than global prefs, so I fixed that. Ready to land!
Attachment #8412975 - Attachment is obsolete: true
Attachment #8416754 - Flags: review+
Keywords: checkin-needed
sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=39043245&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
(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.
(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.
Relanded with small fixes.  Sorry for the delay, jdover!

https://hg.mozilla.org/integration/fx-team/rev/5e1bdfb23c17
Component: Awesomescreen → General
OS: Mac OS X → Android
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/5e1bdfb23c17
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.
Blocks: 1014030
You need to log in before you can comment on or make changes to this bug.