Summary: | Improve IDL support for object arguments that are neither optional nor nullable | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||||||
Component: | WebCore JavaScript | Assignee: | Darin Adler <darin> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, rniwa, sam, youennf | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Darin Adler
2016-04-03 15:05:16 PDT
Created attachment 275511 [details]
Patch
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.
Created attachment 275512 [details]
Patch
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 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
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 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
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 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
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 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
Created attachment 275982 [details]
Patch
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.) 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. 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. Committed r199265: <http://trac.webkit.org/changeset/199265> |