752905 - Move Prompt:Show handler out of handleGeckoMessage

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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]]

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:[e-mail])]

Attachment: Patch

Comment 3

[:Margaret Leibovic]

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:[e-mail])]

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]]

https://hg.mozilla.org/mozilla-central/rev/fc8d0eefddb4

Comment 7

[Brian Nicholson (:bnicholson)]

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?)]

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)]

Landed rollup with bug 764579.

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