The default bug view has changed. See this FAQ.

[birch] Android Prompt Service

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({feature})

unspecified
All
Android
feature
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

6 years ago
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.
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?
Assignee: nobody → wjohnston
OS: All → Android
(Assignee)

Comment 2

6 years ago
Created attachment 567234 [details] [diff] [review]
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.
Attachment #566973 - Attachment is obsolete: true
Attachment #567234 - Flags: feedback?(mark.finkle)
Attachment #567234 - Flags: feedback?(doug.turner)
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.
Component: General → General
Product: Fennec → Fennec Native
Version: Trunk → unspecified
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.
(Assignee)

Comment 5

6 years ago
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).
Duplicate of this bug: 695179
Priority: -- → P1
(Assignee)

Comment 7

6 years ago
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.
Attachment #567234 - Attachment is obsolete: true
Attachment #567234 - Flags: feedback?(mark.finkle)
Attachment #567234 - Flags: feedback?(doug.turner)
Attachment #568153 - Flags: review?(mark.finkle)
Attachment #568153 - Flags: review?(doug.turner)
Attachment #568153 - Flags: review?(blassey.bugs)
(Assignee)

Comment 8

6 years ago
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 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.
(Assignee)

Comment 10

6 years ago
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.
Attachment #568153 - Attachment is obsolete: true
Attachment #568153 - Flags: review?(mark.finkle)
Attachment #568153 - Flags: review?(doug.turner)
Attachment #568153 - Flags: review?(blassey.bugs)
Attachment #568461 - Flags: review?(mark.finkle)
Attachment #568461 - Flags: review?(doug.turner)
Attachment #568461 - Flags: review?(blassey.bugs)
(Assignee)

Comment 11

6 years ago
Created attachment 568468 [details] [diff] [review]
Patch v2.1

Forgot to attach a file.
Attachment #568461 - Attachment is obsolete: true
Attachment #568461 - Flags: review?(mark.finkle)
Attachment #568461 - Flags: review?(doug.turner)
Attachment #568461 - Flags: review?(blassey.bugs)
Attachment #568468 - Flags: review?(mark.finkle)
Attachment #568468 - Flags: review?(doug.turner)
Attachment #568468 - Flags: review?(blassey.bugs)
Attachment #568468 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 12

6 years ago
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.
Attachment #568468 - Attachment is obsolete: true
Attachment #568468 - Flags: review?(mark.finkle)
Attachment #568468 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Attachment #568576 - Flags: review?(mark.finkle)
Attachment #568576 - Flags: review?(doug.turner)
Attachment #568576 - Flags: review?(blassey.bugs)
(Assignee)

Comment 13

6 years ago
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:
http://dump.lassey.us/make_wes_build.patch
Attachment #568576 - Attachment is obsolete: true
Attachment #568576 - Flags: review?(mark.finkle)
Attachment #568576 - Flags: review?(doug.turner)
Attachment #568576 - Flags: review?(blassey.bugs)
Attachment #568598 - Flags: review?(mark.finkle)
Attachment #568598 - Flags: review?(doug.turner)
Attachment #568598 - Flags: review?(blassey.bugs)
(Assignee)

Comment 14

6 years ago
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 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.
Attachment #568598 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 16

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

Carrying this r+ over from the previous review.
Attachment #568598 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 17

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

http://hg.mozilla.org/projects/birch/rev/859fc2a73a59
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Attachment #568598 - Flags: review?(doug.turner)
tracking-fennec: --- → 11+
status-firefox11: --- → fixed

Updated

5 years ago
Keywords: feature
You need to log in before you can comment on or make changes to this bug.