Bug 33672 – [Android] JavaInstance uses JSC-specific types

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

[Steve Block]

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.

[Steve Block]

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.

[Darin Adler]

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.

[Steve Block]

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

[Darin Adler]

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

[Adam Barth]

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.

[Steve Block]

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

[Darin Adler]

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

[Darin Adler]

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

[Dmitry Titov]

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

[Steve Block]

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

[Steve Block]

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

[Steve Block]

Created attachment 46810 [details]
Patch 2 for Bug 33672

Renames WebCore/bridge/jni/jni_instance to WebCore/bridge/jni/jsc/JavaInstanceJSC

[WebKit Review Bot]

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.

[Adam Barth]

Comment on attachment 46810 [details]
Patch 2 for Bug 33672

Thanks for addressing Darin's comments.

[Steve Block]

Comment on attachment 46810 [details]
Patch 2 for Bug 33672

Will land manually to preserve svn history

[WebKit Commit Bot]

Comment on attachment 46810 [details]
Patch 2 for Bug 33672

Clearing flags on attachment: 46810

Committed r53412: <http://trac.webkit.org/changeset/53412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.