Status

()

defect
P1
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({feature})

unspecified
All
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Assignee

Description

8 years ago
Posted file WIP Patch (obsolete) —
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

8 years ago
Posted patch WIP 2 (obsolete) — Splinter 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.
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.
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

8 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

8 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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

8 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

8 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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

8 years ago
Posted patch Patch v2.1 (obsolete) — Splinter Review
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

8 years ago
Posted patch Patch v2.2 (obsolete) — Splinter Review
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

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

Comment 13

8 years ago
Posted patch Patch v2.3Splinter Review
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

8 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

8 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

8 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
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #568598 - Flags: review?(doug.turner)
tracking-fennec: --- → 11+
Keywords: feature
You need to log in before you can comment on or make changes to this bug.