Bug 57859

Created attachment 88281 [details] Patch

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.

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?

> 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

Created attachment 88402 [details] Patch

> Could you please describe the reasons in a little more detail? There's a description in Bug 57230, the umbrella bug for this feature.

> 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.

> 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.

(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.

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.

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

> 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.

(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 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...

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.

Created attachment 102382 [details] Patch

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.

Created attachment 102384 [details] Patch 3 Previous patch seems to have been mangled during upload

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 on attachment 102384 [details] Patch 3 Clearing flags on attachment: 102384 Committed r92659: <http://trac.webkit.org/changeset/92659>

All reviewed patches have been landed. Closing bug.

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