Last Comment Bug 701826 - Preferences gets unchecked temporarily when going to the preferences page
: Preferences gets unchecked temporarily when going to the preferences page
Status: VERIFIED FIXED
[testday-20111111]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: ---
Assigned To: Brian Nicholson (:bnicholson) (PTO through August 1)
:
Mentors:
Depends on:
Blocks: 703326
  Show dependency treegraph
 
Reported: 2011-11-11 12:43 PST by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2012-01-09 10:30 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
logcat (12.49 KB, text/plain)
2011-11-11 13:45 PST, Naoki Hirata :nhirata (please use needinfo instead of cc)
no flags Details
patch (11.82 KB, patch)
2011-11-15 16:55 PST, Brian Nicholson (:bnicholson) (PTO through August 1)
no flags Details | Diff | Splinter Review
patch v2 (11.81 KB, patch)
2011-11-15 16:58 PST, Brian Nicholson (:bnicholson) (PTO through August 1)
mark.finkle: review-
Details | Diff | Splinter Review
patch v3 (11.77 KB, patch)
2011-11-16 14:56 PST, Brian Nicholson (:bnicholson) (PTO through August 1)
mark.finkle: review+
Details | Diff | Splinter Review

Description Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-11 12:43:31 PST

    
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-11 12:50:47 PST
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) 2011-11-11 13:45:42 PST
Created attachment 573912 [details]
logcat

Based on the logcat I believe that it may be a permissions issue that is slowing things down.
Comment 3 Aaron Train [:aaronmt] 2011-11-11 13:52:10 PST
It's probably just a slow read from Gecko over to Java.
Comment 4 Alex Pakhotin (:alexp) 2011-11-14 13:52:51 PST
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] 2011-11-14 14:05:14 PST
(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) 2011-11-14 14:29:34 PST
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) (PTO through August 1) 2011-11-15 16:55:25 PST
Created attachment 574736 [details] [diff] [review]
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) (PTO through August 1) 2011-11-15 16:58:57 PST
Created attachment 574742 [details] [diff] [review]
patch v2

removed unused initValues() from GeckoPreferences
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-16 13:36:26 PST
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) (PTO through August 1) 2011-11-16 14:56:20 PST
Created attachment 575009 [details] [diff] [review]
patch v3

replaced sPrefsLoaded with GeckoPreferences.isLoaded() and fixed nits.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-17 05:23:22 PST
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) (PTO through August 1) 2011-11-17 11:31:41 PST
Landed http://hg.mozilla.org/projects/birch/rev/013cc5141612

Filed bug 703326 for cleanup.
Comment 13 Aaron Train [:aaronmt] 2011-11-18 07:04:10 PST
Samsung Nexus S (Android 2.3.6)
20111118040220
http://hg.mozilla.org/projects/birch/rev/9999a423d8ab

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