Implement per-GeckoView messaging

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
GeckoView
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
Firefox 53
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(16 attachments)

10.06 KB, patch
snorp
: review+
Details | Diff | Splinter Review
7.26 KB, patch
snorp
: review+
sebastian
: review+
Details | Diff | Splinter Review
15.27 KB, patch
snorp
: review+
Details | Diff | Splinter Review
27.75 KB, patch
snorp
: review+
Details | Diff | Splinter Review
2.79 KB, patch
snorp
: review+
Details | Diff | Splinter Review
7.00 KB, patch
snorp
: review+
Details | Diff | Splinter Review
14.00 KB, patch
snorp
: review+
Details | Diff | Splinter Review
12.62 KB, patch
snorp
: review+
Details | Diff | Splinter Review
19.06 KB, patch
snorp
: review+
Details | Diff | Splinter Review
23.67 KB, patch
snorp
: review+
Details | Diff | Splinter Review
14.89 KB, patch
snorp
: review+
Details | Diff | Splinter Review
9.52 KB, patch
snorp
: review+
Details | Diff | Splinter Review
2.35 KB, patch
snorp
: review+
Details | Diff | Splinter Review
10.94 KB, patch
snorp
: review+
Details | Diff | Splinter Review
33.47 KB, patch
jchen
: review+
Details | Diff | Splinter Review
23.82 KB, patch
snorp
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
We need a way to send GeckoView-specific messages from Gecko to Java and vice versa.
(Assignee)

Comment 1

2 years ago
Created attachment 8804345 [details] [diff] [review]
1. Move GeckoApp EventDispatcher to GeckoView (v1)

First step is to make it a GeckoView-specific EventDispatcher instead of
GeckoApp-specific, so that GeckoView consumers can benefit from a per-view
EventDispatcher. In addition, a few events like Gecko:Ready are moved back to
the global EventDispatcher because that makes more sense.
Attachment #8804345 - Flags: review?(snorp)
Attachment #8804345 - Flags: review?(snorp) → review+
(Assignee)

Comment 2

2 years ago
Created attachment 8805611 [details] [diff] [review]
1b. Don't use GeckoApp EventDispatcher during inflation (v1)

During layout inflation, we don't yet have GeckoView and therefore the
GeckoView EventDispatcher, so we should not register events until later,
typically during onAttachedToWindow.
Attachment #8805611 - Flags: review?(snorp)
Attachment #8805611 - Flags: review?(s.kaspari)
(Assignee)

Comment 3

2 years ago
Created attachment 8805614 [details] [diff] [review]
2. Introduce GeckoBundle (v1)

The Android Bundle class has several disadvantages when used for holding
structured data from JS.

The most obvious one is the differentiation between int and double,
which doesn't exist in JS. So when a JS number is converted to either a
Bundle int or double, we run the risk of making a wrong conversion,
resulting in a type mismatch exception when Java uses the Bundle. This
extends to number arrays from JS.

There is one more gotcha when using arrays. When we receive an empty
array from JS, there is no way for us to determine the type of the
array, because even empty arrays in Java have types. We are forced to
pick an arbitrary type like boolean[], which can easily result in a type
mismatch exception when using the array on the Java side.

In addition, Bundle is fairly cumbersome, and we cannot access the inner
structures of Bundle from Java or JNI, making it harder to use.

With these factors in mind, this patch introduces GeckoBundle as a
better choice for Gecko/Java communication. It is almost fully
API-compatible with the Android Bundle; only the Bundle array methods
are different. It resolves the numbers problem by performing conversions
if necessary, and it is a lot more lightweight than Bundle.
Attachment #8805614 - Flags: review?(snorp)
(Assignee)

Comment 4

2 years ago
Created attachment 8805615 [details] [diff] [review]
3. Convert BundleEventListener to use GeckoBundle (v1)

Convert BundleEventListener from using Bundle to using GeckoBundle.
Because NativeJSContainer still only supports Bundle, we do an extra
conversion when sending Bundle messages, but eventually, as we eliminate
the use of NativeJSContainer, that will go away as well.
Attachment #8805615 - Flags: review?(snorp)
(Assignee)

Comment 5

2 years ago
Created attachment 8805616 [details] [diff] [review]
4. Introduce EventDispatcher interfaces (v1)

Introduce several new XPCOM interfaces for the new EventDispatcher API,
these interfaces are mostly mirrored after their Java counterparts.

* nsIAndroidEventDispatcher is the main interface for
  registering/unregistering listeners and for dispatching events from
  JS/C++.

* nsIAndroidEventListener is the interface that JS/C++ clients implement
  to receive events.

* nsIAndroidEventCallback is the interface that JS/C++ clients implement
  to receive responses from dispatched events.

* nsIAndroidView is the new interface that every window receives
  that is specific to the window/GeckoView pair.
Attachment #8805616 - Flags: review?(snorp)
(Assignee)

Comment 6

2 years ago
Created attachment 8805618 [details] [diff] [review]
5. Remove EventDispatcher references from gfx code (v1)

EventDispatcher was used for JPZC, but NPZC doesn't use it anymore.
Attachment #8805618 - Flags: review?(snorp)
Attachment #8805611 - Flags: review?(s.kaspari) → review+
Comment on attachment 8805611 [details] [diff] [review]
1b. Don't use GeckoApp EventDispatcher during inflation (v1)

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

This is kind of annoying to have to do add onAttachedToWindow() everywhere, but *shrug*
Attachment #8805611 - Flags: review?(snorp) → review+
Comment on attachment 8805614 [details] [diff] [review]
2. Introduce GeckoBundle (v1)

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

This lacks the main feature of Bundle, which is that it implements Parcelable and is therefore able to be easily serialized over binder. Can you add that?
Attachment #8805614 - Flags: review?(snorp) → review+
Attachment #8805615 - Flags: review?(snorp) → review+
Attachment #8805616 - Flags: review?(snorp) → review+
Attachment #8805618 - Flags: review?(snorp) → review+
(Assignee)

Comment 9

2 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #8)
> Comment on attachment 8805614 [details] [diff] [review]
> 2. Introduce GeckoBundle (v1)
> 
> Review of attachment 8805614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This lacks the main feature of Bundle, which is that it implements
> Parcelable and is therefore able to be easily serialized over binder. Can
> you add that?

Yeah I can do that in a follow up.
(Assignee)

Comment 10

2 years ago
Created attachment 8806855 [details] [diff] [review]
6. General JNI template improvements (v1)

This patch includes several improvements to the JNI templates.

* Context::RawClassRef is removed to avoid misuse, as Context::ClassRef
  should be used instead.

* Fix a compile error, in certain usages, in the DisposeNative overload
  in NativeStub.

* Add Ref::IsInstanceOf and Context::IsInstanceOf to mirror the
  JNIEnv::IsInstanceOf call.

* Add Ref::operator* and Context::operator* to provide an easy way to
  get a Context object.

* Add built-in declarations for boxed Java objects (e.g. Boolean,
  Integer, etc).

* Add ObjectArray::New for creating new object arrays of specific types.

* Add lvalue qualifiers to LocalRef::operator= and GlobalRef::operator=,
  to prevent accidentally assigning to rvalues. (e.g.
  `objectArray->GetElement(0) = newObject;`, which won't work as intended.)
Attachment #8806855 - Flags: review?(snorp)
(Assignee)

Comment 11

2 years ago
Created attachment 8806856 [details] [diff] [review]
7. Support ownership through RefPtr for native JNI objects (v1)

In addition to direct ownership and weak pointer ownership, add a third
ownership model where a native JNI object owns a RefPtr that holds a
strong reference to the actual C++ object. This ownership model works
well with ref-counted objects such as XPCOM objects, and is activated
through the presence of public members AddRef() and Release() in the C++
object.
Attachment #8806856 - Flags: review?(snorp)
(Assignee)

Comment 12

2 years ago
Created attachment 8806857 [details] [diff] [review]
8. Implement Gecko-side EventDispatcher (v1)

Add a skeletal implementation of EventDispatcher on the Gecko side.
Each widget::EventDispatcher will be associated with a Java
EventDispatcher, so events can be dispatched from Gecko to Java and vice
versa. AndroidBridge and nsWindow will implement
nsIAndroidEventDispatcher through widget::EventDispatcher.

Other patches will add more complete functionality such as
GeckoBundle/JSObject translation and support for callbacks.
Attachment #8806857 - Flags: review?(snorp)
(Assignee)

Comment 13

2 years ago
Created attachment 8806858 [details] [diff] [review]
9. Implement dispatching between Gecko/Java (v1)

Implement translation between JSObject and GeckoBundle, and use that for
dispatching events from Gecko to Java and vice versa.
Attachment #8806858 - Flags: review?(snorp)
(Assignee)

Comment 14

2 years ago
Created attachment 8806859 [details] [diff] [review]
10. Implement callback support (v1)

Implement callback support for both Gecko-to-Java events and
Java-to-Gecko events.

For Gecko-to-Java, we translate nsIAndroidEventCallback to a Java
EventCallback through NativeCallbackDelegate and pass it to the Java
listener.

For Java-to-Gecko, we translate EventCallback to a
nsIAndroidEventCallback through JavaCallbackDelegate and pass it to the
Gecko listener.  There is another JavaCallbackDelegate on the Java side
that redirects the callback to a particular thread. For example, if the
event was dispatched from the UI thread, we make sure the callback
happens on the UI thread as well.
Attachment #8806859 - Flags: review?(snorp)
(Assignee)

Comment 15

2 years ago
Created attachment 8806860 [details] [diff] [review]
11. Add BundleEventListener support for Gecko thread (v1)

Add support for BundleEventListener on the Gecko thread, so that we can
use it to replace any existing GeckoEventListener or NativeEventListener
implementations that require the listener be run synchronously on the
Gecko thread.
Attachment #8806860 - Flags: review?(snorp)
Attachment #8806855 - Flags: review?(snorp) → review+
Attachment #8806856 - Flags: review?(snorp) → review+
Attachment #8806857 - Flags: review?(snorp) → review+
Attachment #8806858 - Flags: review?(snorp) → review+
Attachment #8806859 - Flags: review?(snorp) → review+
Attachment #8806860 - Flags: review?(snorp) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8807308 [details] [diff] [review]
12. Add global EventDispatcher in AndroidBridge (v1)

Add an instance of EventDispatcher to AndroidBridge to act as a global
event dispatcher.
Attachment #8807308 - Flags: review?(snorp)
(Assignee)

Comment 17

2 years ago
Created attachment 8807309 [details] [diff] [review]
13. Add per-nsWindow EventDispatcher (v1)

Add an instance of EventDispatcher to each nsWindow through an
AndroidView object, which implements nsIAndroidView. The nsIAndroidView
is passed to the chrome script through the window argument when opening
the window.
Attachment #8807309 - Flags: review?(snorp)
(Assignee)

Comment 18

2 years ago
Created attachment 8807310 [details] [diff] [review]
14. Update auto-generated bindings (v1)
Attachment #8807310 - Flags: review+
(Assignee)

Comment 19

2 years ago
Created attachment 8807311 [details] [diff] [review]
15. Update testEventDispatcher (v1)

Update testEventDispatcher to include new functionalities in
EventDisptcher.

* Add tests for dispatching events to UI/background thread through
  nsIAndroidEventDispatcher::dispatch.

* Add tests for dispatching events to UI/background thread through
  EventDispatcher.dispatch.

* Add tests for dispatching events to Gecko thread through
  EventDispatcher.dispatch.

Each kind of test exercises both the global EventDispatcher through
EventDispatcher.getInstance() and the per-GeckoView EventDispatcher
through GeckoApp.getEventDispatcher().
Attachment #8807311 - Flags: review?(snorp)
Attachment #8807308 - Flags: review?(snorp) → review+
Attachment #8807309 - Flags: review?(snorp) → review+
Attachment #8807311 - Flags: review?(snorp) → review+

Comment 20

2 years ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4b29e6e5bd
Implement per-GeckoView messaging; r=snorp r=sebastian
(Assignee)

Updated

2 years ago
Blocks: 1317604

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c4b29e6e5bd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Updated

2 years ago
Depends on: 1319173
(Assignee)

Updated

2 years ago
Blocks: 1319558
You need to log in before you can comment on or make changes to this bug.