[webkit-reviews] review granted: [Bug 120260] Add support for Promises : [Attachment 209570] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 120260: Add support for Promises
https://bugs.webkit.org/show_bug.cgi?id=120260

Attachment 209570: Patch
https://bugs.webkit.org/attachment.cgi?id=209570&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.