1012722 - Switch GeckoApp/BrowserApp listeners to using NativeJSObject

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Switch most GeckoApp event listeners to native (v1)

This patch switches most of the event listeners in GeckoApp to using NativeJSObject. A few events are not switched over because switching is more complicated. For example, events used by Robocop (Gecko:Ready, etc) are not switched because Robocop doesn't support NativeJSObject yet. I want to work on these events in follow-up bugs.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Switch most BrowserApp event listeners to native (v1)

Same for BrowserApp

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8425616 [details] [diff] [review]
Switch most GeckoApp event listeners to native (v1)

>     /**
>      * @param aPermissions
>      *        Array of JSON objects to represent site permissions.
>      *        Example: { type: "offline-app", setting: "Store Offline Data", value: "Allow" }
>      */
>-    private void showSiteSettingsDialog(String aHost, JSONArray aPermissions) {
>+    private void showSiteSettingsDialog(final String aHost, final NativeJSObject[] aPermissions) {

Since you are making changes here, could you remove the "a" prefixes? Looks like this function is an odd-ball in the file.

>-            "NativeApp:IsDebuggable",
>-            "SystemUI:Visibility");
>+        EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this,
>+                "Gecko:Ready",
>+                "Gecko:DelayedStartup",

Did you mean to indent two "blocks" ? One 4-space indent seems typical.

>+        EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener)this,
>+                "Accessibility:Ready",
>+                "Bookmark:Insert",

Same

>-            "NativeApp:IsDebuggable",
>-            "SystemUI:Visibility");
>+        EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener)this,
>+                "Gecko:Ready",
>+                "Gecko:DelayedStartup",
>+                "Accessibility:Event",
>+                "NativeApp:IsDebuggable");
>+
>+        EventDispatcher.getInstance().unregisterGeckoThreadListener((NativeEventListener)this,
>+                "Accessibility:Ready",
>+                "Bookmark:Insert",

Same

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8425619 [details] [diff] [review]
Switch most BrowserApp event listeners to native (v1)

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>         EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this,
>-            "CharEncoding:Data",
>-            "CharEncoding:State",
>+                "Menu:Update",
>+                "Reader:Added",

Indents again

>+        EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener)this,
>+                "Accounts:Create",
>+                "CharEncoding:Data",

Same


>         EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener)this,
>-            "CharEncoding:Data",
>-            "CharEncoding:State",
>-            "Feedback:LastUrl",
>+                "Menu:Update",
>+                "Reader:Added",

Same

>+        EventDispatcher.getInstance().unregisterGeckoThreadListener((NativeEventListener)this,
>+                "Accounts:Create",
>+                "CharEncoding:Data",
>+                "CharEncoding:State",

Same

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8425616 [details] [diff] [review]
> Switch most GeckoApp event listeners to native (v1)
> 
> >-            "NativeApp:IsDebuggable",
> >-            "SystemUI:Visibility");
> >+        EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this,
> >+                "Gecko:Ready",
> >+                "Gecko:DelayedStartup",
> 
> Did you mean to indent two "blocks" ? One 4-space indent seems typical.

I remember someone talking about the style being 2 indents for overhanging lines, but I agree I usually see 1 indent being used.


https://hg.mozilla.org/integration/fx-team/rev/bfcd487240a0
https://hg.mozilla.org/integration/fx-team/rev/d151cf4059fa

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bfcd487240a0
https://hg.mozilla.org/mozilla-central/rev/d151cf4059fa