Bug 57859 - Removes V8's JavaInstanceJobject, JavaClassJobject and JavaFieldJobject.
Summary: Removes V8's JavaInstanceJobject, JavaClassJobject and JavaFieldJobject.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on: 55383 55786
Blocks: 57230
  Show dependency treegraph
 
Reported: 2011-04-05 10:19 PDT by Steve Block
Modified: 2011-08-09 05:00 PDT (History)
5 users (show)

See Also:


Attachments
Patch (20.12 KB, patch)
2011-04-05 11:06 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (28.01 KB, patch)
2011-04-06 05:52 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (26.38 KB, patch)
2011-07-29 12:54 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch 3 (30.00 KB, patch)
2011-07-29 13:03 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2011-04-05 10:19:42 PDT
This is required to add Chromium WebKit API methods for injecting Java objects into JavaScript. See Bug 57230.
Comment 1 Steve Block 2011-04-05 11:06:34 PDT
Created attachment 88281 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-04-05 11:12:02 PDT
Comment on attachment 88281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88281&action=review

> Source/WebCore/bridge/jni/v8/JavaFieldProxyV8.h:51
> +    static PassRefPtr<JavaFieldProxy> create(int id, String name, String typeClassName) { return adoptRef(new JavaFieldProxy(id, name, typeClassName)); }
> +
> +    // JavaField implementation
> +    virtual String name() const { return m_name; }
> +    virtual const char* typeClassName() const { return m_typeClassName.utf8().data(); }
> +    virtual JavaType type() const { return javaTypeFromClassName(m_typeClassName.utf8().data()); }
> +
> +    int id() const { return m_id; }

Can you move these out of a decl? You can place them in the header below the class if you want to inline them.

> Source/WebCore/bridge/jni/v8/JavaInstanceProxyV8.h:50
> +    virtual JavaClass* getClass() const { return &m_class; }
> +    virtual JavaValue invokeMethod(const JavaMethod* method, JavaValue* args) { return invokeMethod(static_cast<const JavaMethodProxy*>(method), args); }
> +    virtual JavaValue getField(const JavaField* field) { return getField(static_cast<const JavaFieldProxy*>(field)); }
> +    virtual void begin() {}
> +    virtual void end() {}
> +
> +    void addMethod(int id, bool isStatic, String name, String returnTypeClassName, const Vector<String> &parameters) { return m_class.addMethod(id, isStatic, name, returnTypeClassName, parameters); }
> +    void addField(int id, String name, String typeClassName) { m_class.addField(id, name, typeClassName); }

Ditto.
Comment 3 Alexey Proskuryakov 2011-04-05 21:27:07 PDT
Could you please describe the reasons in a little more detail?

Java objects are obviously exposed to JavaScript already, so why is another layer of indirection needed?
Comment 4 Steve Block 2011-04-06 05:44:58 PDT
> Can you move these out of a decl? You can place them in the header below the class if you want to inline them.
Done

> Ditto.
Done
Comment 5 Steve Block 2011-04-06 05:52:39 PDT
Created attachment 88402 [details]
Patch
Comment 6 Steve Block 2011-04-06 06:32:07 PDT
> Could you please describe the reasons in a little more detail?
There's a description in Bug 57230, the umbrella bug for this feature.
Comment 7 Alexey Proskuryakov 2011-04-06 08:46:25 PDT
> There's a description in Bug 57230, the umbrella bug for this feature.

I don't think that this is a correct approach. A remote object has pretty much nothing in common with a JavaObject implemented over JNI - but it's almost the same as a remote NPAPI object. In fact, Java is gravitating towards being implemented via an NPAPI plug-in, and the new version of JNI is tailored to that.

What you need seems to be implementing remote objects in Java, which is OK, but they don't need to be JavaObjects on WebCore side.

It seems that you've been working in this direction for a while - I'm sorry that I only realized it now what you are trying to do.

+    // FIXME: This ugliness is required because we're not able to use OwnPtr in
+    // a HashMap. An alternative would be to use RefPtr, but making JavaMethod

You can probably use deleteAllValues() here.
Comment 8 Steve Block 2011-04-06 09:06:54 PDT
> What you need seems to be implementing remote objects in Java, which is OK, but
> they don't need to be JavaObjects on WebCore side.
I considered an approach like this. Rather than use WebCore's JavaInstance etc, we could expose an injected NPAPI object directly to the embedder.

The advantage of going though JavaInstance is that we can re-use all of the existing JavaScript-Java conversions already in WebCore. Exposing an NPAPI object to the embedder would mean re-implementing all of this logic for each platform. Similarly, JavaInstance provides a common interface for both JSC and V8.
Comment 9 Andrei Popescu 2011-04-07 07:31:11 PDT
(In reply to comment #8)
> > What you need seems to be implementing remote objects in Java, which is OK, but
> > they don't need to be JavaObjects on WebCore side.
> I considered an approach like this. Rather than use WebCore's JavaInstance etc, we could expose an injected NPAPI object directly to the embedder.
> 
> The advantage of going though JavaInstance is that we can re-use all of the existing JavaScript-Java conversions already in WebCore. Exposing an NPAPI object to the embedder would mean re-implementing all of this logic for each platform. Similarly, JavaInstance provides a common interface for both JSC and V8.

This makes sense to me given that re-writing the conversion logic can be quite an error-prone process. If Alexey has no objections to continuing with this approach, this change looks otherwise fine to me.
Comment 10 Alexey Proskuryakov 2011-04-07 09:28:26 PDT
I think that implementation of remote Java objects should either be common with other remote objects, or it should at least be similar to how out of process NPAPI plug-ins are implemented.

The former can be challenging, especially since some of out of process NPAPI plug-in code is not in open source. But the latter is something that should be considered if we don't want our heads explode.

I can see several differences with NPAPI here:

1) WebKit IPC is implemented in WebKit, not in WebCore, so NPAPI remote objects also live in WebKit. With WebKit2, that is now cross-platform of course (for all platforms that adopt WebKit2). This distinction may seem minor, but I don't see what other platforms would do with proxy Java classes that are being added to WebCore here. And if it's only for Android, it should live in Android WebKit.

2) NPAPI objects don't have a common class for local and remote objects. I don't see any reason for Java to have a common class other than code sharing - but inheritance is an anti-pattern for code sharing. Since these objects behave so differently, I think that JavaInstance and ProxyJavaInstance should be separate JSC::Bindings::Instance subclasses. This objection is more about bug 55383, in fact.

3) If I'm reading this patch correctly, it uses yet another level of inheritance to actually implement IPC (e.g. invokeMethod is pure virtual). This use of inheritance to supply an implementation is against common WebKit practices - we would have a separate class to provide implementation. See e.g. the relationship between ProxyInstance and NetscapePluginInstanceProxy in WebKit/mac.

I'm not saying that out of process NPAPI implementation in WebKit/mac is necessarily the ideal, but it's the most modern code we have for Instance subclasses. If you prefer to do something differently, we should at least agree that the same would be better for NPAPI in the future.
Comment 11 Andrei Popescu 2011-04-07 11:22:37 PDT
Hi Alexey,

Thanks for the comments!

(In reply to comment #10)
> I think that implementation of remote Java objects should either be common with other remote objects, or it should at least be similar to how out of process NPAPI plug-ins are implemented.
>

Since we haven't adopted WebKit2 but we do want remote Java objects, would it be possible for us to make the implementation of remote Java objects be common with other remote objects? As far as I can tell, that would not work for us, so perhaps the best thing to do is your second choice: follow a somewhat similar pattern but keep them different?
 
> The former can be challenging, especially since some of out of process NPAPI plug-in code is not in open source. But the latter is something that should be considered if we don't want our heads explode.
> 

Heh, agreed :)

> I can see several differences with NPAPI here:
> 
> 1) WebKit IPC is implemented in WebKit, not in WebCore, so NPAPI remote objects also live in WebKit. With WebKit2, that is now cross-platform of course (for all platforms that adopt WebKit2). This distinction may seem minor, but I don't see what other platforms would do with proxy Java classes that are being added to WebCore here. And if it's only for Android, it should live in Android WebKit.
> 

Ok so how about we just leave the JavaInstance be an interface whose header lives in WebCore/bridge/v8? We can then have the proxy class live in WebKit?

> 2) NPAPI objects don't have a common class for local and remote objects. I don't see any reason for Java to have a common class other than code sharing - but inheritance is an anti-pattern for code sharing. Since these objects behave so differently, I think that JavaInstance and ProxyJavaInstance should be separate JSC::Bindings::Instance subclasses. This objection is more about bug 55383, in fact.
> 

But I think they can't be subclasses of JSC::Bindings::Instance since the Instance class is JSC-specific and we use V8. We inject our instances via NPAPI, which is just a V8-specific detail. This is why we have two JavaInstance classes: one in bridge/jsc and one in bridge/v8. 

> 3) If I'm reading this patch correctly, it uses yet another level of inheritance to actually implement IPC (e.g. invokeMethod is pure virtual). This use of inheritance to supply an implementation is against common WebKit practices - we would have a separate class to provide implementation. See e.g. the relationship between ProxyInstance and NetscapePluginInstanceProxy in WebKit/mac.
> 
> I'm not saying that out of process NPAPI implementation in WebKit/mac is necessarily the ideal, but it's the most modern code we have for Instance subclasses. If you prefer to do something differently, we should at least agree that the same would be better for NPAPI in the future.


If I understand things right:  since the V8-specific JavaInstance isn't really an Instance subclass, is this the right goal here? Instead, we can have the JavaInstance be a pure interface and have the implementations live in WebKit...

Thanks,
Andrei
Comment 12 Alexey Proskuryakov 2011-04-07 11:38:44 PDT
> If I understand things right:  since the V8-specific JavaInstance isn't really an Instance subclass, is this the right goal here?

Indeed, I forgot about that.

What does v8 do for NPAPI plug-ins? Those didn't involve making CInstance an interface class.
Comment 13 Andrei Popescu 2011-04-07 12:05:52 PDT
(In reply to comment #12)
> > If I understand things right:  since the V8-specific JavaInstance isn't really an Instance subclass, is this the right goal here?
> 
> Indeed, I forgot about that.
> 
> What does v8 do for NPAPI plug-ins? Those didn't involve making CInstance an interface class.

There's a separate implementation that maps between NPObjects and v8::Objects here:

http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/NPV8Object.cpp
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/npruntime.cpp
etc

Thanks,
Andrei
Comment 14 Eric Seidel (no email) 2011-05-23 15:05:09 PDT
Comment on attachment 88402 [details]
Patch

This hasn't been touched in over 3 weeks.  Looks like there are still concerns above.  Marking r-.

I would love it if someone would go through the exising JNI code and give it a bath... Our second-most common cause of flaky tests is something to do with liveconnect objects toString implementations being racy or something...
Comment 15 Steve Block 2011-07-19 10:48:19 PDT
I'll look into the approach of keeping JavaInstanceV8.h in WebCore, with implementations provided in the WebKit layer. But first I'll fix Bug 55786 to simplify the refactoring.
Comment 16 Steve Block 2011-07-29 12:54:22 PDT
Created attachment 102382 [details]
Patch
Comment 17 Steve Block 2011-07-29 13:00:46 PDT
I've uploaded a new patch which removes V8's JavaInstanceJobject, JavaClassJobject and JavaFieldJobject, which were added in Bug 55383, Bug 57533 and Bug 55766.

These classes were only used by Android. In the future, Android will use the Chromium WebKit API to implement the Java bridge.

The patch also simplifies the JavaInstance, JavaClass and JavaField and JavaMethod interfaces for V8. Following Alexey's comments, these will be implemented in the Chromium WebKit layer in a later patch.

I'll also send a couple of patches to clean up JavaMethodJobject and JavaString, which are now only needed for JSC.
Comment 18 Steve Block 2011-07-29 13:03:31 PDT
Created attachment 102384 [details]
Patch 3

Previous patch seems to have been mangled during upload
Comment 19 Alexey Proskuryakov 2011-08-08 15:58:45 PDT
Comment on attachment 102384 [details]
Patch 3

This patch has been up for review for a while, with several Google folks CC'ed, so I'm going to say r=me despite this being V8-specific code. Please yell if you'd like someone else to take a look!
Comment 20 WebKit Review Bot 2011-08-08 17:55:45 PDT
Comment on attachment 102384 [details]
Patch 3

Clearing flags on attachment: 102384

Committed r92659: <http://trac.webkit.org/changeset/92659>
Comment 21 WebKit Review Bot 2011-08-08 17:55:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Steve Block 2011-08-09 05:00:45 PDT
Thanks Alexey.

> I'll also send a couple of patches to clean up JavaMethodJobject
Bug 65910

> and JavaString, which are now only needed for JSC.
Bug 65909