Closed Bug 989046 Opened 6 years ago Closed 6 years ago

Add NativeJSObject support in the event dispatcher

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(8 files)

Goal is to support NativeJSObject while retaining JSON support for backward compatibility.
This patch changes the handleGeckoMessage method in nsIAndroidBridge from taking a string argument to taking a JS value argument.

The new handleGeckoMessage implementation still checks if the passed value is a string, and if it is, assume it's JSON and convert it to an object. This is provided for backwards compatibility with any addons that still pass strings to handleGeckoMessage. Internally, all callers of handleGeckoMessage will be converted to passing JS objects directly.
Attachment #8399691 - Flags: review?(blassey.bugs)
This patch changes all callers of nsIAndroidBridge.handleGeckoMessage to pass in JS objects instead of JSON strings.
Attachment #8399692 - Flags: review?(mark.finkle)
This patch rewrites a lot of EventDispatcher. It adds support for the new native JS objects while keeping support for existing JSON events. The new API also has some features like support for registering multiple events at once.

Separate HashMaps are used to keep track of native and JSON events to reduce memory usage. To keep track of both types of events in one map, we'd need a separate inner class with multiple fields, which is memory inefficient.
Attachment #8399701 - Flags: review?(mark.finkle)
Comment on attachment 8399691 [details] [diff] [review]
a. Convert Gecko side messaging to use NativeJSContainer;

Review of attachment 8399691 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/AndroidBridge.h
@@ +280,5 @@
>      void ReleaseNativeWindowForSurfaceTexture(void *window);
>  
>      bool LockWindow(void *window, unsigned char **bits, int *width, int *height, int *format, int *stride);
>      bool UnlockWindow(void *window);
>      

extra ws here
Attachment #8399691 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8399692 [details] [diff] [review]
b. Drop JSON.stringify when calling handleGeckoMessage;

>--- a/accessible/src/jsat/AccessFu.jsm

>       let bridgeCc = Cc['@mozilla.org/android/bridge;1'];
>       bridgeCc.getService(Ci.nsIAndroidBridge).handleGeckoMessage(
>-          JSON.stringify({ type: 'Accessibility:Ready' }));
>+          { type: 'Accessibility:Ready' });

Could I get you to change this to use: Services.androidBridge.handleGeckoMessage ?

>         if (Utils.MozBuildApp == 'mobile/android')
>           // Return focus to native Android browser chrome.
>           Cc['@mozilla.org/android/bridge;1'].
>             getService(Ci.nsIAndroidBridge).handleGeckoMessage(
>-              JSON.stringify({ type: 'ToggleChrome:Focus' }));
>+              { type: 'ToggleChrome:Focus' });

Services.androidBridge.handleGeckoMessage


>diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js

>       Cc["@mozilla.org/android/bridge;1"].
>         getService(Ci.nsIAndroidBridge).
>-        handleGeckoMessage(JSON.stringify(message));
>+        handleGeckoMessage(message);

Services.androidBridge

>   Cc["@mozilla.org/android/bridge;1"].
>     getService(Ci.nsIAndroidBridge).
>-    handleGeckoMessage(JSON.stringify(message));
>+    handleGeckoMessage(message);

Services.androidBridge
Attachment #8399692 - Flags: review?(mark.finkle) → review+
Comment on attachment 8399701 [details] [diff] [review]
c. Support multiple listener types in EventDispatcher (v1)

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

>+    public void dispatchEvent(final JSONObject json) {

>+            final JSONObject message;
>+            if (!json.has("type")) {
>+                message = json.getJSONObject("gecko");
>+            } else {
>+                message = json;

Maybe it's time to kill support for the old "gecko" object. We have been carrying this around long enough. I am OK if your remove it.

I wouldn't mind having Wes take a quick look too.
Attachment #8399701 - Flags: review?(wjohnston)
Attachment #8399701 - Flags: review?(mark.finkle)
Attachment #8399701 - Flags: review+
Wes, patch (a) takes out the changes from bug 981852. Just ping me if you want to discuss it more. Thanks!
Duplicate of this bug: 987985
In some tests the event is being unregistered before being registered. Adding a registration on startup fixes it.
Attachment #8400085 - Flags: review?(liuche)
In some tests, we could be unregistering the event before the event was registered. Adding a check fixes it.
Attachment #8400088 - Flags: review?(markcapella)
In some tests, we have a blur before a focus, and we try to unregister the event in blur, before the event was ever registered. Adding a focused flag fixes it.
Attachment #8400090 - Flags: review?(markcapella)
Comment on attachment 8400088 [details] [diff] [review]
e. Only unregister event in FindInPageBar if already registered (v1)

wfm :)
Attachment #8400088 - Flags: review?(markcapella) → review+
Comment on attachment 8400090 [details] [diff] [review]
f. Only unregister event in GeckoEditable if already registered (v1)

Interesting! (dup thread flags)
Attachment #8400090 - Flags: review?(markcapella) → review+
In some tests, we try to uninitialize before we successfully initialize BrowserHealthRecorder, which means we try to unregister the events before they were ever registered. Adding a check fixes it.
Attachment #8400097 - Flags: review?(rnewman)
We're running into several "unregister before register" assertions on try. I think to be safe we should make it throw only on nightly/aurora. This patch also adds a check for already-registered events when registering.
Attachment #8400099 - Flags: review?(mark.finkle)
Comment on attachment 8400097 [details] [diff] [review]
g. Only unregister event in BrowserHealthRecorder if already registered (v1)

Review of attachment 8400097 [details] [diff] [review]:
-----------------------------------------------------------------

N.B., you seem to have an extra "g." in the commit message :D
Attachment #8400097 - Flags: review?(rnewman) → review+
Comment on attachment 8400085 [details] [diff] [review]
d. Register prompt event on startup (v1)

Review of attachment 8400085 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think it's necessary to register a listener for Prompt:ShowTop here; we use this listener so we can foreground BrowserApp if the Activity needs to be brought to the front - see its register/unregister in onPause and onResume. Basically, we want to listen for this when BrowserApp is not in the foreground.

Anyways, I think this patch is unnecessary.
Attachment #8400085 - Flags: review?(liuche) → review-
Comment on attachment 8400085 [details] [diff] [review]
d. Register prompt event on startup (v1)

Sorry I should have clarified more. Basically the issue is we unregister the event in onResume, even the first time that onResume is called before onPause was ever called. In that case we try to unregister an event (in onResume), even though we have *not* registered the event before (in onPause).

When that happens, it used to be a warning, but now it's an error. To fix it, we can add a check to skip the unregistering in onResume in case onPause has not been called before. Or we can just register the event in onCreate, which will be immediately unregistered in onResume later on in the startup sequence.
Attachment #8400085 - Flags: review- → review?(liuche)
Comment on attachment 8400099 [details] [diff] [review]
h. Be more strict about event registration in non-release versions (v1)

We might find some assertions in GeckoView examples using this too
Attachment #8400099 - Flags: review?(mark.finkle) → review+
Comment on attachment 8400085 [details] [diff] [review]
d. Register prompt event on startup (v1)

Review of attachment 8400085 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I see, that's a good point. Thanks for the clarification!
Attachment #8400085 - Flags: review?(liuche) → review+
Comment on attachment 8399701 [details] [diff] [review]
c. Support multiple listener types in EventDispatcher (v1)

Review of attachment 8399701 [details] [diff] [review]:
-----------------------------------------------------------------

I missed this boat apparently. I'm still curious why we're using such heavyweight locking, and I really don't like this checkNotRegistered method so I'll put this up.

::: mobile/android/base/EventDispatcher.java
@@ +61,5 @@
>          }
>      }
>  
> +    private <T> void checkNotRegistered(final Map<String, List<T>> listenersMap,
> +                                        final String[] events) {

This method seems... confusing to me. It throws if the map contains any of these events? Can you name it something like throwIfRegistered, but also... throwing confuses me. It implies something has gone wrong in this function, when really its run just fine. Can it just return a boolean? Or maybe a little documentation note at the top would make it clearer to me.

@@ +62,5 @@
>      }
>  
> +    private <T> void checkNotRegistered(final Map<String, List<T>> listenersMap,
> +                                        final String[] events) {
> +        synchronized (listenersMap) {

:( Locking on the entire map feels awful. Can we use ConcurrentHashMap here?

@@ +64,5 @@
> +    private <T> void checkNotRegistered(final Map<String, List<T>> listenersMap,
> +                                        final String[] events) {
> +        synchronized (listenersMap) {
> +            for (final String event: events) {
> +                if (listenersMap.get(event) != null) {

Why not use containsKey? If there's a reason, can you document it?

@@ +80,5 @@
> +            for (final String event : events) {
> +                List<T> listeners = listenersMap.get(event);
> +                if (listeners == null ||
> +                    !listeners.remove(listener)) {
> +                    throw new IllegalArgumentException(event + " was not registered");

I wouldn't throw an IllegalArgumentException here either. It doesn't feel right to say the argument was illegal to me. Neither the List or AbstractMap interfaces throw in this type of situation either.

@@ +88,5 @@
> +    }
> +
> +    @SuppressWarnings("unchecked")
> +    public void registerGeckoThreadListener(final NativeEventListener listener,
> +                                            final String... events) {

We wanted something more generic here if we wound up doing this. i.e. Pass in a thread with your register call rather than having separate methods for every thread you might want to dispatch on. But we've also talked about just having a separate thread/threadpool for this.

@@ +183,1 @@
>                  Log.w(LOGTAG, "Message '" + type + "' has deprecated 'gecko' property!");

Can you just do this up above in the !json.has("type") branch and avoid this string compare for every message? I'd be happy if you wound up removing this too though.

@@ +192,3 @@
>                  return;
>              }
> +            for (final GeckoEventListener listener : listeners) {

add a blank line before this.
Attachment #8399701 - Flags: review?(wjohnston) → review-
Depends on: 992890
Depends on: 993084
Depends on: 993195
No longer depends on: 993084
Depends on: 1021443
You need to log in before you can comment on or make changes to this bug.