Bug 55772 - JavaParameter should be removed
Summary: JavaParameter should be removed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 55383 55774
  Show dependency treegraph
 
Reported: 2011-03-04 06:36 PST by Steve Block
Modified: 2011-03-07 07:18 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 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.
Comment 1 Steve Block 2011-03-04 06:41:17 PST
Created attachment 84740 [details]
Patch
Comment 2 Andrei Popescu 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?
Comment 3 Steve Block 2011-03-04 07:54:54 PST
Created attachment 84750 [details]
Patch
Comment 4 Andrei Popescu 2011-03-04 08:24:13 PST
Thanks, LGTM
Comment 5 Jeremy Orlow 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
Comment 6 WebKit Commit Bot 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
Comment 7 Steve Block 2011-03-07 05:33:18 PST
Created attachment 84936 [details]
Patch
Comment 8 Steve Block 2011-03-07 05:33:57 PST
Fixed potential crash due to CString local going out of scope when using String::utf8()::data()
Comment 9 Steve Block 2011-03-07 06:53:10 PST
Comment on attachment 84936 [details]
Patch

Retaining jorlow's r+
Comment 10 Steve Block 2011-03-07 07:16:41 PST
Committed r80467: <http://trac.webkit.org/changeset/80467>