Last Comment Bug 752905 - Move Prompt:Show handler out of handleGeckoMessage
: Move Prompt:Show handler out of handleGeckoMessage
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
-- normal (vote)
: Firefox 16
Assigned To: Kartikaya Gupta (
: Sebastian Kaspari (:sebastian)
: 696897 (view as bug list)
Depends on: 752935
  Show dependency treegraph
Reported: 2012-05-08 07:38 PDT by Kartikaya Gupta (
Modified: 2012-07-18 21:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (9.34 KB, patch)
2012-06-13 12:49 PDT, Kartikaya Gupta (
margaret.leibovic: review+
mark.finkle: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Kartikaya Gupta ( 2012-05-08 07:38:51 PDT
Currently the GeckoAppShell.handleGeckoMessage has special code for a couple of events. This should be moved out and made to use the general GeckoEventListener/GeckoEventResponder framework both for modularity and for performance (doing unnecessary string creation/comparison for each message isn't good).
Comment 1 User image Eitan Isaacson [:eeejay] 2012-05-08 09:05:23 PDT
I would like to use this opportunity to have the accessibility check be a java->js message. So I opened bug 752935, the patch there should remove the offending Accessibility:IsEnabled block from GeckoAppShell.handleGeckoMessage and make a GeckoEventListener.
Comment 2 User image Kartikaya Gupta ( 2012-06-13 12:49:59 PDT
Created attachment 632822 [details] [diff] [review]
Comment 3 User image :Margaret Leibovic 2012-06-13 14:03:58 PDT
Comment on attachment 632822 [details] [diff] [review]

Nice, I like this decoupling.

>diff --git a/mobile/android/base/ b/mobile/android/base/
>@@ -2687,17 +2690,17 @@ abstract public class GeckoApp

>         public void run() {
>-            GeckoAppShell.getPromptService().Show(mTitle, "", null, mItems, false);
>+            mPromptService.Show(mTitle, "", null, mItems, false);

Nit: Not your bug, but can we make this Show method lowercase? Is there any reason it should be uppercase?
Comment 4 User image Kartikaya Gupta ( 2012-06-13 14:16:42 PDT

I filed bug 764579 for /Show/show/ since I didn't want to lump it in here.
Comment 5 User image Ed Morley [:emorley] 2012-06-14 02:46:30 PDT
Comment 6 User image Wesley Johnston (:wesj) 2012-06-18 10:32:30 PDT
*** Bug 696897 has been marked as a duplicate of this bug. ***
Comment 7 User image Brian Nicholson (:bnicholson) 2012-07-15 13:46:09 PDT
Comment on attachment 632822 [details] [diff] [review]

Needed to land bug 769269.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Comment 8 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-07-15 19:47:44 PDT
Comment on attachment 632822 [details] [diff] [review]

This patch itself has been baking awhile and is low risk. I would not take the patch if it wasn't needed to make uplifting bug 769269 easier.
Comment 9 User image Brian Nicholson (:bnicholson) 2012-07-18 21:08:56 PDT
Landed rollup with bug 764579.

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