866271 - Add Javascript interface to Android SharedPreferences

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect, P1)

Description

[Nick Alexander :nalexander [he/him]]

We are designing Firefox Health Report on Android to mostly be implemented in Java, and with this design it is most natural for preferences controlling data reporting and health report collection and upload to be managed by Android SharedPreferences.

To make about:healthreport seamlessly get and set preferences, we need to implement a bridge between JS and Java.  There's no sense in making this specific to FHR, so let's make a generic JSM for this.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 866271 - Part 1: Add AndroidSharedPreferencesPrefBranch.

rnewman: mfinkle: I'm using you as a distribution points; can you assign this r? and f? as appropriate.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 866271 - Part 2: Add Robocop Java harness for writing tests in Javascript.

joey: you have any problems with these small changes to build/mobile/robocop/Makefile.in?

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 866271 - Part 3: Add Robocop tests for AndroidSharedPreferencesPrefBranch.

This is still WIP -- I need to add Java preference observers signalling back to Javascipt -- but I'd like feedback on the approach.

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 742550 [details] [diff] [review]
Bug 866271 - Part 2: Add Robocop Java harness for writing tests in Javascript.

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

This looks good. I think it will encourage more JS testing via robocop. Thanks!

::: mobile/android/base/tests/JavascriptTest.java.in
@@ +15,5 @@
> +public class JavascriptTest extends BaseTest {
> +    public static final String LOGTAG = "JavascriptTest";
> +
> +    // Keep these constants consistent with /dom/imptests/testharness.js.
> +    public static final int PASS = 0;

Could you use strings instead?

@@ +28,5 @@
> +        this.javascriptUrl = javascriptUrl;
> +    }
> +
> +    @Override
> +    protected void setUp() throws Exception {

Probably not needed?

@@ +45,5 @@
> +        // the test harness runs each test in the suite and completes testing
> +        // before the page load event is fired.  See the comments in
> +        // /dom/imptests/testharness.js.
> +        Actions.EventExpecter expecter = mActions.expectGeckoEvent("Robocop:Status");
> +        Log.w(LOGTAG, "Registered listener for Robocop:Status");

If you want any of these in the final patch, I encourage you to switch to mAsserter.dumpLog.

@@ +61,5 @@
> +
> +            JSONObject o = new JSONObject(data);
> +            String innerType = o.getString("innerType");
> +            if (!"completion".equals(innerType))
> +                throw new Exception("Unexpected Robocop:Status innerType " + innerType);

You could mAsserter.ok(false, ...) instead.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 742548 [details] [diff] [review]
Bug 866271 - Part 1: Add AndroidSharedPreferencesPrefBranch.

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

r+ with the obvious nit that you still have XXX/TODO to fill in, as we discussed earlier!

::: mobile/android/base/BrowserApp.java
@@ +536,5 @@
>          registerEventListener("Telemetry:Gather");
>  
>          Distribution.init(this, getPackageResourcePath());
>          JavaAddonManager.getInstance().init(getApplicationContext());
> +        AndroidSharedPreferencesHelper.getInstance().init(getApplicationContext());

My singleton sadface, let me show you it.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 742550 [details] [diff] [review]
Bug 866271 - Part 2: Add Robocop Java harness for writing tests in Javascript.

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

No additional comments beyond gbrown's. consts probably won't change because testharness.js.

::: mobile/android/base/tests/JavascriptTest.java.in
@@ +1,1 @@
> +#filter substitution

License.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 742552 [details] [diff] [review]
Bug 866271 - Part 3: Add Robocop tests for AndroidSharedPreferencesPrefBranch.

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

::: mobile/android/base/tests/testAndroidSharedPreferencesPrefBranch.java.in
@@ +1,1 @@
> +#filter substitution

License.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 742548 [details] [diff] [review]
Bug 866271 - Part 1: Add AndroidSharedPreferencesPrefBranch.

>diff --git a/mobile/android/base/AndroidSharedPreferencesHelper.java b/mobile/android/base/AndroidSharedPreferencesHelper.java

nit: I'd be fine with a shorter name: SharedPreferencesHelper.java

>+        mDispatcher.registerEventListener("AndroidSharedPreferences:Set", this);
>+        mDispatcher.registerEventListener("AndroidSharedPreferences:Get", this);

Moar naming OCD: "AndroidSharedPreferences:Xxx" -> "SharedPreferences:Xxx"


>+    public void handleMessage(String event, JSONObject message) {
>+        // Everything here is synchronous and serial, so we need not worry about
>+        // overwriting an in-progress response.
>+        mResponse = null;

Thinking out loud: messages come in on the Gecko thread. We try not to block that thread, so Gecko can continue it's super-important work. SharedPreferences.editor.commit() will cause writes to "disk" and cause strict warnings on UI thread. Maybe we should use a background thread for at least the writes. Thoughts?

>+            if (event.equals("AndroidSharedPreferences:Set")) {
>+                Log.w(LOGTAG, "Got AndroidSharedPreferences:Set message.");
>+                handleSet(message);
>+            } else if (event.equals("AndroidSharedPreferences:Get")) {
>+                Log.w(LOGTAG, "Got AndroidSharedPreferences:Get message.");

Drop the Log.w before landing?

>diff --git a/mobile/android/modules/AndroidSharedPreferencesPrefBranch.jsm b/mobile/android/modules/AndroidSharedPreferencesPrefBranch.jsm

"AndroidSharedPreferencesPrefBranch.jsm" is a mouthful. "SharedPreferences.jsm"?

>+this.EXPORTED_SYMBOLS = ['AndroidSharedPreferencesPrefBranch'];

nit: " instead of '

>+const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

nit" { classes ... Cu }

Comment 9

[Richard Newman [:rnewman]]

Nick: next on the list is to clean these up for landing.

Comment 10

[Nick Alexander :nalexander [he/him]]

Here's a try build exercising Javascript robocop tests:

https://tbpl.mozilla.org/?tree=Try&rev=63977c0b128c

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 866271 - Add Javascript interface to Android SharedPreferences. r=rnewman

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 866271 - Android SharedPreferences tests. r=rnewman

Comment 13

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=1b77c88798f2

Comment 14

[Nick Alexander :nalexander [he/him]]

Mercurial 2.0.2 seems to have a Mac OS X filename case bug.  Try

https://tbpl.mozilla.org/?tree=Try&rev=85dad1ab2455

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 748283 [details] [diff] [review]
Bug 866271 - Add Javascript interface to Android SharedPreferences. r=rnewman

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

::: mobile/android/base/SharedPreferencesHelper.java
@@ +179,5 @@
> +
> +                // Truly, this is awful, but the API impedence is strong: there
> +                // is no way to get a single untyped value from a
> +                // SharedPreferences instance.
> +                msg.put("value", sharedPreferences.getAll().get(key));

Wow, that is bad. I wonder if it's cheaper to try each type and catch ClassCastException?

::: mobile/android/modules/SharedPreferences.jsm
@@ +1,4 @@
> +// -*- Mode: js2; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Newline after license.

Comment 16

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/3c93eccac865
https://hg.mozilla.org/services/services-central/rev/42c50c90ca58

Comment 17

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/3c93eccac865
https://hg.mozilla.org/mozilla-central/rev/42c50c90ca58