Move Prompt:Show handler out of handleGeckoMessage

RESOLVED FIXED in Firefox 15

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 16
All
Android
Points:
---

Firefox Tracking Flags

(firefox15 fixed, firefox16 fixed)

Details

Attachments

(1 attachment)

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
Created attachment 632822 [details] [diff] [review]
Patch
Attachment #632822 - Flags: review?(margaret.leibovic)

Comment 3

5 years ago
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
status-firefox16: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/fc8d0eefddb4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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+

Updated

5 years ago
Attachment #632822 - Flags: approval-mozilla-beta+
Landed rollup with bug 764579.

http://hg.mozilla.org/releases/mozilla-beta/rev/b04c6364ebf6
status-firefox15: --- → fixed
You need to log in before you can comment on or make changes to this bug.