JavaMethod, which is used by the Java bridge to represent a method of a Java class, uses JavaString for the return value of its name() method. JavaString is a utility class intended to convert from the JNI string type jstring to WTF::String and char*. There's no need for JavaMethod to expose this utility type in its API. Instead it should just expose WTF::String, using JavaString internally where appropriate. JavaString exposes JNI types in its API and JavaInstance exposes JavaMethod in its API, so removing JavaString from JavaMethod's API will allow us to remove the dependency of JavaInstance on JNI types. See Bug 55383.
Created attachment 85044 [details] Patch 1
Looks good but: > virtual String name() const = 0; ... > virtual const char* signature() const= 0; Why is one returning a String and the other a char*?
Comment on attachment 85044 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=85044&action=review > Source/WebCore/bridge/jni/JavaMethod.h:48 > + virtual const char* signature() const= 0; name() used to return const JavaString& but this had to be changed to avoid JNI types in the API. String seems to be the obvious choice instead. I left signature() returning const char* because I wanted to make as few changes as possible. Note that returnType() also returns const char*. If you think all three should use String, I'm happy to make the change but would rather do so in a separate patch to keep this one as simple as possible.
LGTM (In reply to comment #3) > (From update of attachment 85044 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85044&action=review > > > Source/WebCore/bridge/jni/JavaMethod.h:48 > > + virtual const char* signature() const= 0; > > name() used to return const JavaString& but this had to be changed to avoid JNI types in the API. String seems to be the obvious choice instead. I left signature() returning const char* because I wanted to make as few changes as possible. Note that returnType() also returns const char*. > > If you think all three should use String, I'm happy to make the change but would rather do so in a separate patch to keep this one as simple as possible. Fine with doing in a subsequent CL, just wanted to understand why the difference.
Comment on attachment 85044 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=85044&action=review rubber stamp = me assuming andrei LGTMs it >>> Source/WebCore/bridge/jni/JavaMethod.h:48 >>> + virtual const char* signature() const= 0; >> >> name() used to return const JavaString& but this had to be changed to avoid JNI types in the API. String seems to be the obvious choice instead. I left signature() returning const char* because I wanted to make as few changes as possible. Note that returnType() also returns const char*. >> >> If you think all three should use String, I'm happy to make the change but would rather do so in a separate patch to keep this one as simple as possible. > > Fine with doing in a subsequent CL, just wanted to understand why the difference. Space before = > Source/WebCore/bridge/jni/JavaMethod.h:49 > + virtual JNIType JNIReturnType() const= 0; Space before =
Comment on attachment 85044 [details] Patch 1 Will fix style and land manually. Also needs rebasing due to Bug 55966 and Bug 55967.
Created attachment 87501 [details] Patch
Attachment 87501 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8297396
Attachment 87501 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8297397
Created attachment 87504 [details] Patch 3
> Fine with doing in a subsequent CL, just wanted to understand why the difference. Filed Bug 57443 to track this.
Committed r82423: <http://trac.webkit.org/changeset/82423>
http://trac.webkit.org/changeset/82423 might have broken GTK Linux 32-bit Debug
Comment on attachment 87504 [details] Patch 3 Cleared review? from attachment 87504 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).