[webkit-reviews] review granted: [Bug 25170] Upstream V8WorkerCustom.cpp and V8WorkerContextCustom.cpp for V8 bindings. : [Attachment 29449] Proposed Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted Jian Li
<jianli at chromium.org>'s request for review:
Bug 25170: Upstream V8WorkerCustom.cpp and V8WorkerContextCustom.cpp for V8
bindings.
https://bugs.webkit.org/show_bug.cgi?id=25170

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Apologies for the delay. Good scrubbing! Needs just a few more changes.

> +ACCESSOR_GETTER(WorkerContextSelf) {

Brace on new line.

> +ACCESSOR_GETTER(WorkerContextOnmessage) {

Ditto.

> +ACCESSOR_SETTER(WorkerContextOnmessage) {

Ditto.

> +v8::Handle<v8::Value> SetTimeoutOrInterval(const v8::Arguments& args, bool
singleShot) {

Ditto.

> +v8::Handle<v8::Value> ClearTimeoutOrInterval(const v8::Arguments& args) {

Ditto.

> +    int tid = ToInt32(args[0], ok);

toInt32, and let's expand tid to timerId.

> +CALLBACK_FUNC_DECL(WorkerContextImportScripts) {

Brace on new line.

> +CALLBACK_FUNC_DECL(WorkerContextSetTimeout) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerContextClearTimeout) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerContextSetInterval) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerContextClearInterval) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerContextAddEventListener) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerContextRemoveEventListener) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerConstructor) {

Ditto.

> +    if (!WorkerContextExecutionProxy::isWebWorkersEnabled()) {
> +	   V8Proxy::ThrowError(V8Proxy::SYNTAX_ERROR, "Worker is not
enabled.");
> +	   return v8::Undefined();

Use throwError helper from V8Proxy.

> +	   V8Proxy::ThrowError(V8Proxy::TYPE_ERROR, "DOM object constructor
cannot be called as a function.");
> +	   return v8::Undefined();

Ditto.

> +    if (args.Length() == 0) {
> +	   V8Proxy::ThrowError(V8Proxy::SYNTAX_ERROR, "Not enough arguments");
> +	   return v8::Undefined();

Ditto.

> +    if (tryCatch.HasCaught()) {
> +	   v8::ThrowException(tryCatch.Exception());
> +	   return v8::Undefined();

Ditto.

> +ACCESSOR_GETTER(WorkerOnmessage) {

Brace on new line.

> +ACCESSOR_SETTER(WorkerOnmessage) {

Ditto.

> +ACCESSOR_GETTER(WorkerOnerror) {

Ditto.

> +ACCESSOR_SETTER(WorkerOnerror) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerAddEventListener) {

Ditto.

> +CALLBACK_FUNC_DECL(WorkerRemoveEventListener) {

Ditto.