Bug 55765

Summary: JavaMethod should not expose JavaString in its API
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Steve Block <steveblock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, dglazkov, eric, gustavo.noronha, gustavo, jorlow, steveblock, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 55774, 55966, 55967    
Bug Blocks: 55383    
Attachments:
Description Flags
Patch 1
none
Patch
none
Patch 3 none

Steve Block
Reported 2011-03-04 04:23:02 PST
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.
Attachments
Patch 1 (36.79 KB, patch)
2011-03-08 05:52 PST, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch (21.25 KB, patch)
2011-03-30 04:33 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch 3 (33.03 KB, patch)
2011-03-30 04:55 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Steve Block
Comment 1 2011-03-08 05:52:53 PST
Created attachment 85044 [details] Patch 1
Andrei Popescu
Comment 2 2011-03-08 06:58:30 PST
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 3 2011-03-08 07:24:13 PST
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
Comment 4 2011-03-08 08:22:41 PST
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 5 2011-03-08 21:06:29 PST
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 6 2011-03-09 04:29:56 PST
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
Comment 7 2011-03-30 04:33:31 PDT
Created attachment 87501 [details] Patch
Collabora GTK+ EWS bot
Comment 8 2011-03-30 04:44:14 PDT
Attachment 87501 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8297396
WebKit Review Bot
Comment 9 2011-03-30 04:47:47 PDT
Attachment 87501 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8297397
Steve Block
Comment 10 2011-03-30 04:55:39 PDT
Created attachment 87504 [details] Patch 3
Steve Block
Comment 11 2011-03-30 05:20:06 PDT
> Fine with doing in a subsequent CL, just wanted to understand why the difference. Filed Bug 57443 to track this.
Steve Block
Comment 12 2011-03-30 05:28:42 PDT
Committed r82423: <http://trac.webkit.org/changeset/82423>
WebKit Review Bot
Comment 13 2011-03-30 14:40:57 PDT
http://trac.webkit.org/changeset/82423 might have broken GTK Linux 32-bit Debug
Eric Seidel (no email)
Comment 14 2011-04-06 10:39:28 PDT
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).
Note You need to log in before you can comment on or make changes to this bug.