Last Comment Bug 694455 - [birch] Android Prompt Service
: [birch] Android Prompt Service
: feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
P1 normal (vote)
: ---
Assigned To: Wesley Johnston (:wesj)
: Sebastian Kaspari (:sebastian)
: 695179 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-10-13 16:49 PDT by Wesley Johnston (:wesj)
Modified: 2012-01-10 11:34 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP Patch (52.87 KB, text/plain)
2011-10-13 16:49 PDT, Wesley Johnston (:wesj)
no flags Details
WIP 2 (64.80 KB, patch)
2011-10-14 18:12 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v1 (40.08 KB, patch)
2011-10-19 12:29 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 (42.44 KB, patch)
2011-10-20 11:52 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2.1 (42.36 KB, patch)
2011-10-20 12:19 PDT, Wesley Johnston (:wesj)
blassey.bugs: review+
Details | Diff | Splinter Review
Patch v2.2 (43.56 KB, patch)
2011-10-20 18:01 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2.3 (47.26 KB, patch)
2011-10-20 20:57 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
wjohnston2000: review+
Details | Diff | Splinter Review

Description User image Wesley Johnston (:wesj) 2011-10-13 16:49:37 PDT
Created attachment 566973 [details]
WIP Patch

We'll need to show prompts somehow on Android. Some of them will come through the android prompt service. Others may come through some sort sort of "Context menu handler" or "Select element handler", the master password stuff, extensions?

This gives us a prompt handler in java that we can talk to through our messaging component. Then this uses it for the prompt service. I'm currently seeing system crashes if I hold a dialog open for too long, and there are multiple places where i changed stuff without thinking about it to much. i.e. needs some cleanup, but wanted to put this up for some feedback.
Comment 1 User image Lucas Rocha (:lucasr) 2011-10-14 03:28:38 PDT
Had a quick look at the patch. Some preliminary comments:
- No need to import org.mozilla.gecko as they are in the same package
- I'd try to avoid using * imports as much as possible. Usually you're just using a few classes from the package.
- Add empty line between methods
- The "Show" method should probably be "show" for consistency
- Coding style fixes in PromptService.js should probably go in a separate patch
- I guess you're just using aboutHome for testing your changes, right?
Comment 2 User image Wesley Johnston (:wesj) 2011-10-14 18:12:31 PDT
Created attachment 567234 [details] [diff] [review]

This works slightly better.

I think the crash I am seeing is related to the way I am spinning the event loop in Gecko. I am passing "false" to the threadmanager processnextevent method because no events are on the queue when the dialog is hidden (i.e. you have to drag or something to kick off javascript execution again). I THINK that while the dialog is open, this may cause Android to think we are hung... sometimes.

This right now passes true, and then fires a fake MOVE event to try and get things moving again. I haven't seen the same hangs occur when I do this. Its a hack though. I need a better way to tickle this.
Comment 3 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-10-15 21:35:06 PDT
The general idea here is a good one: passing prompt request to Java and using the native dialogs.

I don't see how we can avoid spinning an event loop in Gecko. These dialogs need to be modal. I know we want to avoid JNI, but that might be an approach where we wouldn't need to spin the event loop because Java/JNI would block for us.
Comment 4 User image Lucas Rocha (:lucasr) 2011-10-17 13:06:46 PDT
Side comment: maybe the changes adding 'Log' calls everywhere should go on a separate patch. Also, let's not use wildcard imports in java code. Only import what the java source file needs.
Comment 5 User image Wesley Johnston (:wesj) 2011-10-17 13:31:44 PDT
Yeah. I apologise about the logging. Just trying to figure out if there were IME events being sent to Gecko that were causing these hangs for some reason. Should have pulled them out before putting this up (or made a new patch for them).
Comment 6 User image [:fabrice] Fabrice Desré 2011-10-17 15:01:26 PDT
*** Bug 695179 has been marked as a duplicate of this bug. ***
Comment 7 User image Wesley Johnston (:wesj) 2011-10-19 12:29:35 PDT
Created attachment 568153 [details] [diff] [review]
Patch v1

Rebased on blassey explaining to me how to block the gecko thread in java.

This adds a string return value from our message passing code. In java, we block when the prompt is dismissed we return a json response with the button pressed and any values input by the user.
Comment 8 User image Wesley Johnston (:wesj) 2011-10-19 12:47:46 PDT
Crud. Just remembered this also crashes on orientation changes. I'm leaving the review up for now and I'll write that fix in a second patch.
Comment 9 User image Sriram Ramasubramanian [:sriram] 2011-10-20 10:04:09 PDT
Comment on attachment 568153 [details] [diff] [review]
Patch v1

I feel, a better approach would be to create layouts in XML, and inflate it based on the type. The kinds of dialogs we would have would be minimal (say 5). It's better to have 5 such dialog boxes designed in XML and inflate it based on the type we receive. The type is going to specify the dialog box to use, and not the controls it needs. The buttons would be predefined and the result is going to carry the button that was clicked.

The menulist will be loaded in a dialog as above and the value can be passed back. Creating based on individual type of control needed sounds scary to me. I am sorry, but, I don't know the exact use case.
Comment 10 User image Wesley Johnston (:wesj) 2011-10-20 11:52:05 PDT
Created attachment 568461 [details] [diff] [review]
Patch v2

Now without crashing on rotation!

I had to remove my "take" stuff and replace it with a poll because, when the phone rotates the take stuff was resulting in ANR errors. Also have to dismiss and then reshow whatever prompt is up every time we are destroyed/resumed.
Comment 11 User image Wesley Johnston (:wesj) 2011-10-20 12:19:34 PDT
Created attachment 568468 [details] [diff] [review]
Patch v2.1

Forgot to attach a file.
Comment 12 User image Wesley Johnston (:wesj) 2011-10-20 18:01:23 PDT
Created attachment 568576 [details] [diff] [review]
Patch v2.2

Hmm... Apparently missed that file still.

This includes a few other little fixes to make menu lists work better and to return -1 if the user cancels a dialog.
Comment 13 User image Wesley Johnston (:wesj) 2011-10-20 20:57:50 PDT
Created attachment 568598 [details] [diff] [review]
Patch v2.3

Really sorry for the churn here. I've got whatever weird Java they ship with Ubuntu 11.10 here now which apparently lets me get away with murder.

This includes the fixes blassey pointed out:
Comment 14 User image Wesley Johnston (:wesj) 2011-10-20 21:01:16 PDT
Also, that patch now includes some magical changes to browser.js that don't need to be there. Grrr.

There shouldn't be any browser.js changes here. I yanked them out locally.
Comment 15 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-10-21 06:51:48 PDT
Comment on attachment 568598 [details] [diff] [review]
Patch v2.3

>diff --git a/embedding/android/ b/embedding/android/
>+        PromptButton(JSONObject aJsonButton) {

>+        public PromptInput(JSONObject aJsonInput) {

nit: if you have a situation where you need to caps an all-caps word (JSON), then use all caps in the variable:

aJsonFoo -> aJSONFoo

>diff --git a/embedding/android/resources/layout/dialog_checkbox.xml b/embedding/android/resources/layout/dialog_checkbox.xml

>+<CheckBox xmlns:android=""
>+          android:id="@+id/favicon"

"favicon"? Maybe "checkbox"  ?  (assuming this is the checkbox image)

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

You said this should be reverted. Just make sure it is :)

>diff --git a/mobile/components/PromptService.js b/mobile/components/PromptService.js

>   alertCheck: function alertCheck(aTitle, aText, aCheckMsg, aCheckState) {

>+    var data = this.commonPrompt(aTitle, aText, [{ label: PromptUtils.getLocaleString("OK") }], aCheckMsg, aCheckState, []);

>   confirm: function confirm(aTitle, aText) {

>+    var data = this.commonPrompt(aTitle, aText, null, "", {value: false}, []);

>   confirmCheck: function confirmCheck(aTitle, aText, aCheckMsg, aCheckState) {

>+    var data = this.commonPrompt(aTitle, aText, null, aCheckMsg, aCheckState, []);

(more in the rest of the file too)

>+  sendMessageToJava: function(aMsg) {
>+    var data = Cc[";1"].getService(Ci.nsIAndroidBridge).handleGeckoMessage(JSON.stringify({ gecko: aMsg }));
>+    Cc[";1"].getService(Ci.nsIConsoleService).logStringMessage("Result2: " + data);

Do we want to keep this debugging?

r+ with those nits fixed. I mainly looked at the JS stuff. The Java stuff looks OK to me, but dougt and blassey will provide better reviews.
Comment 16 User image Wesley Johnston (:wesj) 2011-10-24 09:09:59 PDT
Comment on attachment 568598 [details] [diff] [review]
Patch v2.3

Carrying this r+ over from the previous review.
Comment 17 User image Wesley Johnston (:wesj) 2011-10-24 10:09:13 PDT
via irc, decided to take dougt's review post landing if he wants it.

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