| Summary: | Refactor binding generated casted-this checks for methods | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||
| Component: | Bindings | Assignee: | youenn fablet <youennf> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | buildbot, cdumez, commit-queue, darin, joepeck, rniwa | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
youenn fablet
2016-10-10 00:17:25 PDT
Created attachment 291069 [details]
Patch
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 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
Created attachment 291241 [details]
Patch
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 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
Created attachment 291244 [details]
Fixing custom promise functions
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. 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? Created attachment 291334 [details]
Patch for landing
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 Created attachment 291341 [details]
Patch for landing
Comment on attachment 291341 [details] Patch for landing Clearing flags on attachment: 291341 Committed r207192: <http://trac.webkit.org/changeset/207192> All reviewed patches have been landed. Closing bug. (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. |