Bug 163198 – Refactor binding generated casted-this checks for methods

• Component: Bindings
• Status: RESOLVED FIXED

[youenn fablet]

We should align methods with attribute getters/setters

[youenn fablet]

Created attachment 291069 [details]
Patch

[Build Bot]

Comment on attachment 291069 [details]
Patch

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

New failing tests:
js/promises-tests/promises-tests-2-3-2.html
fast/dom/Window/window-postmessage-args.html
js/promises-tests/promises-tests-2-2-7.html
js/promises-tests/promises-tests-2-1-3.html
js/dom/toString-and-valueOf-override.html
js/promises-tests/promises-tests-2-2-3.html
http/tests/security/listener/xss-window-onclick-addEventListener.html
http/tests/security/listener/xss-window-onclick-shortcut.html
js/promises-tests/promises-tests-2-2-6.html
js/promises-tests/promises-tests-2-2-4.html
fast/dom/Window/addEventListener-implicit-this.html
js/promises-tests/promises-tests-2-2-2.html
http/tests/security/listener/xss-JSTargetNode-onclick-addEventListener.html
js/promises-tests/promises-tests-2-2-5.html
fast/workers/worker-call.html
http/tests/security/listener/xss-JSTargetNode-onclick-shortcut.html
fetch/fetch-error-messages.html
http/tests/security/listener/xss-XMLHttpRequest-shortcut.html
js/promises-tests/promises-tests-2-3-4.html
js/promises-tests/promises-tests-2-3-1.html
js/promises-tests/promises-tests-2-2-1.html
http/tests/security/listener/xss-XMLHttpRequest-addEventListener.html

[Build Bot]

Created attachment 291080 [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

[youenn fablet]

Created attachment 291241 [details]
Patch

[Build Bot]

Comment on attachment 291241 [details]
Patch

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

New failing tests:
fast/mediastream/MediaDevices-getUserMedia.html

[Build Bot]

Created attachment 291242 [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

[youenn fablet]

Created attachment 291244 [details]
Fixing custom promise functions

[Darin Adler]

Comment on attachment 291244 [details]
Fixing custom promise functions

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

OK. I’d rather see this done without introducing the term "method".

> Source/WebCore/ChangeLog:11
> +        Introducing BindingCaller::callMethod and callPromiseMethod to encapsulate casted-this checks for methods.
> +        This is supported for all methods except seralizer and iterators methods.

I’m not sure why we are calling these "methods". WebIDL calls one of these an "operation", and C++ calls it a "function" or "member function". Adding in the term "method" brings yet another set of terminology in.

> Source/WebCore/bindings/js/JSEventTargetCustom.h:54
> +template<>
> +struct BindingCaller<JSEventTarget> {

I like to put these all on one line; I know I haven’t commented on this much before, but generally I don’t like how these look spread across two lines both with the same indentation level.

[youenn fablet]

Thanks for the review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291244&action=review
> 
> OK. I’d rather see this done without introducing the term "method".
> 
> > Source/WebCore/ChangeLog:11
> > +        Introducing BindingCaller::callMethod and callPromiseMethod to encapsulate casted-this checks for methods.
> > +        This is supported for all methods except seralizer and iterators methods.
> 
> I’m not sure why we are calling these "methods". WebIDL calls one of these
> an "operation", and C++ calls it a "function" or "member function". Adding
> in the term "method" brings yet another set of terminology in.

Method is more used in binding generator than Operation currently.
I will switch to Operation in this patch.

> > Source/WebCore/bindings/js/JSEventTargetCustom.h:54
> > +template<>
> > +struct BindingCaller<JSEventTarget> {
> 
> I like to put these all on one line; I know I haven’t commented on this much
> before, but generally I don’t like how these look spread across two lines
> both with the same indentation level.

I don't have strong feelings.
Are you suggesting this style for fully specialized ones?

[youenn fablet]

Created attachment 291334 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291334 [details]
Patch for landing

Rejecting attachment 291334 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ease/DerivedSources/WebCore/JSFontFaceSet.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/JSFontFaceSet.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/JSEventTarget.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSEventTarget.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/2267869

[youenn fablet]

Created attachment 291341 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291341 [details]
Patch for landing

Clearing flags on attachment: 291341

Committed r207192: <http://trac.webkit.org/changeset/207192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #9)
> > > Source/WebCore/bindings/js/JSEventTargetCustom.h:54
> > > +template<>
> > > +struct BindingCaller<JSEventTarget> {
> > 
> > I like to put these all on one line; I know I haven’t commented on this much
> > before, but generally I don’t like how these look spread across two lines
> > both with the same indentation level.
> 
> I don't have strong feelings.
> Are you suggesting this style for fully specialized ones?

I like this style for all cases, but I am not sure other agree.