Implement per-instance forwarding of native Java methods

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Attachment #8639506 - Flags: review?(snorp)
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.
Attachment #8639507 - Flags: review?(snorp)
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?
Attachment #8639506 - Flags: review?(snorp) → review-
Attachment #8639507 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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()?
Attachment #8639506 - Attachment is obsolete: true
Attachment #8640173 - Flags: review?(snorp)
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.
Attachment #8640173 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/d9f1a6f4ede0
https://hg.mozilla.org/mozilla-central/rev/866e22abf371
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.