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


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

Description Kartikaya Gupta (email:kats@mozilla.com) 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 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 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-13 12:49:59 PDT
Created attachment 632822 [details] [diff] [review]
Patch
Comment 3 :Margaret Leibovic 2012-06-13 14:03:58 PDT
Comment on attachment 632822 [details] [diff] [review]
Patch

Nice, I like this decoupling.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>@@ -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 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-13 14:16:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8d0eefddb4

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

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 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-15 19:47:44 PDT
Comment on attachment 632822 [details] [diff] [review]
Patch

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 Brian Nicholson (:bnicholson) 2012-07-18 21:08:56 PDT
Landed rollup with bug 764579.

http://hg.mozilla.org/releases/mozilla-beta/rev/b04c6364ebf6

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