[webkit-reviews] review denied: [Bug 61016] [WebWorkers][Chromium] Use v8 Isolates for in-process implementation of WebWorkers : [Attachment 103993] CR feedback.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Lomov <dslomov at google.com>'s
request for review:
Bug 61016: [WebWorkers][Chromium] Use v8 Isolates for in-process implementation
of WebWorkers
https://bugs.webkit.org/show_bug.cgi?id=61016

Attachment 103993: CR feedback.
https://bugs.webkit.org/attachment.cgi?id=103993&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103993&action=review


This looks good to me pending a little bit of clean up and a few comments.

> Source/WebKit/chromium/ChangeLog:13
> +	   This patch extracts some new common interfaces (WebCommonWorkerBase
and WebCommonWorkerClient) for these 

Update names: WebCommonWorkerBase and WebCommonWorkerClient

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:47
> +class NewWebCommonWorkerClient {

It feels like this class should have a comment just like WebCommonWorkerClient
has one.

Also all of these methods are simply about calling things on the main webkit
thread so I suspect some simplification of the comments below could happen if
there were a top level comment.

> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:67
> +    // WebCommonWorkerBase methods:

s/WebCommonWorkerBase/NewWebWorkerBase/

> Source/WebKit/chromium/src/WebWorkerBase.h:61
> +class NewWebWorkerBase : public WebCore::WorkerLoaderProxy {

Comment for this class (like there is one for WWB below).

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:41
> +

It doesn't seem like a blank line is needed here.

> Source/WebKit/chromium/src/WebWorkerImpl.h:68
> +    // WebCommonWorkerBase methods:

WebCommonWorkerBase update name.