Bug 55772

Summary:

JavaParameter should be removed

Product:

WebKit

Reporter:

Steve Block <steveblock>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andreip, commit-queue, jorlow, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

55383, 55774

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Steve Block

Reported 2011-03-04 06:36:35 PST

JavaParameter is used by the Java bridge to represent the parameter of a Java method. However, there are several problems with the class ... - The getJNIType() method is simply the cached result of the trivial JNITypeFromClassName() utility method acting on m_type. It seems odd to cache the result of a utility method. - The class is really just a wrapper around the m_type string, so isn't worth the code complexity overhead. - It exposes details of JNI in its API. This prevents us from providing a clean interface for JavaInstance/JavaClass etc which does not depend on a JNI/jobject implementation. See Bug 55383.

Attachments
Patch (10.11 KB, patch) 2011-03-04 06:41 PST, Steve Block	no flags	Details Formatted Diff Diff
Patch (10.13 KB, patch) 2011-03-04 07:54 PST, Steve Block	no flags	Details Formatted Diff Diff
Patch (10.63 KB, patch) 2011-03-07 05:33 PST, Steve Block	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Steve Block

Comment 1 2011-03-04 06:41:17 PST

Created attachment 84740 [details] Patch

Andrei Popescu

Comment 2 2011-03-04 07:38:23 PST

LGTM with a minor nit > 64: for (int i = 0; i < numParameters; i++) Here 'i' is an int. > 123: for (unsigned int i = 0; i < m_parameters.size(); i++) Here 'i' is an unsigned. Maybe make this consistent?

Steve Block

Comment 3 2011-03-04 07:54:54 PST

Created attachment 84750 [details] Patch

Andrei Popescu

Comment 4 2011-03-04 08:24:13 PST

Thanks, LGTM

Jeremy Orlow

Comment 5 2011-03-04 10:03:52 PST

Comment on attachment 84750 [details] Patch I don't really understand this stuff, but I trust Andrei's LGTM...r=me

WebKit Commit Bot

Comment 6 2011-03-04 21:10:52 PST

Comment on attachment 84750 [details] Patch Rejecting attachment 84750 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: .... inspector/audits . inspector/console ...................... inspector/debugger .................... inspector/editor ... inspector/elements ................ inspector/extensions ....... inspector/protocol . inspector/styles ............. inspector/timeline ........... java . java/argument-to-object-type.html -> crashed Exiting early after 1 failures. 12852 tests run. 435.17s total testing time 12851 test cases (99%) succeeded 1 test case (<1%) crashed 8 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8086895

Steve Block

Comment 7 2011-03-07 05:33:18 PST

Created attachment 84936 [details] Patch

Steve Block

Comment 8 2011-03-07 05:33:57 PST

Fixed potential crash due to CString local going out of scope when using String::utf8()::data()

Steve Block

Comment 9 2011-03-07 06:53:10 PST

Comment on attachment 84936 [details] Patch Retaining jorlow's r+

Steve Block

Comment 10 2011-03-07 07:16:41 PST

Committed r80467: <http://trac.webkit.org/changeset/80467>

Note You need to log in before you can comment on or make changes to this bug.