694455 - [birch] Android Prompt Service

Status: RESOLVED FIXED

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

Description

[Wesley Johnston (:wesj)]

Attachment: 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

[Lucas Rocha (:lucasr)]

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

[Wesley Johnston (:wesj)]

Attachment: WIP 2

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

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

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

[Lucas Rocha (:lucasr)]

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

[Wesley Johnston (:wesj)]

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 7

[Wesley Johnston (:wesj)]

Attachment: 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

[Wesley Johnston (:wesj)]

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

[Sriram Ramasubramanian [:sriram]]

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

[Wesley Johnston (:wesj)]

Attachment: 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

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Forgot to attach a file.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: 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

[Wesley Johnston (:wesj)]

Attachment: 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:
http://dump.lassey.us/make_wes_build.patch

Comment 14

[Wesley Johnston (:wesj)]

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

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

Comment on attachment 568598 [details] [diff] [review]
Patch v2.3


>diff --git a/embedding/android/PromptService.java b/embedding/android/PromptService.java
>+        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="http://schemas.android.com/apk/res/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, []);

Y U NO LIKE 'LET' ?
(more in the rest of the file too)

>+  sendMessageToJava: function(aMsg) {
>+    var data = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge).handleGeckoMessage(JSON.stringify({ gecko: aMsg }));
>+    Cc["@mozilla.org/consoleservice;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

[Wesley Johnston (:wesj)]

Comment on attachment 568598 [details] [diff] [review]
Patch v2.3

Carrying this r+ over from the previous review.

Comment 17

[Wesley Johnston (:wesj)]

via irc, decided to take dougt's review post landing if he wants it.

http://hg.mozilla.org/projects/birch/rev/859fc2a73a59