Last Comment Bug 694688 - [birch] Implement Preferences activity
: [birch] Implement Preferences activity
Status: RESOLVED FIXED
[birch]
: feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P1 normal (vote)
: ---
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
: 695185 (view as bug list)
Depends on:
Blocks: 695458
  Show dependency treegraph
 
Reported: 2011-10-14 16:04 PDT by Alex Pakhotin (:alexp)
Modified: 2012-01-10 11:16 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Fix v4 (29.89 KB, patch)
2011-10-14 16:23 PDT, Alex Pakhotin (:alexp)
blassey.bugs: review-
Details | Diff | Splinter Review
Fix v5 (33.13 KB, patch)
2011-10-17 14:20 PDT, Alex Pakhotin (:alexp)
doug.turner: review-
Details | Diff | Splinter Review
Fix v6 (18.37 KB, patch)
2011-10-18 16:48 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Fix v6.1 (18.15 KB, patch)
2011-10-19 07:11 PDT, Alex Pakhotin (:alexp)
doug.turner: review-
Details | Diff | Splinter Review
rebased alexp's patch (18.47 KB, patch)
2011-10-24 16:21 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
rebased with mfinkle's fixes (21.00 KB, patch)
2011-10-25 14:21 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review

Description Alex Pakhotin (:alexp) 2011-10-14 16:04:05 PDT
Implement a native Preferences activity with preference values pulled from Gecko, and changes to be sent back to Gecko.
Comment 1 Alex Pakhotin (:alexp) 2011-10-14 16:23:48 PDT
Created attachment 567215 [details] [diff] [review]
Fix v4

Asynchronous implementation without extra JNI calls.
- resources/xml/preferences.xml - the preferences screen
  (this version only has one boolean CheckBox preference)
- GeckoPreferences.java - preferences activity
- nsPreferences.cpp - widget side handler
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2011-10-16 22:22:51 PDT
Comment on attachment 567215 [details] [diff] [review]
Fix v4

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

Since SET_PREF is only setting on preference, there's really no need to deal with JSON. If it is a string pref, you can use mCharacters, one of the int fields for in values and add a bool field for bool values.
Comment 3 Alex Pakhotin (:alexp) 2011-10-17 14:20:46 PDT
Created attachment 567574 [details] [diff] [review]
Fix v5

Using int and boolean class members to pass these types.
Comment 4 [:fabrice] Fabrice Desré 2011-10-17 15:02:16 PDT
*** Bug 695185 has been marked as a duplicate of this bug. ***
Comment 5 Doug Turner (:dougt) 2011-10-18 06:35:10 PDT
Comment on attachment 567574 [details] [diff] [review]
Fix v5

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

So high level comments:

1) We should not save gecko prefs to the android system
2) We should not do the jscon parsing in nsPreferences.*.
3) We should reuse the broadcast message type

::: embedding/android/GeckoAppShell.java
@@ +1627,5 @@
>                  String uri = geckoObject.getString("uri");
>                  Tabs.getInstance().addTab(tabId, uri);
> +            } else if (type.equals("preferences")) {
> +                final JSONArray jsonPrefs = geckoObject.getJSONArray("preferences");
> +                GeckoPreferences.savePreferences(jsonPrefs);

I don't understand savePreferences name.  It should be LoadPreferences() because you should not save these preferences anywhere on the android.

::: embedding/android/GeckoEvent.java
@@ +75,5 @@
>      public static final int ACTIVITY_START = 17;
>      public static final int SAVE_STATE = 18;
>      public static final int BROADCAST = 19;
> +    public static final int GET_PREFS = 20;
> +    public static final int SET_PREF = 21;

You should just use the BROADCAST type.  It allows you to send arbitrary data.  So, from java, you could just do:


GeckoEvent e = new GeckoEvent("preferences", <json>);
GeckoAppShell.sendEventToGecko(e);

@@ +112,5 @@
>      public int mRangeType, mRangeStyles;
>      public int mRangeForeColor, mRangeBackColor;
>      public Location mLocation;
>      public Address  mAddress;
> +    public int mIntValue;

can we reuse on of the other int's here?

@@ +113,5 @@
>      public int mRangeForeColor, mRangeBackColor;
>      public Location mLocation;
>      public Address  mAddress;
> +    public int mIntValue;
> +    public boolean mBoolValue;

would it be okay to use an int here and cast it to a boolean?  Maybe Java is strict about that.

::: embedding/android/GeckoPreferences.java
@@ +40,5 @@
> +import android.content.SharedPreferences;
> +import android.os.Bundle;
> +import android.content.res.Resources;
> +import android.preference.*;
> +import android.preference.Preference.*;

android.preferences.* not needed.  Maybe I am missing something.  I do not think you want to save any of the gecko preferences to the android system.

@@ +45,5 @@
> +import android.util.Log;
> +import java.util.ArrayList;
> +import org.json.*;
> +
> +public class GeckoPreferences

I suspect alot of this class is going to change given that I do not think you want to save gecko preferences into the android system.

@@ +124,5 @@
> +        if (prefsEditor == null)
> +            return;
> +
> +        try {
> +            for (int i = 0; i < jsonPrefs.length(); i++) {

doesn't length() get evaluated every time through this loop.  i can't remember if java optimized such loops, but i suspect not.

::: widget/src/android/AndroidJavaWrappers.cpp
@@ +69,5 @@
>  jfieldID AndroidGeckoEvent::jRangeBackColorField = 0;
>  jfieldID AndroidGeckoEvent::jLocationField = 0;
>  jfieldID AndroidGeckoEvent::jAddressField = 0;
> +jfieldID AndroidGeckoEvent::jIntValueField = 0;
> +jfieldID AndroidGeckoEvent::jBoolValueField = 0;

I hope none of the changes in AndroidJavaWrapper.cpp are needed.

::: widget/src/android/AndroidJavaWrappers.h
@@ +426,5 @@
>      int RangeBackColor() { return mRangeBackColor; }
>      nsGeoPosition* GeoPosition() { return mGeoPosition; }
>      nsGeoPositionAddress* GeoAddress() { return mGeoAddress; }
> +    int IntValue() { return mIntValue; }
> +    int BoolValue() { return mBoolValue; }

I hope none of the changes in AndroidJavaWrapper.h are needed.

::: widget/src/android/nsAppShell.cpp
@@ +53,5 @@
>  #include "nsDeviceMotionSystem.h"
>  #include <android/log.h>
>  #include <pthread.h>
>  #include <wchar.h>
> +#include "nsPreferences.h"

I am hoping that none of the changes to nsAppShell.cpp are needed.

::: widget/src/android/nsPreferences.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

I do not want to do json parsing here.  can you do either one of two things:

1) move this to javascript where it is easy, and most likely bug free.
2) extend nsIJSON to allow C++ code to easily deal with json data.


I would do (1).
Comment 6 Alex Pakhotin (:alexp) 2011-10-18 16:48:55 PDT
Created attachment 567930 [details] [diff] [review]
Fix v6

Moved the Gecko part to JavaScript.
Now using JSON to send preferences in both directions.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-18 19:43:33 PDT
Comment on attachment 567930 [details] [diff] [review]
Fix v6

Just a drive by:
* "boolVal", "intVal" and "stringVal" could all be "value" in JSON. We don't need the specialized names.

* Why are we using:
let branch = Services.prefs.getBranch(null).QueryInterface(Ci.nsIPrefBranch2);

Why not just
Services.prefs ?
Comment 8 Alex Pakhotin (:alexp) 2011-10-18 20:27:53 PDT
(In reply to Mark Finkle (:mfinkle) from comment #7)
> * "boolVal", "intVal" and "stringVal" could all be "value" in JSON. We don't
> need the specialized names.

I was thinking about this. Agree - with JSON all over the place it would make sense.

> * Why are we using:
> let branch =
> Services.prefs.getBranch(null).QueryInterface(Ci.nsIPrefBranch2);
> 
> Why not just
> Services.prefs ?

I had the same question when was looking at config.js. Maybe because getPrefType is a method of nsIPrefBranch? I just left this to be clarified later.
Comment 9 Alex Pakhotin (:alexp) 2011-10-19 07:11:13 PDT
Created attachment 568036 [details] [diff] [review]
Fix v6.1

Addressed Mark's comments.
Comment 10 Doug Turner (:dougt) 2011-10-22 18:51:12 PDT
Comment on attachment 568036 [details] [diff] [review]
Fix v6.1

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

i handed this patch over to Brian.
Comment 11 Brian Nicholson (:bnicholson) 2011-10-24 16:21:40 PDT
Created attachment 569220 [details] [diff] [review]
rebased alexp's patch
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 10:08:43 PDT
Comment on attachment 569220 [details] [diff] [review]
rebased alexp's patch


>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>+            } else if (type.equals("preferences")) {
>+                final JSONArray jsonPrefs = geckoObject.getJSONArray("preferences");
>+                GeckoPreferences.updatePreferenceScreen(jsonPrefs);

"preferences" -> "Preferences:Data"

>diff --git a/embedding/android/GeckoPreferences.java b/embedding/android/GeckoPreferences.java

>+    protected void onCreate(Bundle savedInstanceState) {

>+        initPreferenceGroup(mPreferenceScreen);
>+        initPreferenceValues();

initPreferenceGroup -> initGroups
initPreferenceValues -> initValues

>+    // Initialize preferences by sending the "preferences-get" command to Gecko
>+    private void initPreferenceValues() {
>+        JSONArray jsonPrefs = new JSONArray(mPreferencesList);
>+
>+        GeckoEvent event = new GeckoEvent("preferences-get", jsonPrefs.toString());

"preferences-get" -> "Preferences:Data"

>+    public static void updatePreferenceScreen(JSONArray jsonPrefs) {

updatePreferenceScreen -> refresh

>+                    GeckoAppShell.getMainHandler().post(new Runnable() {
>+                            public void run() {
>+                                if (((CheckBoxPreference)pref).isChecked() != value)
>+                                    ((CheckBoxPreference)pref).setChecked(value);
>+                            }
>+                        });

Change this to | GeckoAppShell.getHandler().post(... |
(in all places)
un-indent the code bock 4 spaces

>+            }
>+
>+        } catch (JSONException e) {

Remove blank line

>+    // Send the "preferences-set" command with a preference value to Gecko
>+    public static void setPref(String pref, Object value) {

setPref -> setPreference

"preferences-set" -> "Preferences:Set"

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+    Services.obs.addObserver(this, "preferences-get", false);
>+    Services.obs.addObserver(this, "preferences-set", false);

"preferences-get" -> "Preferences:Get"
"preferences-set" -> "Preferences:Set"

>+  getPreferences: function getPreferences(aPrefNames) {

>+      for each (let prefName in json) {
>+        let pref = {
>+                     name: prefName,
>+                   };

          let pref = {
            name: prefName,
          };

>+      sendMessageToJava({
>+        gecko: {
>+          type: "preferences",

"preferences" -> "Preferences:Data"

>   observe: function(aSubject, aTopic, aData) {

>+    else if (aTopic == "preferences-get") 
>+      this.getPreferences(aData);
>+    else if (aTopic == "preferences-set") 
>+      this.setPreferences(aData);

Update the strings

r+, but update the patch and fix nits
Comment 13 Brian Nicholson (:bnicholson) 2011-10-25 14:21:09 PDT
Created attachment 569505 [details] [diff] [review]
rebased with mfinkle's fixes
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 14:40:47 PDT
https://hg.mozilla.org/projects/birch/rev/e6b20c1f682d

Note You need to log in before you can comment on or make changes to this bug.