1186530 - Implement per-instance forwarding of native Java methods

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Bug 1178850 added a new framework to implement native methods. This bug will add support for instance native methods: native calls on a Java object will automatically be forwarded to calls on a C++ object that corresponds to the Java instance.

Comment 1

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

Attachment: Add support for instance native pointers;

Introduce a JNIObject class that serves as a base class for classes
that wish to use per-instance native methods. JNIObject includes a long
native pointer field that the C++ code accesses to associate the Java
object instance with a C++ object instance.

Comment 2

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

Attachment: Fix compile error when using native methods with ref parameters;

We use Ref::From() inside TypeAdapter<Ref>::ToNative to convert a raw JNI
ref argument to a Ref argument for the C++ function. However, that
generates a compile error, unless we make TypeAdapter<Ref> a friend of
Ref, because we intentionally made Ref's copy constructor private and
returning from TypeAdapter<Ref>::ToNative requires the copy constructor.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8639506 [details] [diff] [review]
Add support for instance native pointers;

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

I think we want JNIObject to be an interface, that way we don't have to force an hierarchy that may not be possible (LayerView, for instance). You could add an AbstractJNIObject to implement it for simpler cases.

::: mobile/android/base/mozglue/JNIObject.java
@@ +6,5 @@
> +    // Pointer to a WeakPtr object that refers to the native object.
> +    private long mHandle;
> +
> +    // Dispose of any reference to a native object.
> +    protected abstract void dispose();

I think this could easily conflict with other methods in the subclasses. I recommend something more specific. disposeNativeHandle() maybe?

Comment 4

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

Attachment: Add support for instance native pointers (v2)

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> Comment on attachment 8639506 [details] [diff] [review]
> Add support for instance native pointers;
> 
> Review of attachment 8639506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we want JNIObject to be an interface, that way we don't have to
> force an hierarchy that may not be possible (LayerView, for instance). You
> could add an AbstractJNIObject to implement it for simpler cases.

Discussed in 1:1 that classes like LayerView would use an inner class, which inherits from JNIObject, to implement instance native methods. Having JNIObject being an abstract class makes things on the C++ side a lot easier.

> ::: mobile/android/base/mozglue/JNIObject.java
> @@ +6,5 @@
> > +    // Pointer to a WeakPtr object that refers to the native object.
> > +    private long mHandle;
> > +
> > +    // Dispose of any reference to a native object.
> > +    protected abstract void dispose();
> 
> I think this could easily conflict with other methods in the subclasses. I
> recommend something more specific. disposeNativeHandle() maybe?

Okay, disposeNative()?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8640173 [details] [diff] [review]
Add support for instance native pointers (v2)

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

After discussing on Vidyo, Jim convinced me that the abstract class is alright.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f1a6f4ede0
https://hg.mozilla.org/integration/mozilla-inbound/rev/866e22abf371

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9f1a6f4ede0
https://hg.mozilla.org/mozilla-central/rev/866e22abf371