[webkit-reviews] review denied: [Bug 61016] [WebWorkers][Chromium] Use v8 Isolates for in-process implementation of WebWorkers : [Attachment 103954] 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 103954: CR feedback
https://bugs.webkit.org/attachment.cgi?id=103954&action=review

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


> Source/WebCore/workers/WorkerMessagingProxy.h:53
> +	   virtual ~WorkerMessagingProxy();

This shouldn't be exposed because no one should ever call it. This class is
self-deleting. (There should probably be a comment before this destructor.)

Also it is odd that it is virtual at all as discussed.

> Source/WebKit/chromium/ChangeLog:14
> +	   three implementations. 

Why are new interfaces needed?

Why are we keeping the old interfaces?

What does shared workers use? (There is a lot of code that got deleted in
WebWorkerClientImpl. Did shared workers avoid using that?)

> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:68
> +    WebCommonWorkerClientBase* commonClientBase() { return m_client; }

What about the other method from WebCommonWorkerBase?

> Source/WebKit/chromium/src/WebWorkerBase.h:96
> +    virtual WebView* view() const { return m_webView; }

The comment about "WebCore::WorkerLoaderProxy methods:" should change to
"WebCommonWorkerBase" and the "WebView* view" method should be part of that
group.

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:82
>      //     return new WorkerMessagingProxy(worker);

Should this commented out code be removed?

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:95
> +    : m_proxy(adoptPtr(new WorkerMessagingProxy(worker)))

As noted previously, WorkerMessagingProxy is self-deleting so best not to store
it in a OwnPtr.

Perhaps we should expose a WorkerMessagingProxy::create method which only
returns a WorkerMessagingProxy* to emphasize this.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:53
> +using WebCore::SerializedScriptValue;

Avoid using in header files.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:-57
> -    WebWorkerClientImpl(WebCore::Worker*);

Note that Source/WebKit/chromium/src/* try to keep the methods in the file in
the same order as the declaration in the class so these rearrangements here
should result in the same rearrangements in the .cpp file.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:67
> +    virtual ~WebWorkerClientImpl();

Why did this become public and why was it private before?

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:114
> +    virtual WebCommonWorkerClientBase* commonClientBase() { return this; }

WebCommonWorkerBase methods?

> Source/WebKit/chromium/src/WebWorkerImpl.cpp:77
> +WebCommonWorkerClientBase* WebWorkerImpl::commonClientBase()

Wrong location.

> Source/WebKit/chromium/src/WebWorkerImpl.h:69
> +    virtual WebCommonWorkerClientBase* commonClientBase();

other methods for WebCommonWorkerBase?