Bug 120260 – Add support for Promises

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Sam Weinig]

Add support for Promises

[Sam Weinig]

Created attachment 209569 [details]
Patch

[Filip Pizlo]

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.

[Sam Weinig]

Created attachment 209570 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

(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.

[Early Warning System Bot]

Comment on attachment 209570 [details]
Patch

Attachment 209570 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1563189

[Early Warning System Bot]

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

[Darin Adler]

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.

[Build Bot]

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

[EFL EWS Bot]

Comment on attachment 209570 [details]
Patch

Attachment 209570 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1565072

[Build Bot]

Comment on attachment 209570 [details]
Patch

Attachment 209570 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1555765

[EFL EWS Bot]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[kov's GTK+ EWS bot]

Comment on attachment 209570 [details]
Patch

Attachment 209570 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1544809

[Chris Dumez]

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"

[Gavin Barraclough]

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.

[Sam Weinig]

(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.

[Sam Weinig]

(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.

[Oliver Hunt]

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 }

?

[Sam Weinig]

(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.

[Sam Weinig]

Created attachment 209599 [details]
Patch

[WebKit Commit Bot]

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.

[Early Warning System Bot]

Comment on attachment 209599 [details]
Patch

Attachment 209599 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1557815

[Early Warning System Bot]

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

[Sam Weinig]

Created attachment 209600 [details]
Patch

[WebKit Commit Bot]

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.

[Early Warning System Bot]

Comment on attachment 209600 [details]
Patch

Attachment 209600 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1558658

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[EFL EWS Bot]

Comment on attachment 209600 [details]
Patch

Attachment 209600 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1542977

[Sam Weinig]

Created attachment 209602 [details]
Patch

[WebKit Commit Bot]

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.

[Oliver Hunt]

(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

[EFL EWS Bot]

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

[Early Warning System Bot]

Comment on attachment 209602 [details]
Patch

Attachment 209602 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1560677

[Early Warning System Bot]

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

[Sam Weinig]

Created attachment 209604 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

(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.

[Oliver Hunt]

(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

[Sam Weinig]

(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.

[Darin Adler]

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?

[Sam Weinig]

Committed r154629: <http://trac.webkit.org/changeset/154629>

[Brent Fulgham]

It looks like the JavaScriptCore.vcxproj changes listed in the ChangeLog did not actually get landed in the archive.  Windows build is broken.

[Ryosuke Niwa]

Windows build fix landed in http://trac.webkit.org/changeset/154635.