701826 - Preferences gets unchecked temporarily when going to the preferences page

Status: VERIFIED FIXED

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

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. load preferences
2. watch the checkboxes

Expected: already checked should stay checked on load
Actual: it takes a moment (1.5 seconds) for the checks to show.

http://www.youtube.com/watch?v=ddemMOd_goQ

Motorola Xoom, Droid 2, HTC FLyer

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat

Based on the logcat I believe that it may be a permissions issue that is slowing things down.

Comment 3

[Aaron Train [:aaronmt]]

It's probably just a slow read from Gecko over to Java.

Comment 4

[Alex Pakhotin (:alexp)]

The preferences screen gets initialized asynchronously - when it opens, a message is sent to Gecko to get the preference values, then the response returns back, and the UI is updated. There is a small delay between the request and response with the UI update, which might be a second or two on the slower devices.

My first implementation (bug 694688) cached the preference values in the standard Android shared preferences, which back the the preferences screen, and which the UI gets initial values from. We could re-use that solution if the delay is too annoying.

Comment 5

[Aaron Train [:aaronmt]]

(In reply to Alex Pakhotin (:alexp) from comment #4)
> The preferences screen gets initialized asynchronously - when it opens, a
> message is sent to Gecko to get the preference values, then the response
> returns back, and the UI is updated. There is a small delay between the
> request and response with the UI update, which might be a second or two on
> the slower devices.

It the read time was pretty noticeable on the Nexus S; I would imagine it being more-so evident on a junkier phone.

Comment 6

[Doug Turner (:dougt)]

brian, can you (1) start the preferences lookup before the activity is created, and (b) can you put up a spinner while this is happening?

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: patch

This patch gets the preferences from Gecko immediately after it is ready, and the preferences are kept in memory.  This way, we don't need to issue a Preferences:Get message each time we create the GeckoPreferences activity.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

removed unused initValues() from GeckoPreferences

Comment 9

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

Comment on attachment 574742 [details] [diff] [review]
patch v2


>@@ -103,17 +105,17 @@ abstract public class GeckoApp
>     public static File sGREDir = null;
>     public static Menu sMenu;
>     public Handler mMainHandler;
>     private IntentFilter mConnectivityFilter;
>     private BroadcastReceiver mConnectivityReceiver;
>     public static BrowserToolbar mBrowserToolbar;
>     public static DoorHangerPopup mDoorHangerPopup;
>     public Favicons mFavicons;
>-    private static boolean sIsGeckoReady = false;
>+    private static boolean sPrefsLoaded = false;
>     private IntentFilter mBatteryFilter;
>     private BroadcastReceiver mBatteryReceiver;
>     private Geocoder mGeocoder;
>     private Address  mLastGeoAddress;
>     private static LayerController mLayerController;
>     private static GeckoSoftwareLayerClient mSoftwareLayerClient;
>     boolean mUserDefinedProfile = false;
> 
>@@ -393,17 +395,17 @@ abstract public class GeckoApp
>                             }
>                         });
>                     }
>                 }
>                 mi.setOnMenuItemClickListener(item);
>             }
>         }
> 
>-        if (!sIsGeckoReady)
>+        if (!sPrefsLoaded)
>             aMenu.findItem(R.id.preferences).setEnabled(false);

Do we even need this since we are setEnable in the "Preferences:Daata" handler?

I'd like to kill sPrefsLoaded. Can't you init the menu to disabled in gecko_menu.xml

>+            } else if (event.equals("Gecko:Ready")) {
>+
>+                // retrieve the list of preferences from our preferences.xml file

No blank line

>                 connectGeckoLayerClient();
>+
>             } else if (event.equals("PanZoom:Ack")) {

Remove the blank line

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

>+    private static JSONArray sJsonPrefs = null;

sJsonPrefs -> sJSONPrefs

>     public static void setPreference(String pref, Object value) {
>+
>+        // update the in-memory preferences cache

Remove blank line

r- to see if we can kill sPrefsLoaded

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

replaced sPrefsLoaded with GeckoPreferences.isLoaded() and fixed nits.

Comment 11

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

Comment on attachment 575009 [details] [diff] [review]
patch v3

We could clean this up a little more by moving the initial "make list of preferences, send request to Gecko, listen for return message" into GeckoPreferences too.

File a new bug so we can refactor at some point?

Comment 12

[Brian Nicholson (:bnicholson)]

Landed http://hg.mozilla.org/projects/birch/rev/013cc5141612

Filed bug 703326 for cleanup.

Comment 13

[Aaron Train [:aaronmt]]

Samsung Nexus S (Android 2.3.6)
20111118040220
http://hg.mozilla.org/projects/birch/rev/9999a423d8ab