Closed
Bug 866271
Opened 12 years ago
Closed 12 years ago
Add Javascript interface to Android SharedPreferences
Categories
(Firefox Health Report Graveyard :: Client: Android, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: nalexander, Assigned: nalexander)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
rnewman: mfinkle: I'm using you as a distribution points; can you assign this r? and f? as appropriate.
Attachment #742548 -
Flags: review?(rnewman)
Attachment #742548 -
Flags: feedback?(mark.finkle)
Assignee | ||
Updated•12 years ago
|
Attachment #742548 -
Attachment description: Patch against s-c, part 1 → Bug 866271 - Part 1: Add AndroidSharedPreferencesPrefBranch.
Assignee | ||
Comment 2•12 years ago
|
||
joey: you have any problems with these small changes to build/mobile/robocop/Makefile.in?
Attachment #742550 -
Flags: review?(rnewman)
Attachment #742550 -
Flags: feedback?(joey)
Assignee | ||
Updated•12 years ago
|
Attachment #742550 -
Flags: feedback?(gbrown)
Assignee | ||
Comment 3•12 years ago
|
||
This is still WIP -- I need to add Java preference observers signalling back to Javascipt -- but I'd like feedback on the approach.
Attachment #742552 -
Flags: review?(rnewman)
Comment 4•12 years ago
|
||
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.
Attachment #742550 -
Flags: feedback?(gbrown) → feedback+
Comment 5•12 years ago
|
||
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.
Attachment #742548 -
Flags: review?(rnewman)
Attachment #742548 -
Flags: feedback?(mark.finkle)
Attachment #742548 -
Flags: feedback+
Comment 6•12 years ago
|
||
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.
Attachment #742550 -
Flags: review?(rnewman) → review+
Comment 7•12 years ago
|
||
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.
Attachment #742552 -
Flags: review?(rnewman) → review+
Comment 8•12 years ago
|
||
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 }
Attachment #742548 -
Flags: feedback?(mark.finkle) → feedback+
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Updated•12 years ago
|
Comment 9•12 years ago
|
||
Nick: next on the list is to clean these up for landing.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•12 years ago
|
||
Here's a try build exercising Javascript robocop tests:
https://tbpl.mozilla.org/?tree=Try&rev=63977c0b128c
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #742548 -
Attachment is obsolete: true
Attachment #742550 -
Attachment is obsolete: true
Attachment #742552 -
Attachment is obsolete: true
Attachment #742550 -
Flags: feedback?(joey)
Attachment #748283 -
Flags: review?(rnewman)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #748284 -
Flags: review?(rnewman)
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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.
Attachment #748283 -
Flags: review?(rnewman) → review+
Updated•12 years ago
|
Attachment #748284 -
Flags: review?(rnewman) → review+
Comment 16•12 years ago
|
||
https://hg.mozilla.org/services/services-central/rev/3c93eccac865
https://hg.mozilla.org/services/services-central/rev/42c50c90ca58
Whiteboard: [fixed in services]
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3c93eccac865
https://hg.mozilla.org/mozilla-central/rev/42c50c90ca58
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 23
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•