Bug 55765 – JavaMethod should not expose JavaString in its API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

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.

[Steve Block]

Created attachment 85044 [details]
Patch 1

[Andrei Popescu]

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*?

[Steve Block]

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.

[Andrei Popescu]

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.

[Jeremy Orlow]

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 =

[Steve Block]

Comment on attachment 85044 [details]
Patch 1

Will fix style and land manually. Also needs rebasing due to Bug 55966 and Bug 55967.

[Steve Block]

Created attachment 87501 [details]
Patch

[Collabora GTK+ EWS bot]

Attachment 87501 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8297396

[WebKit Review Bot]

Attachment 87501 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8297397

[Steve Block]

Created attachment 87504 [details]
Patch 3

[Steve Block]

> Fine with doing in a subsequent CL, just wanted to understand why the difference.
Filed Bug 57443 to track this.

[Steve Block]

Committed r82423: <http://trac.webkit.org/changeset/82423>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82423 might have broken GTK Linux 32-bit Debug

[Eric Seidel (no email)]

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