Bug 156149 – Improve IDL support for object arguments that are neither optional nor nullable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Darin Adler]

Improve IDL support for object arguments that are neither optional nor nullable

[Darin Adler]

Created attachment 275511 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:59:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:60:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:61:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:62:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:63:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:64:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:65:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:66:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:67:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:68:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:69:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:70:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:71:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 13 in 21 files


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

[Darin Adler]

Created attachment 275512 [details]
Patch

[Build Bot]

Comment on attachment 275512 [details]
Patch

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

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 275513 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275512 [details]
Patch

Attachment 275512 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1094550

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 275514 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275512 [details]
Patch

Attachment 275512 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1094557

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 275515 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275512 [details]
Patch

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

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 275516 [details]
Archive of layout-test-results from ews114 for mac-yosemite

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

[Darin Adler]

Created attachment 275982 [details]
Patch

[Darin Adler]

OK, looks like this newer patch now is passing on EWS. Review time!

(Let me know if you think of any other potential reviewers I should cc.)

[Chris Dumez]

Comment on attachment 275982 [details]
Patch

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

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3650
> +                if ($isTearOff) {

May be nicer as a ternary:
$value = $isTearOff ? "$name->propertyReference()" : "*$name";

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3665
> +        push @arguments, $value;

Not a big deal but I think we usually use the version with parentheses:
push(@arguments, $value);

> Source/WebCore/bindings/scripts/test/JS/JSTestMediaQueryListListener.cpp:163
>      RefPtr<MediaQueryListListener> listener = JSMediaQueryListListener::create(asObject(state->uncheckedArgument(0)), castedThis->globalObject());

This should probably use a Ref<>

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:510
>      RefPtr<TestCallbackFunction> testCallbackFunction = JSTestCallbackFunction::create(asObject(state->uncheckedArgument(1)), castedThis->globalObject());

If we used a Ref<> here, we would not need to add the stars below.

[Darin Adler]

Comment on attachment 275982 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3650
>> +                if ($isTearOff) {
> 
> May be nicer as a ternary:
> $value = $isTearOff ? "$name->propertyReference()" : "*$name";

OK.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3665
>> +        push @arguments, $value;
> 
> Not a big deal but I think we usually use the version with parentheses:
> push(@arguments, $value);

I see that now. In my own perl programming I don’t do that, but I’ll follow what most people do here. You’ll notice that the code I replaced was using push like this.

Will change it.

>> Source/WebCore/bindings/scripts/test/JS/JSTestMediaQueryListListener.cpp:163
>>      RefPtr<MediaQueryListListener> listener = JSMediaQueryListListener::create(asObject(state->uncheckedArgument(0)), castedThis->globalObject());
> 
> This should probably use a Ref<>

I think it should use auto. I’ll try to change this, but I’ll only do it if it’s easy.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:510
>>      RefPtr<TestCallbackFunction> testCallbackFunction = JSTestCallbackFunction::create(asObject(state->uncheckedArgument(1)), castedThis->globalObject());
> 
> If we used a Ref<> here, we would not need to add the stars below.

I think it should use auto. I’ll try to change this, but I’ll only do it if it’s easy. I suspect it won’t be easy.

[Darin Adler]

Committed r199265: <http://trac.webkit.org/changeset/199265>