[webkit-reviews] review granted: [Bug 103210] [V8] Remove WorkerContextExecutionProxy : [Attachment 175900] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 103210: [V8] Remove WorkerContextExecutionProxy
https://bugs.webkit.org/show_bug.cgi?id=103210

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=175900&action=review


> Source/WebCore/bindings/v8/V8WorkerContextEventListener.cpp:64
> +    WorkerScriptController* script =
static_cast<WorkerContext*>(context)->script();

We lost the ASSERT here.  We should add it back.

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:62
> +    , m_disableEvalPending(String())

No need for this line.	The compiler will init this variable for you.

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:97
> +    // Bail out if the context has already been initialized.

I'd remove this comment.

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:101
> +    // Create a new environment

This one too

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:129
> +    // Bail out if allocation failed.

ditto

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:135
> +    // Wrap the object.

ditto

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:149
> +    v8::HandleScope hs;

hs -> handleScope

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:160
> +    v8::Context::Scope scope(m_context.get());

I'm surprised we don't need to create a new local handle here, but that's
something to worry about in the future.

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:162
> +    v8::TryCatch exceptionCatcher;

exceptionCatcher -> block

> Source/WebCore/bindings/v8/WorkerScriptController.h:101
>      private:

Please add a blank line above this line.