Bug 183429 – [JSC] Add inherits<T>(VM&) leveraging JSCast fast path

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Add inherits<T>(VM&) leveraging JSCast fast path

[Yusuke Suzuki]

Created attachment 335266 [details]
Patch

[EWS Watchlist]

Attachment 335266 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1755:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 335273 [details]
Patch

[EWS Watchlist]

Attachment 335273 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1755:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 335279 [details]
Patch

[EWS Watchlist]

Attachment 335279 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1755:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 335282 [details]
Patch

[EWS Watchlist]

Attachment 335282 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1755:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 335285 [details]
Patch

[EWS Watchlist]

Attachment 335285 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1755:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 335285 [details]
Patch

Attachment 335285 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6853687

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 335286 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 335287 [details]
Patch

[EWS Watchlist]

Attachment 335287 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1755:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:1777:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 335287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335287&action=review

r=me.  Nice work.

> Source/JavaScriptCore/runtime/JSType.h:121
> +static const uint32_t LastObjectType = MaxJSType;

I think you should put your ChangeLog comment near here to explain why you use MaxJSType and not the last type in the enum list.  BTW, do we still need LastJSCObjectType?  Are the other uses of LastJSCObjectType correct (in light of the bug you found)?

> Source/WebCore/bindings/js/JSEventTargetCustom.cpp:60
> +    if (value.inherits<JS##interfaceName>(vm))                      \

nit: the amount of whitespace at the end seems arbitrary.  Might as well trim it so that the \ is aligned with the line above, or just 1 space after the end of this line.

[Yusuke Suzuki]

Comment on attachment 335287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335287&action=review

Thank you!

>> Source/JavaScriptCore/runtime/JSType.h:121
>> +static const uint32_t LastObjectType = MaxJSType;
> 
> I think you should put your ChangeLog comment near here to explain why you use MaxJSType and not the last type in the enum list.  BTW, do we still need LastJSCObjectType?  Are the other uses of LastJSCObjectType correct (in light of the bug you found)?

Yes, LastJSCObjectType is now used in JSDollarVM to create extended object types by using `LastJSCObjectType + 1`.
I think we can use the highest bit + type number instead, but in the meantime, it is used, and I keep it now in this patch.

And the other use of LastJSCObjectType is correct :)

>> Source/WebCore/bindings/js/JSEventTargetCustom.cpp:60
>> +    if (value.inherits<JS##interfaceName>(vm))                      \
> 
> nit: the amount of whitespace at the end seems arbitrary.  Might as well trim it so that the \ is aligned with the line above, or just 1 space after the end of this line.

OK, I'll just put one space before `\` since it is easy to keep it.

[Yusuke Suzuki]

Committed r229410: <https://trac.webkit.org/changeset/229410>

[Radar WebKit Bug Importer]

<rdar://problem/38262932>

[Keith Miller]

Comment on attachment 335287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335287&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        We also fix the implementation of jsDynamicCast for JSObject since it uses LastJSCObjectType.
> +        The embedder can add their extended object types after that.

Oh really? where does this happen?

[Yusuke Suzuki]

(In reply to Keith Miller from comment #19)
> Comment on attachment 335287 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335287&action=review
> 
> > Source/JavaScriptCore/ChangeLog:17
> > +        We also fix the implementation of jsDynamicCast for JSObject since it uses LastJSCObjectType.
> > +        The embedder can add their extended object types after that.
> 
> Oh really? where does this happen?

You can check WebCore::JSElementType for example ;)

[Yusuke Suzuki]

Committed r229465: <https://trac.webkit.org/changeset/229465>