| Summary: | Add support for Promises | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Sam Weinig <sam> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, darin, eflews.bot, gtk-ews, gyuyoung.kim, oliver, philn, rakuco, rniwa, webkit-ews, xan.lopez | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Sam Weinig
2013-08-24 20:26:50 PDT
Created attachment 209569 [details]
Patch
Comment on attachment 209569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209569&action=review High-level question: is there anything in here that *needs* to be written in C++? Long-term, I think that stuff like this would work better if it was implemented in JS. If it doesn't involve computational code (this doesn't seem to) and if it doesn't need deep hooks into the VM, then JS code would be better. I realize that we don't have good support for writing library code in JS ... but maybe that should change? > Source/JavaScriptCore/runtime/JSPromise.cpp:99 > + visitor.append(&(thisObject->m_fulfillCallbacks[i])); I think these are unnecessary parentheses. > Source/JavaScriptCore/runtime/JSPromise.cpp:101 > + visitor.append(&(thisObject->m_rejectCallbacks[i])); Ditto. Created attachment 209570 [details]
Patch
Attachment 209570 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/Promise-already-fulfilled-expected.txt', u'LayoutTests/fast/js/Promise-already-fulfilled.html', u'LayoutTests/fast/js/Promise-already-rejected-expected.txt', u'LayoutTests/fast/js/Promise-already-rejected.html', u'LayoutTests/fast/js/Promise-already-resolved-expected.txt', u'LayoutTests/fast/js/Promise-already-resolved.html', u'LayoutTests/fast/js/Promise-catch-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers.html', u'LayoutTests/fast/js/Promise-catch.html', u'LayoutTests/fast/js/Promise-chain-expected.txt', u'LayoutTests/fast/js/Promise-chain.html', u'LayoutTests/fast/js/Promise-exception-expected.txt', u'LayoutTests/fast/js/Promise-exception.html', u'LayoutTests/fast/js/Promise-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers.html', u'LayoutTests/fast/js/Promise-fulfill.html', u'LayoutTests/fast/js/Promise-init-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers.html', u'LayoutTests/fast/js/Promise-init.html', u'LayoutTests/fast/js/Promise-reject-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers.html', u'LayoutTests/fast/js/Promise-reject.html', u'LayoutTests/fast/js/Promise-resolve-chain-expected.txt', u'LayoutTests/fast/js/Promise-resolve-chain.html', u'LayoutTests/fast/js/Promise-resolve-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers.html', u'LayoutTests/fast/js/Promise-resolve-with-then-exception-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-exception.html', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill.html', u'LayoutTests/fast/js/Promise-resolve-with-then-reject-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-reject.html', u'LayoutTests/fast/js/Promise-resolve.html', u'LayoutTests/fast/js/Promise-simple-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback.html', u'LayoutTests/fast/js/Promise-simple-fulfill.html', u'LayoutTests/fast/js/Promise-simple-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-simple-in-workers.html', u'LayoutTests/fast/js/Promise-simple.html', u'LayoutTests/fast/js/Promise-static-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-static-fulfill.html', u'LayoutTests/fast/js/Promise-static-reject-expected.txt', u'LayoutTests/fast/js/Promise-static-reject.html', u'LayoutTests/fast/js/Promise-static-resolve-expected.txt', u'LayoutTests/fast/js/Promise-static-resolve.html', u'LayoutTests/fast/js/Promise-then-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks.html', u'LayoutTests/fast/js/Promise-then.html', u'LayoutTests/fast/js/Promise-types-expected.txt', u'LayoutTests/fast/js/Promise-types.html', u'LayoutTests/fast/js/Promise.html', u'LayoutTests/fast/js/resources/Promise-catch-in-workers.js', u'LayoutTests/fast/js/resources/Promise-fulfill-in-workers.js', u'LayoutTests/fast/js/resources/Promise-init-in-workers.js', u'LayoutTests/fast/js/resources/Promise-reject-in-workers.js', u'LayoutTests/fast/js/resources/Promise-resolve-in-workers.js', u'LayoutTests/fast/js/resources/Promise-simple-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-without-callbacks-in-workers.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/DerivedSources.pri', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSPromise.cpp', u'Source/JavaScriptCore/runtime/JSPromise.h', u'Source/JavaScriptCore/runtime/JSPromiseCallback.cpp', u'Source/JavaScriptCore/runtime/JSPromiseCallback.h', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.h', u'Source/JavaScriptCore/runtime/JSPromisePrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.h', u'Source/JavaScriptCore/runtime/JSPromiseResolver.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolver.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/UseJSC.cmake', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSBindingsAllInOne.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.h', u'Source/WebCore/bindings/js/JSMainThreadExecState.h', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.h']" exit_code: 1
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:46: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 116 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > (From update of attachment 209569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209569&action=review > > High-level question: is there anything in here that *needs* to be written in C++? Long-term, I think that stuff like this would work better if it was implemented in JS. If it doesn't involve computational code (this doesn't seem to) and if it doesn't need deep hooks into the VM, then JS code would be better. > > I realize that we don't have good support for writing library code in JS ... but maybe that should change? > The only parts that requires C++ integration that I can think of is the queueing to the event loop. It certainly feels like long term, this could be be written in JS. > > Source/JavaScriptCore/runtime/JSPromise.cpp:99 > > + visitor.append(&(thisObject->m_fulfillCallbacks[i])); > > I think these are unnecessary parentheses. > > > Source/JavaScriptCore/runtime/JSPromise.cpp:101 > > + visitor.append(&(thisObject->m_rejectCallbacks[i])); > > Ditto. Ok. Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1563189 Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1560487 Comment on attachment 209570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review A lot of places where we are using pointers we could use references instead. Might be worth doing that in the new code. Even thisObject seems like it should be a reference. Looks really great. I am going to say review+; hope I looked closely enough. > Source/JavaScriptCore/ChangeLog:10 > + in preparation for the Promises eventually being defined move to ECMAScript. "defined move in"? > Source/JavaScriptCore/ChangeLog:77 > + (JSC::JSPromiseTaskContext::create): > + (JSC::JSPromiseTaskContext::JSPromiseTaskContext): > + (JSC::JSPromiseTaskContext::~JSPromiseTaskContext): > + (JSC::JSPromise::create): > + (JSC::JSPromise::createStructure): > + (JSC::JSPromise::JSPromise): > + (JSC::JSPromise::finishCreation): > + (JSC::JSPromise::destroy): > + (JSC::JSPromise::visitChildren): > + (JSC::JSPromise::setResolver): > + (JSC::JSPromise::resolver): > + (JSC::JSPromise::setResult): > + (JSC::JSPromise::result): > + (JSC::JSPromise::setState): > + (JSC::JSPromise::state): > + (JSC::JSPromise::appendCallbacks): > + (JSC::JSPromise::queueTaskToProcessFulfillCallbacks): > + (JSC::JSPromise::queueTaskToProcessRejectCallbacks): > + (JSC::JSPromise::processFulfillCallbacksForTask): > + (JSC::JSPromise::processRejectCallbacksForTask): > + (JSC::JSPromise::processFulfillCallbacksWithValue): > + (JSC::JSPromise::processRejectCallbacksWithValue): I think you can omit the function lists in new files. > Source/JavaScriptCore/runtime/JSPromise.cpp:37 > +struct JSPromiseTaskContext : public TaskContext { I understand the appeal of simplicity in making this just a struct, but wouldn't it be better if the constructor was private, and maybe the data member, too? > Source/JavaScriptCore/runtime/JSPromise.cpp:50 > + virtual ~JSPromiseTaskContext() > + { > + } I don’t think this is necessary. > Source/JavaScriptCore/runtime/JSPromiseCallback.cpp:75 > + JSPromiseCallback* promiseCallback = jsCast<JSPromiseCallback*>(exec->callee()); How about using a reference instead? > Source/JavaScriptCore/runtime/JSPromiseCallback.cpp:76 > + JSPromiseResolver* resolver = promiseCallback->m_resolver.get(); How about using a reference instead? > Source/JavaScriptCore/runtime/JSPromiseCallback.cpp:94 > + switch (promiseCallback->m_algorithm) { > + case JSPromiseCallback::Fulfill: > + resolver->fulfill(exec, value, true); > + break; > + > + case JSPromiseCallback::Resolve: > + resolver->resolve(exec, value, true); > + break; > + > + case JSPromiseCallback::Reject: > + resolver->reject(exec, value, true); > + break; > + } These “true” (for async) have the classic boolean true problem. > Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp:66 > + if (context->isDocument()) { > + JSMainThreadExecState currentState(exec); > + m_functionPtr(exec, m_taskContext.get()); > + } else > + m_functionPtr(exec, m_taskContext.get()); Needs a why comment. Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1556757 Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1565072 Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1555765 Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1556761 Comment on attachment 209570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:85 > + putDirectWithoutTransition(exec->vm(), exec->propertyNames().length, jsNumber(2), ReadOnly | DontEnum | DontDelete); Shouldn't this be 1? The Promise constructor takes only 1 mandatory argument so the length properly on the Promise interface object should be 1. This does not seem to be covered by the added layout tests. > LayoutTests/fast/js/Promise-types-expected.txt:14 > +PASS aPromise.then.length is 2 As per Web IDL, the length of then operation should be 0 (all arguments are optional). 2 is valid as per EcmaScript but shouldn't we comply with Web IDL considering Promises are not officially part of EcmaScript yet? > LayoutTests/fast/js/Promise-types-expected.txt:17 > +PASS aPromise.catch.length is 1 Ditto. Comment on attachment 209570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review >> LayoutTests/fast/js/Promise-types-expected.txt:14 >> +PASS aPromise.then.length is 2 > > As per Web IDL, the length of then operation should be 0 (all arguments are optional). 2 is valid as per EcmaScript but shouldn't we comply with Web IDL considering Promises are not officially part of EcmaScript yet? BTW, I don't get why EcmaScript and Web IDL behave differently here so I filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23056 Either way though, I believe Promise.length should return 1. Comment on attachment 209570 [details] Patch Attachment 209570 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1544809 Comment on attachment 209570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:33 > +#include "JSGlobalObjectTask.h" #include "JSDOMGlobalObjectTask.h" Comment on attachment 209570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.h:77 > +struct TaskContext : RefCounted<TaskContext> { Since this has a vtable anyway, perhaps there could just be a virtual callback, rather than needing to queue a separate function pointer? > Source/JavaScriptCore/runtime/JSPromise.cpp:101 > + visitor.append(&(thisObject->m_rejectCallbacks[i])); Rather than looping here you can mark arrays of values with 'visitor.appendValues'. > Source/JavaScriptCore/runtime/JSPromiseCallback.h:39 > + // FIXME: Consider using a pointer-to-member function instead. pointer-to-member is usually a bad idea. > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:94 > + if (!fulfillCallback.isUndefined()) { It looks like you're permitting a case in which undefined is explicitly passed, but by the description on (3.) below this should probably be an error. > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:102 > + if (!rejectCallback.isUndefined()) { Ditto. (In reply to comment #17) > (From update of attachment 209570 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > Source/JavaScriptCore/runtime/JSGlobalObject.h:77 > > +struct TaskContext : RefCounted<TaskContext> { > > Since this has a vtable anyway, perhaps there could just be a virtual callback, rather than needing to queue a separate function pointer? After thinking about it a bit, I think maybe I should pass a JSFunction instead of this TaskContext stuff. This would make it more like other callbacks in WebCore (reducing code there) and make it more future proof if we move more of this to be in JavaScript. Thoughts? > > Source/JavaScriptCore/runtime/JSPromise.cpp:101 > > + visitor.append(&(thisObject->m_rejectCallbacks[i])); > > Rather than looping here you can mark arrays of values with 'visitor.appendValues'. I think that only works with WriteBarrierBase<Unknown>* types right now. Should I templatize it? > > > Source/JavaScriptCore/runtime/JSPromiseCallback.h:39 > > + // FIXME: Consider using a pointer-to-member function instead. > > pointer-to-member is usually a bad idea. Ok, will remove the comment. > > > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:94 > > + if (!fulfillCallback.isUndefined()) { > > It looks like you're permitting a case in which undefined is explicitly passed, but by the description on (3.) below this should probably be an error. > > > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:102 > > + if (!rejectCallback.isUndefined()) { > The spec says to treat undefined as missing (http://dom.spec.whatwg.org/#promises-api) so I think this is right. (In reply to comment #13) > (From update of attachment 209570 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:85 > > + putDirectWithoutTransition(exec->vm(), exec->propertyNames().length, jsNumber(2), ReadOnly | DontEnum | DontDelete); > > Shouldn't this be 1? The Promise constructor takes only 1 mandatory argument so the length properly on the Promise interface object should be 1. This does not seem to be covered by the added layout tests. Good catch. I'll change it and add a test. > > > LayoutTests/fast/js/Promise-types-expected.txt:14 > > +PASS aPromise.then.length is 2 > > As per Web IDL, the length of then operation should be 0 (all arguments are optional). 2 is valid as per EcmaScript but shouldn't we comply with Web IDL considering Promises are not officially part of EcmaScript yet? > > > LayoutTests/fast/js/Promise-types-expected.txt:17 > > +PASS aPromise.catch.length is 1 > > Ditto. Hm. I'll match WebIDL for now, but it does seem a bit silly. Comment on attachment 209570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > Source/JavaScriptCore/runtime/JSPromise.cpp:52 > + Strong<JSPromise> m_promise; What's the lifetime of m_promise bound by? >> Source/JavaScriptCore/runtime/JSPromiseCallback.cpp:94 >> + } > > These “true” (for async) have the classic boolean true problem. Agreed, let's use an enum. I wonder if we can make the style bot complain about new bool parameters/fields? > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:183 > + There's so much duplicated code, can we reduce it via a a helper function? Essentially InternalFunction* createWrapper(…, kind) { if (undefined) return JSPromiseCallback::create(exec, globalObject, globalObject->promiseCallbackStructure(), resolver, kind); if (callType == None) throw; return JSPromiseWrapperCallback::create } ? (In reply to comment #20) > (From update of attachment 209570 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > Source/JavaScriptCore/runtime/JSPromise.cpp:52 > > + Strong<JSPromise> m_promise; > > What's the lifetime of m_promise bound by? TaskContext is RefCounted. > > >> Source/JavaScriptCore/runtime/JSPromiseCallback.cpp:94 > >> + } > > > > These “true” (for async) have the classic boolean true problem. > > Agreed, let's use an enum. I wonder if we can make the style bot complain about new bool parameters/fields? > > > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:183 > > + Ok, switched to an enum. > There's so much duplicated code, can we reduce it via a a helper function? > > Essentially > > InternalFunction* createWrapper(…, kind) { if (undefined) return JSPromiseCallback::create(exec, globalObject, globalObject->promiseCallbackStructure(), resolver, kind); if (callType == None) throw; return JSPromiseWrapperCallback::create } I added a helper, but it does not handle the error case, as I wanted to keep the fidelity of the error strings without having to jump through hoops. Created attachment 209599 [details]
Patch
Attachment 209599 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/Promise-already-fulfilled-expected.txt', u'LayoutTests/fast/js/Promise-already-fulfilled.html', u'LayoutTests/fast/js/Promise-already-rejected-expected.txt', u'LayoutTests/fast/js/Promise-already-rejected.html', u'LayoutTests/fast/js/Promise-already-resolved-expected.txt', u'LayoutTests/fast/js/Promise-already-resolved.html', u'LayoutTests/fast/js/Promise-catch-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers.html', u'LayoutTests/fast/js/Promise-catch.html', u'LayoutTests/fast/js/Promise-chain-expected.txt', u'LayoutTests/fast/js/Promise-chain.html', u'LayoutTests/fast/js/Promise-exception-expected.txt', u'LayoutTests/fast/js/Promise-exception.html', u'LayoutTests/fast/js/Promise-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers.html', u'LayoutTests/fast/js/Promise-fulfill.html', u'LayoutTests/fast/js/Promise-init-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers.html', u'LayoutTests/fast/js/Promise-init.html', u'LayoutTests/fast/js/Promise-reject-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers.html', u'LayoutTests/fast/js/Promise-reject.html', u'LayoutTests/fast/js/Promise-resolve-chain-expected.txt', u'LayoutTests/fast/js/Promise-resolve-chain.html', u'LayoutTests/fast/js/Promise-resolve-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers.html', u'LayoutTests/fast/js/Promise-resolve-with-then-exception-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-exception.html', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill.html', u'LayoutTests/fast/js/Promise-resolve-with-then-reject-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-reject.html', u'LayoutTests/fast/js/Promise-resolve.html', u'LayoutTests/fast/js/Promise-simple-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback.html', u'LayoutTests/fast/js/Promise-simple-fulfill.html', u'LayoutTests/fast/js/Promise-simple-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-simple-in-workers.html', u'LayoutTests/fast/js/Promise-simple.html', u'LayoutTests/fast/js/Promise-static-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-static-fulfill.html', u'LayoutTests/fast/js/Promise-static-reject-expected.txt', u'LayoutTests/fast/js/Promise-static-reject.html', u'LayoutTests/fast/js/Promise-static-resolve-expected.txt', u'LayoutTests/fast/js/Promise-static-resolve.html', u'LayoutTests/fast/js/Promise-then-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks.html', u'LayoutTests/fast/js/Promise-then.html', u'LayoutTests/fast/js/Promise-types-expected.txt', u'LayoutTests/fast/js/Promise-types.html', u'LayoutTests/fast/js/Promise.html', u'LayoutTests/fast/js/resources/Promise-catch-in-workers.js', u'LayoutTests/fast/js/resources/Promise-fulfill-in-workers.js', u'LayoutTests/fast/js/resources/Promise-init-in-workers.js', u'LayoutTests/fast/js/resources/Promise-reject-in-workers.js', u'LayoutTests/fast/js/resources/Promise-resolve-in-workers.js', u'LayoutTests/fast/js/resources/Promise-simple-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-without-callbacks-in-workers.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/DerivedSources.pri', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSPromise.cpp', u'Source/JavaScriptCore/runtime/JSPromise.h', u'Source/JavaScriptCore/runtime/JSPromiseCallback.cpp', u'Source/JavaScriptCore/runtime/JSPromiseCallback.h', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.h', u'Source/JavaScriptCore/runtime/JSPromisePrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.h', u'Source/JavaScriptCore/runtime/JSPromiseResolver.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolver.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/UseJSC.cmake', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSBindingsAllInOne.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.h', u'Source/WebCore/bindings/js/JSMainThreadExecState.h', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.h']" exit_code: 1
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:46: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 116 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209599 [details] Patch Attachment 209599 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1557815 Comment on attachment 209599 [details] Patch Attachment 209599 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1559660 Created attachment 209600 [details]
Patch
Attachment 209600 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/Promise-already-fulfilled-expected.txt', u'LayoutTests/fast/js/Promise-already-fulfilled.html', u'LayoutTests/fast/js/Promise-already-rejected-expected.txt', u'LayoutTests/fast/js/Promise-already-rejected.html', u'LayoutTests/fast/js/Promise-already-resolved-expected.txt', u'LayoutTests/fast/js/Promise-already-resolved.html', u'LayoutTests/fast/js/Promise-catch-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers.html', u'LayoutTests/fast/js/Promise-catch.html', u'LayoutTests/fast/js/Promise-chain-expected.txt', u'LayoutTests/fast/js/Promise-chain.html', u'LayoutTests/fast/js/Promise-exception-expected.txt', u'LayoutTests/fast/js/Promise-exception.html', u'LayoutTests/fast/js/Promise-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers.html', u'LayoutTests/fast/js/Promise-fulfill.html', u'LayoutTests/fast/js/Promise-init-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers.html', u'LayoutTests/fast/js/Promise-init.html', u'LayoutTests/fast/js/Promise-reject-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers.html', u'LayoutTests/fast/js/Promise-reject.html', u'LayoutTests/fast/js/Promise-resolve-chain-expected.txt', u'LayoutTests/fast/js/Promise-resolve-chain.html', u'LayoutTests/fast/js/Promise-resolve-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers.html', u'LayoutTests/fast/js/Promise-resolve-with-then-exception-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-exception.html', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill.html', u'LayoutTests/fast/js/Promise-resolve-with-then-reject-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-reject.html', u'LayoutTests/fast/js/Promise-resolve.html', u'LayoutTests/fast/js/Promise-simple-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback.html', u'LayoutTests/fast/js/Promise-simple-fulfill.html', u'LayoutTests/fast/js/Promise-simple-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-simple-in-workers.html', u'LayoutTests/fast/js/Promise-simple.html', u'LayoutTests/fast/js/Promise-static-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-static-fulfill.html', u'LayoutTests/fast/js/Promise-static-reject-expected.txt', u'LayoutTests/fast/js/Promise-static-reject.html', u'LayoutTests/fast/js/Promise-static-resolve-expected.txt', u'LayoutTests/fast/js/Promise-static-resolve.html', u'LayoutTests/fast/js/Promise-then-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks.html', u'LayoutTests/fast/js/Promise-then.html', u'LayoutTests/fast/js/Promise-types-expected.txt', u'LayoutTests/fast/js/Promise-types.html', u'LayoutTests/fast/js/Promise.html', u'LayoutTests/fast/js/resources/Promise-catch-in-workers.js', u'LayoutTests/fast/js/resources/Promise-fulfill-in-workers.js', u'LayoutTests/fast/js/resources/Promise-init-in-workers.js', u'LayoutTests/fast/js/resources/Promise-reject-in-workers.js', u'LayoutTests/fast/js/resources/Promise-resolve-in-workers.js', u'LayoutTests/fast/js/resources/Promise-simple-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-without-callbacks-in-workers.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/DerivedSources.pri', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSPromise.cpp', u'Source/JavaScriptCore/runtime/JSPromise.h', u'Source/JavaScriptCore/runtime/JSPromiseCallback.cpp', u'Source/JavaScriptCore/runtime/JSPromiseCallback.h', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.h', u'Source/JavaScriptCore/runtime/JSPromisePrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.h', u'Source/JavaScriptCore/runtime/JSPromiseResolver.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolver.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/UseJSC.cmake', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSBindingsAllInOne.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.h', u'Source/WebCore/bindings/js/JSMainThreadExecState.h', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.h']" exit_code: 1
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:47: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:50: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 116 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209600 [details] Patch Attachment 209600 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1558658 Comment on attachment 209600 [details] Patch Attachment 209600 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1563388 Comment on attachment 209600 [details] Patch Attachment 209600 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1557821 Comment on attachment 209600 [details] Patch Attachment 209600 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1542977 Created attachment 209602 [details]
Patch
Attachment 209602 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/Promise-already-fulfilled-expected.txt', u'LayoutTests/fast/js/Promise-already-fulfilled.html', u'LayoutTests/fast/js/Promise-already-rejected-expected.txt', u'LayoutTests/fast/js/Promise-already-rejected.html', u'LayoutTests/fast/js/Promise-already-resolved-expected.txt', u'LayoutTests/fast/js/Promise-already-resolved.html', u'LayoutTests/fast/js/Promise-catch-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers.html', u'LayoutTests/fast/js/Promise-catch.html', u'LayoutTests/fast/js/Promise-chain-expected.txt', u'LayoutTests/fast/js/Promise-chain.html', u'LayoutTests/fast/js/Promise-exception-expected.txt', u'LayoutTests/fast/js/Promise-exception.html', u'LayoutTests/fast/js/Promise-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers.html', u'LayoutTests/fast/js/Promise-fulfill.html', u'LayoutTests/fast/js/Promise-init-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers.html', u'LayoutTests/fast/js/Promise-init.html', u'LayoutTests/fast/js/Promise-reject-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers.html', u'LayoutTests/fast/js/Promise-reject.html', u'LayoutTests/fast/js/Promise-resolve-chain-expected.txt', u'LayoutTests/fast/js/Promise-resolve-chain.html', u'LayoutTests/fast/js/Promise-resolve-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers.html', u'LayoutTests/fast/js/Promise-resolve-with-then-exception-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-exception.html', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill.html', u'LayoutTests/fast/js/Promise-resolve-with-then-reject-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-reject.html', u'LayoutTests/fast/js/Promise-resolve.html', u'LayoutTests/fast/js/Promise-simple-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback.html', u'LayoutTests/fast/js/Promise-simple-fulfill.html', u'LayoutTests/fast/js/Promise-simple-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-simple-in-workers.html', u'LayoutTests/fast/js/Promise-simple.html', u'LayoutTests/fast/js/Promise-static-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-static-fulfill.html', u'LayoutTests/fast/js/Promise-static-reject-expected.txt', u'LayoutTests/fast/js/Promise-static-reject.html', u'LayoutTests/fast/js/Promise-static-resolve-expected.txt', u'LayoutTests/fast/js/Promise-static-resolve.html', u'LayoutTests/fast/js/Promise-then-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks.html', u'LayoutTests/fast/js/Promise-then.html', u'LayoutTests/fast/js/Promise-types-expected.txt', u'LayoutTests/fast/js/Promise-types.html', u'LayoutTests/fast/js/Promise.html', u'LayoutTests/fast/js/resources/Promise-catch-in-workers.js', u'LayoutTests/fast/js/resources/Promise-fulfill-in-workers.js', u'LayoutTests/fast/js/resources/Promise-init-in-workers.js', u'LayoutTests/fast/js/resources/Promise-reject-in-workers.js', u'LayoutTests/fast/js/resources/Promise-resolve-in-workers.js', u'LayoutTests/fast/js/resources/Promise-simple-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-without-callbacks-in-workers.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/DerivedSources.pri', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSPromise.cpp', u'Source/JavaScriptCore/runtime/JSPromise.h', u'Source/JavaScriptCore/runtime/JSPromiseCallback.cpp', u'Source/JavaScriptCore/runtime/JSPromiseCallback.h', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.h', u'Source/JavaScriptCore/runtime/JSPromisePrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.h', u'Source/JavaScriptCore/runtime/JSPromiseResolver.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolver.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/UseJSC.cmake', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSBindingsAllInOne.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.h', u'Source/WebCore/bindings/js/JSMainThreadExecState.h', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.h']" exit_code: 1
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp:47: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:51: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 3 in 116 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 209570 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > > > Source/JavaScriptCore/runtime/JSPromise.cpp:52 > > > + Strong<JSPromise> m_promise; > > > > What's the lifetime of m_promise bound by? > > TaskContext is RefCounted. Yes but what holds the reference to TaskContext? My concern is that the strong reference of m_promise could indirectly keep its owner around > > > > > >> Source/JavaScriptCore/runtime/JSPromiseCallback.cpp:94 > > >> + } > > > > > > These “true” (for async) have the classic boolean true problem. > > > > Agreed, let's use an enum. I wonder if we can make the style bot complain about new bool parameters/fields? > > > > > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:183 > > > + > > Ok, switched to an enum. > > > > There's so much duplicated code, can we reduce it via a a helper function? > > > > Essentially > > > > InternalFunction* createWrapper(…, kind) { if (undefined) return JSPromiseCallback::create(exec, globalObject, globalObject->promiseCallbackStructure(), resolver, kind); if (callType == None) throw; return JSPromiseWrapperCallback::create } > > I added a helper, but it does not handle the error case, as I wanted to keep the fidelity of the error strings without having to jump through hoops. Maybe take an error message string? templates? macros creating templates? ;) If you think it is to many hoops involved in getting the right thing i'll accept your judgement. If you go for macros generating templates i will assign all SVG bugs to you :D Comment on attachment 209602 [details] Patch Attachment 209602 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1564303 Comment on attachment 209602 [details] Patch Attachment 209602 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1560677 Comment on attachment 209602 [details] Patch Attachment 209602 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1564304 Created attachment 209604 [details]
Patch
Attachment 209604 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/Promise-already-fulfilled-expected.txt', u'LayoutTests/fast/js/Promise-already-fulfilled.html', u'LayoutTests/fast/js/Promise-already-rejected-expected.txt', u'LayoutTests/fast/js/Promise-already-rejected.html', u'LayoutTests/fast/js/Promise-already-resolved-expected.txt', u'LayoutTests/fast/js/Promise-already-resolved.html', u'LayoutTests/fast/js/Promise-catch-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-catch-in-workers.html', u'LayoutTests/fast/js/Promise-catch.html', u'LayoutTests/fast/js/Promise-chain-expected.txt', u'LayoutTests/fast/js/Promise-chain.html', u'LayoutTests/fast/js/Promise-exception-expected.txt', u'LayoutTests/fast/js/Promise-exception.html', u'LayoutTests/fast/js/Promise-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-fulfill-in-workers.html', u'LayoutTests/fast/js/Promise-fulfill.html', u'LayoutTests/fast/js/Promise-init-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-init-in-workers.html', u'LayoutTests/fast/js/Promise-init.html', u'LayoutTests/fast/js/Promise-reject-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-reject-in-workers.html', u'LayoutTests/fast/js/Promise-reject.html', u'LayoutTests/fast/js/Promise-resolve-chain-expected.txt', u'LayoutTests/fast/js/Promise-resolve-chain.html', u'LayoutTests/fast/js/Promise-resolve-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-resolve-in-workers.html', u'LayoutTests/fast/js/Promise-resolve-with-then-exception-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-exception.html', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-fulfill.html', u'LayoutTests/fast/js/Promise-resolve-with-then-reject-expected.txt', u'LayoutTests/fast/js/Promise-resolve-with-then-reject.html', u'LayoutTests/fast/js/Promise-resolve.html', u'LayoutTests/fast/js/Promise-simple-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback-expected.txt', u'LayoutTests/fast/js/Promise-simple-fulfill-inside-callback.html', u'LayoutTests/fast/js/Promise-simple-fulfill.html', u'LayoutTests/fast/js/Promise-simple-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-simple-in-workers.html', u'LayoutTests/fast/js/Promise-simple.html', u'LayoutTests/fast/js/Promise-static-fulfill-expected.txt', u'LayoutTests/fast/js/Promise-static-fulfill.html', u'LayoutTests/fast/js/Promise-static-reject-expected.txt', u'LayoutTests/fast/js/Promise-static-reject.html', u'LayoutTests/fast/js/Promise-static-resolve-expected.txt', u'LayoutTests/fast/js/Promise-static-resolve.html', u'LayoutTests/fast/js/Promise-then-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers-expected.txt', u'LayoutTests/fast/js/Promise-then-without-callbacks-in-workers.html', u'LayoutTests/fast/js/Promise-then-without-callbacks.html', u'LayoutTests/fast/js/Promise-then.html', u'LayoutTests/fast/js/Promise-types-expected.txt', u'LayoutTests/fast/js/Promise-types.html', u'LayoutTests/fast/js/Promise.html', u'LayoutTests/fast/js/resources/Promise-catch-in-workers.js', u'LayoutTests/fast/js/resources/Promise-fulfill-in-workers.js', u'LayoutTests/fast/js/resources/Promise-init-in-workers.js', u'LayoutTests/fast/js/resources/Promise-reject-in-workers.js', u'LayoutTests/fast/js/resources/Promise-resolve-in-workers.js', u'LayoutTests/fast/js/resources/Promise-simple-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-in-workers.js', u'LayoutTests/fast/js/resources/Promise-then-without-callbacks-in-workers.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/DerivedSources.pri', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSPromise.cpp', u'Source/JavaScriptCore/runtime/JSPromise.h', u'Source/JavaScriptCore/runtime/JSPromiseCallback.cpp', u'Source/JavaScriptCore/runtime/JSPromiseCallback.h', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.h', u'Source/JavaScriptCore/runtime/JSPromisePrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.h', u'Source/JavaScriptCore/runtime/JSPromiseResolver.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolver.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/UseJSC.cmake', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSBindingsAllInOne.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.h', u'Source/WebCore/bindings/js/JSMainThreadExecState.h', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.h']" exit_code: 1
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp:47: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:51: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 3 in 116 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #34) > (In reply to comment #21) > > (In reply to comment #20) > > > (From update of attachment 209570 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > > > > > Source/JavaScriptCore/runtime/JSPromise.cpp:52 > > > > + Strong<JSPromise> m_promise; > > > > > > What's the lifetime of m_promise bound by? > > > > TaskContext is RefCounted. > > Yes but what holds the reference to TaskContext? My concern is that the strong reference of m_promise could indirectly keep its owner around It is ref'd by the JSGlobalObjectTask, which is a ScriptExecutionContext::Task, which is in turned owned by the ScriptExecutionContext's (be it the Document or Worker) event queue. (In reply to comment #40) > (In reply to comment #34) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > (From update of attachment 209570 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > > > > > > > Source/JavaScriptCore/runtime/JSPromise.cpp:52 > > > > > + Strong<JSPromise> m_promise; > > > > > > > > What's the lifetime of m_promise bound by? > > > > > > TaskContext is RefCounted. > > > > Yes but what holds the reference to TaskContext? My concern is that the strong reference of m_promise could indirectly keep its owner around > > It is ref'd by the JSGlobalObjectTask, which is a ScriptExecutionContext::Task, which is in turned owned by the ScriptExecutionContext's (be it the Document or Worker) event queue. Okay, this feels like a cycle to me - if the event captures the document, how does it get killed? Presumably the document keeps its event queue alive? --Oliver (In reply to comment #41) > (In reply to comment #40) > > (In reply to comment #34) > > > (In reply to comment #21) > > > > (In reply to comment #20) > > > > > (From update of attachment 209570 [details] [details] [details] [details] [details]) > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=209570&action=review > > > > > > > > > > > Source/JavaScriptCore/runtime/JSPromise.cpp:52 > > > > > > + Strong<JSPromise> m_promise; > > > > > > > > > > What's the lifetime of m_promise bound by? > > > > > > > > TaskContext is RefCounted. > > > > > > Yes but what holds the reference to TaskContext? My concern is that the strong reference of m_promise could indirectly keep its owner around > > > > It is ref'd by the JSGlobalObjectTask, which is a ScriptExecutionContext::Task, which is in turned owned by the ScriptExecutionContext's (be it the Document or Worker) event queue. > > Okay, this feels like a cycle to me - if the event captures the document, how does it get killed? Presumably the document keeps its event queue alive? > > --Oliver I'm not sure I follow. Once the document performs the task, it discards it, which should break the cycle. This is the same idiom we use most other places where we have callbacks, I think, see JSCallbackData. Comment on attachment 209604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209604&action=review > Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp:37 > +class JSGlobalObjectCallback : public RefCounted<JSGlobalObjectCallback>, public ActiveDOMCallback { Should be FINAL. And I think ActiveDOMCallback should be private. > Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp:46 > + virtual ~JSGlobalObjectCallback() > + { > + } I suggest just omitting this entirely. No reason to define it explicitly. > Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp:64 > + // push the current't ExecState on to the JSMainThreadExecState stack. current't? > Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h:36 > +class JSGlobalObjectTask : public ScriptExecutionContext::Task { Should be FINAL. > Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h:44 > + virtual void performTask(ScriptExecutionContext*); Should be private and OVERRIDE. > Source/WebCore/bindings/js/JSMainThreadExecState.h:77 > explicit JSMainThreadExecState(JSC::ExecState* exec) There are a lot of stray semicolons at the ends of functions in this class. > Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:75 > } > > + > +bool JSWorkerGlobalScopeBase::allowsAccessFrom(const JSGlobalObject* object, ExecState* exec) Extra blank line here? Committed r154629: <http://trac.webkit.org/changeset/154629> It looks like the JavaScriptCore.vcxproj changes listed in the ChangeLog did not actually get landed in the archive. Windows build is broken. Windows build fix landed in http://trac.webkit.org/changeset/154635. |