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

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied  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 Oliver Hunt <oliver at apple.com>
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 }

?