[webkit-reviews] review denied: [Bug 103306] Support custom V8 bindings for WebWorkers : [Attachment 181028] Updated patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Marshall Greenblatt
<marshall at chromium.org>'s request for review:
Bug 103306: Support custom V8 bindings for WebWorkers
https://bugs.webkit.org/show_bug.cgi?id=103306

Attachment 181028: Updated patch
https://bugs.webkit.org/attachment.cgi?id=181028&action=review

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


> Platform/chromium/public/WebWorkerScriptObserver.h:35
> +    class WebWorkerScriptObserver {

This shouldn't be indented.  I know some code in WebKit indents inside a
namespace, but we're trying to slowly remove that style.

> WebCore/bindings/v8/WorkerScriptController.cpp:278
> +	 return;

four-space indent

> WebCore/bindings/v8/WorkerScriptController.cpp:286
> +	 return;

ditto

> WebCore/bindings/v8/WorkerScriptController.h:45
> +namespace WebKit {
> +class WebWorkerScriptObserver;
> +}

v8/WorkerScriptController.h isn't a Chromium-specific file (even though its
only used in Chromium).  Therefore, it shouldn't depend on Chromium-specific
platform types.

> WebKit/chromium/tests/WebWorkerTest.cpp:135
> +    bool got_create() { return m_got_create; }
> +    bool got_release() { return m_got_release; }
> +    bool got_callback() { return m_got_callback; }

These are in the wrong style.  We use WebKit style for code in WebKit.

> WebKit/chromium/tests/WebWorkerTest.cpp:149
> +	   delete [] buf;

We shouldn't be calling new and delete manually.  Can't we use a smart point
from either WTF or base?

> WebKit/chromium/tests/data/worker_test.js:6
>  Property changes on: WebKit\chromium\tests\data\worker_test.js
>  ___________________________________________________________________
>  Added: svn:eol-style
>    + LF

Why are we making this property change?