694688 - [birch] Implement Preferences activity

Status: RESOLVED FIXED

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

Description

[Alex Pakhotin (:alexp)]

Implement a native Preferences activity with preference values pulled from Gecko, and changes to be sent back to Gecko.

Comment 1

[Alex Pakhotin (:alexp)]

Attachment: 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?)]

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)]

Attachment: Fix v5

Using int and boolean class members to pass these types.

Comment 5

[Doug Turner (:dougt)]

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)]

Attachment: Fix v6

Moved the Gecko part to JavaScript.
Now using JSON to send preferences in both directions.

Comment 7

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

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)]

(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)]

Attachment: Fix v6.1

Addressed Mark's comments.

Comment 10

[Doug Turner (:dougt)]

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)]

Attachment: rebased alexp's patch

Comment 12

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

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)]

Attachment: rebased with mfinkle's fixes

Comment 14

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

https://hg.mozilla.org/projects/birch/rev/e6b20c1f682d