WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
120260
Add support for Promises
https://bugs.webkit.org/show_bug.cgi?id=120260
Summary
Add support for Promises
Sam Weinig
Reported
2013-08-24 20:26:50 PDT
Add support for Promises
Attachments
Patch
(233.47 KB, patch)
2013-08-24 21:02 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(233.42 KB, patch)
2013-08-24 23:01 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(231.79 KB, patch)
2013-08-25 13:07 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(232.01 KB, patch)
2013-08-25 13:26 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(232.25 KB, patch)
2013-08-25 13:54 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(232.29 KB, patch)
2013-08-25 14:10 PDT
,
Sam Weinig
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2013-08-24 21:02:51 PDT
Created
attachment 209569
[details]
Patch
Filip Pizlo
Comment 2
2013-08-24 21:12:34 PDT
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
Comment 3
2013-08-24 23:01:36 PDT
Created
attachment 209570
[details]
Patch
WebKit Commit Bot
Comment 4
2013-08-24 23:03:34 PDT
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
Comment 5
2013-08-24 23:09:32 PDT
(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 6
2013-08-24 23:13:14 PDT
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 7
2013-08-24 23:14:33 PDT
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 8
2013-08-24 23:16:13 PDT
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 9
2013-08-24 23:31:09 PDT
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 10
2013-08-24 23:31:51 PDT
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 11
2013-08-24 23:45:49 PDT
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 12
2013-08-24 23:49:34 PDT
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 13
2013-08-25 00:04:11 PDT
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 14
2013-08-25 00:29:27 PDT
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 15
2013-08-25 00:42:59 PDT
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 16
2013-08-25 00:53:34 PDT
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 17
2013-08-25 09:52:48 PDT
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
Comment 18
2013-08-25 12:07:24 PDT
(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
Comment 19
2013-08-25 12:08:29 PDT
(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 20
2013-08-25 12:09:59 PDT
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
Comment 21
2013-08-25 12:51:13 PDT
(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
Comment 22
2013-08-25 13:07:31 PDT
Created
attachment 209599
[details]
Patch
WebKit Commit Bot
Comment 23
2013-08-25 13:09:57 PDT
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 24
2013-08-25 13:17:50 PDT
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 25
2013-08-25 13:18:23 PDT
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
Comment 26
2013-08-25 13:26:40 PDT
Created
attachment 209600
[details]
Patch
WebKit Commit Bot
Comment 27
2013-08-25 13:30:19 PDT
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 28
2013-08-25 13:35:46 PDT
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 29
2013-08-25 13:37:40 PDT
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 30
2013-08-25 13:40:28 PDT
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 31
2013-08-25 13:49:23 PDT
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
Comment 32
2013-08-25 13:54:40 PDT
Created
attachment 209602
[details]
Patch
WebKit Commit Bot
Comment 33
2013-08-25 13:57:55 PDT
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
Comment 34
2013-08-25 14:04:50 PDT
(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 35
2013-08-25 14:05:44 PDT
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 36
2013-08-25 14:06:39 PDT
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 37
2013-08-25 14:09:35 PDT
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
Comment 38
2013-08-25 14:10:35 PDT
Created
attachment 209604
[details]
Patch
WebKit Commit Bot
Comment 39
2013-08-25 14:13:14 PDT
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
Comment 40
2013-08-25 14:14:37 PDT
(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
Comment 41
2013-08-25 14:18:15 PDT
(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
Comment 42
2013-08-25 14:35:34 PDT
(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 43
2013-08-26 10:35:47 PDT
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
Comment 44
2013-08-26 12:19:51 PDT
Committed
r154629
: <
http://trac.webkit.org/changeset/154629
>
Brent Fulgham
Comment 45
2013-08-26 13:17:26 PDT
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
Comment 46
2013-08-26 13:47:55 PDT
Windows build fix landed in
http://trac.webkit.org/changeset/154635
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug