[webkit-reviews] review denied: [Bug 65997] [Chromium] Decouple implementation of allowFileSystem, openFileSystem and allowDatabase from WebWorkerBase : [Attachment 103504] Implementation

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Dmitry Lomov <dslomov at google.com>'s
request for review:
Bug 65997: [Chromium] Decouple implementation of allowFileSystem,
openFileSystem and allowDatabase from WebWorkerBase
https://bugs.webkit.org/show_bug.cgi?id=65997

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

------- Additional Comments from Jian Li <jianli at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103504&action=review


> Source/WebKit/chromium/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=65997.

Please add an empty line after the link.

> Source/WebKit/chromium/ChangeLog:9
> +	   * src/DatabaseObserver.cpp:

Please add more detail regarding this particular file here. For example, you
can add:
  Move allowDatabase from WebWorkerBase and update the caller.

> Source/WebKit/chromium/src/DatabaseObserver.cpp:90
> +								  
createCallbackTask(&didComplete, WebCore::AllowCrossThreadAccess(this),
result), m_mode);

You can join this line and the line above it.

> Source/WebKit/chromium/src/DatabaseObserver.cpp:107
> +	   if (!commonClient)

You can flip the if condition and swap the two actions.

> Source/WebKit/chromium/src/DatabaseObserver.cpp:124
> +bool allowDatabaseForWorker(WebCommonWorkerClient* commonClient, WebFrame*
frame, const WebString& name, const WebString& displayName, unsigned long
estimatedSize)

I think allowDatabaseOnWorker sounds better.

> Source/WebKit/chromium/src/DatabaseObserver.cpp:130
> +    WebCore::WorkerLoaderProxy* workerLoaderProxy = 
&workerContext->thread()->workerLoaderProxy();

You can replace "workerContext->thread()" with workerThread.

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:145
> +    WebCore::WorkerLoaderProxy* workerLoaderProxy = 
&workerContext->thread()->workerLoaderProxy();

ditto.

Also, can you use a helper function to get workerThread?

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:168
> +    WebCore::WorkerLoaderProxy* workerLoaderProxy = 
&workerContext->thread()->workerLoaderProxy();

ditto.

> Source/WebKit/chromium/src/WebWorkerBase.cpp:53
> +#include "WorkerLoaderProxy.h"

It seems that you can also remove the including of
WorkerFileSystemCallbacksBridge.h, WorkerScriptController.h and others.

> Source/WebKit/chromium/src/WebWorkerBase.h:95
> +    WebView* webView() { return m_webView; }

Please add const modifier.

> Source/WebKit/chromium/src/WebWorkerBase.h:97
> +    virtual WebCommonWorkerClient* commonClient() = 0;

Please add empty line after this.

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:42
> +#include "WorkerLoaderProxy.h"

You can use forward declaration for WorkerLoaderProxy, instead of including it.


Do not forget to remove the forward declaration for WebWorkerBase.