Bug 33672

Summary: [Android] JavaInstance uses JSC-specific types
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Steve Block <steveblock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, android-webkit-unforking, commit-queue, darin, dimich, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Bug Depends on: 33710, 33792    
Bug Blocks: 32154    
Attachments:
Description Flags
Patch 1 for Bug 33672
none
Patch 2 for Bug 33672 none

Description Steve Block 2010-01-14 06:18:32 PST
JavaInstance, defined in WebCore/bridge/jni/jni_instance.h, uses JSC-specific types.
This causes problems on Android, where we can build with JSC or V8.
See Bug 32154, which this bug blocks.

The JSC-specific version of JavaInstance should be moved to a different location, to
keep WebCore/bridge/jni for common code can be used with both JSC and V8.
Comment 1 Steve Block 2010-01-14 06:50:54 PST
Created attachment 46563 [details]
Patch 1 for Bug 33672

Moves JSC-specific JavaInstance from bridge/jni/jni_instance to bridge/jni/jsc/jni_instance_jsc

A later patch will add the V8 version of JavaInstance.
Comment 2 Darin Adler 2010-01-14 11:09:59 PST
If you're making a new file, its name should be the name of the class. So it should be JavaInstance.h, not jni_instance.h.
Comment 3 Steve Block 2010-01-14 16:59:52 PST
(In reply to comment #2)
> If you're making a new file, its name should be the name of the class. So it
> should be JavaInstance.h, not jni_instance.h.
OK - I'd stuck with those names to match the style of the existing files.

The new files I'm adding are jni_instance_jsc.[cpp|h], as they provide the JSC-specific version of JavaInstance. A V8-specific version will be added in the future. Is JavaInstanceJsc.[cpp|h] the correct file name, or maybe JSJavaInstance.[cpp|h] to match the style used for the JSC bindings?

On a related note, when moving around existing code, should I fix the style, or leave it as-is to minimize the diff?
Comment 4 Darin Adler 2010-01-14 17:27:09 PST
(In reply to comment #3)
> Is JavaInstanceJsc.[cpp|h] the correct file name, or maybe
> JSJavaInstance.[cpp|h] to match the style used for the JSC bindings?

Given the use of the abbreviate “JSC” to refer to WebKit’s built-in JavaScript engine (a decision I wasn’t consulted on), the right name would be JavaInstanceJSC.cpp.

In my opinion the JS prefix is naming the JavaScript language to differentiate the binding from the core DOM class it’s a binding for, rather than naming a particular JavaScript engine.

> On a related note, when moving around existing code, should I fix the style, or
> leave it as-is to minimize the diff?

I typically leave code as-is, using "svn move" to preserve the history, but it depends on the quantity of code.
Comment 5 Adam Barth 2010-01-14 17:57:34 PST
Based on our other naming conventions WebCore/bridge/jni/jsc/ should be called WebCore/bridge/jni/js/.  There's only two files in there currently.  Maybe it's not too late to correct this?

The distinction between JS / JSC is confusing.  I'm not entirely sure we're consistent, but WebCore/bindings/js seems like the precedent here.
Comment 6 Steve Block 2010-01-15 04:16:55 PST
(In reply to comment #5)
> Based on our other naming conventions WebCore/bridge/jni/jsc/ should be called
> WebCore/bridge/jni/js/.  There's only two files in there currently.  Maybe it's
> not too late to correct this?
Ok, I've opened Bug 33710 to address this.

This brought up the fact that all of the jni_foo files in WebCore/bridge/jni should be renamed. See Bug 33712. Maybe its best to address this before I start renaming the JSC-specific ones in this bug.
Comment 7 Darin Adler 2010-01-15 12:03:40 PST
(In reply to comment #5)
> Based on our other naming conventions WebCore/bridge/jni/jsc/ should be called
> WebCore/bridge/jni/js/.  There's only two files in there currently.  Maybe it's
> not too late to correct this?
> 
> The distinction between JS / JSC is confusing.  I'm not entirely sure we're
> consistent, but WebCore/bindings/js seems like the precedent here.

JS has always been intended to mean the JavaScript language, not a particular engine. The bindings/js directory meant “bindings for JavaScript”, not “bindings for JavaScriptCore”, and predates the idea of putting support for a non-WebKit JavaScript engine into the WebKit tree.

When engineers at Google added support for V8, the folks doing that chose to use a V8 prefix wherever the JS code previously used a JS prefix. This was not a discussion I was involved in, but I see the appeal of this terse scheme. That could make it seem like “JS” is the name of WebKit’s own JavaScript engine.

JSC is the namespace we used for JavaScript engine code in the JavaScriptCore directories. When talking about V8 vs. the WebKit built-in JavaScript, someone decided to use JSC and JavaScriptCore as the name of the engine. I think that’s a bit more logical than trying to redefine the term “JS” to mean a particular engine.
Comment 8 Darin Adler 2010-01-15 12:06:17 PST
(In reply to comment #6)
> This brought up the fact that all of the jni_foo files in WebCore/bridge/jni
> should be renamed. See Bug 33712. Maybe its best to address this before I start
> renaming the JSC-specific ones in this bug.

There’s a difference between renaming an existing file and adding a new one. I would prefer not to add any additional files with an old-style name even if we haven’t yet renamed the old ones.

By the way, the term “bridge” is a more modern term for the code here. The older code uses the prefix “Runtime” and the term “RuntimeObject”. I find those terms quite difficult to comprehend, and so it’s more than just file renaming that would need to be done.

Those of us at Apple have been leaving this code alone, especially the Java binding. We do not have particularly good testing coverage for it. But given the critical importance of Java to Android, I could see how you might want to put more effort into maintaining and improving the code.
Comment 9 Dmitry Titov 2010-01-15 15:00:24 PST
It would make this way easier to review if it was split in 2 patches: 

- 'svn mv' patch that just moves files
- all the variables renamings etc, like _class->m_class
Comment 10 Steve Block 2010-01-18 03:45:01 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Based on our other naming conventions WebCore/bridge/jni/jsc/ should be called
> > WebCore/bridge/jni/js/.  There's only two files in there currently.  Maybe it's
> > not too late to correct this?
> Ok, I've opened Bug 33710 to address this.
I've closed this as won't fix based on Darin's comments.

> This brought up the fact that all of the jni_foo files in WebCore/bridge/jni
> should be renamed. See Bug 33712. Maybe its best to address this before I start
> renaming the JSC-specific ones in this bug.
I'll leave the general renaming in Bug 33712 for later.
Comment 11 Steve Block 2010-01-18 03:53:47 PST
(In reply to comment #9)
> It would make this way easier to review if it was split in 2 patches: 
> 
> - 'svn mv' patch that just moves files
> - all the variables renamings etc, like _class->m_class
OK, I'll send a patch to do just the renaming and have opened Bug 33792 to do the style fix.
Comment 12 Steve Block 2010-01-18 03:54:23 PST
Created attachment 46810 [details]
Patch 2 for Bug 33672

Renames WebCore/bridge/jni/jni_instance to WebCore/bridge/jni/jsc/JavaInstanceJSC
Comment 13 WebKit Review Bot 2010-01-18 03:59:46 PST
Attachment 46810 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:180:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:179:  Missing space before {  [whitespace/braces] [5]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:218:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:217:  Missing space before {  [whitespace/braces] [5]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:224:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:228:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:229:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:231:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:233:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:235:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:286:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:297:  Declaration has space between type name and * in JavaClass *aClass  [whitespace/declaration] [3]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:315:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:315:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:321:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:323:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:325:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:325:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:326:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:327:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:330:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:331:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp:332:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 50


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Adam Barth 2010-01-18 09:54:28 PST
Comment on attachment 46810 [details]
Patch 2 for Bug 33672

Thanks for addressing Darin's comments.
Comment 15 Steve Block 2010-01-18 10:06:31 PST
Comment on attachment 46810 [details]
Patch 2 for Bug 33672

Will land manually to preserve svn history
Comment 16 WebKit Commit Bot 2010-01-18 10:15:31 PST
Comment on attachment 46810 [details]
Patch 2 for Bug 33672

Clearing flags on attachment: 46810

Committed r53412: <http://trac.webkit.org/changeset/53412>
Comment 17 WebKit Commit Bot 2010-01-18 10:15:43 PST
All reviewed patches have been landed.  Closing bug.