Closed Bug 752905 Opened 9 years ago Closed 9 years ago

Move Prompt:Show handler out of handleGeckoMessage

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox15 fixed, firefox16 fixed)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox15 --- fixed
firefox16 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

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).
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.
Depends on: 752935
Summary: Move Prompt:Show and Accessibility:IsEnabled handlers out of handleGeckoMessage → Move Prompt:Show handler out of handleGeckoMessage
Attached patch PatchSplinter Review
Attachment #632822 - Flags: review?(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?
Attachment #632822 - Flags: review?(margaret.leibovic) → review+
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.
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/fc8d0eefddb4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 696897
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
Attachment #632822 - Flags: approval-mozilla-aurora?
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.
Attachment #632822 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #632822 - Flags: approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.