Bug 65997 – [Chromium] Decouple implementation of allowFileSystem, openFileSystem and allowDatabase from WebWorkerBase

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Dmitry Lomov]

After dedicated workers move in-process, they will no longer be using WebWorkerBase as a base implementation class.

[Dmitry Lomov]

Created attachment 103504 [details]
Implementation

This patch moves implementation of allowFileSystem, openFileSystem and allowDatabase from WebWorkerBase to respectively LocalFileSystemChromium and DatabaseObserver, parametrizing them with relevant data from WebWorker.

[Dmitry Lomov]

(chromium trybots are happy)

[Jian Li]

Comment on attachment 103504 [details]
Implementation

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.

[Dmitry Lomov]

Comment on attachment 103504 [details]
Implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=103504&action=review

Thanks for review!

>> Source/WebKit/chromium/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=65997.
> 
> Please add an empty line after the link.

Done.

>> 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.

Done

>> 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.

Changed formatting to make code clearer.

>> Source/WebKit/chromium/src/DatabaseObserver.cpp:107
>> +        if (!commonClient)
> 
> You can flip the if condition and swap the two actions.

Done although this was the orinal code.

>> 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.

Why for? We are allowing database _for_ worker. Also consistent with other naming (openFileSystemForWorker)

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

Done.

>> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:145
>> +    WebCore::WorkerLoaderProxy* workerLoaderProxy =  &workerContext->thread()->workerLoaderProxy();
> 
> ditto.
> 
> Also, can you use a helper function to get workerThread?

I need both workerThread and workerContext out of this 3 lines of code, so I do not feel separate function is very useful.

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

Done.

>> 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.

Done

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

Done.

>> Source/WebKit/chromium/src/WebWorkerBase.h:97
>> +    virtual WebCommonWorkerClient* commonClient() = 0;
> 
> Please add empty line after this.

Done.

>> 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.

Done.

[Dmitry Lomov]

Created attachment 103523 [details]
CR feedback addressed

[Jian Li]

Comment on attachment 103523 [details]
CR feedback addressed

Almost there. Just some more minor comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=103523&action=review

> Source/WebKit/chromium/ChangeLog:8
> +        parametrizing them with relevant data from WebWorker.

typo: parameterizing

> Source/WebKit/chromium/ChangeLog:10
> +        * src/DatabaseObserver.cpp:Move allowDatabase from WebWorkerBase and update the caller.

Please add a whitespace after ':'.

> Source/WebKit/chromium/ChangeLog:20
> +        * src/LocalFileSystemChromium.cpp:Move allowFileSystem and openFileSystem from WebWorkerBase and update the caller.

ditto.

> Source/WebKit/chromium/ChangeLog:30
> +        * src/WorkerFileSystemCallbacksBridge.h:

Please also add more comment here, like:
  Change WorkerFileSystemCallbacksBridge constructor to take WorkerLoaderProxy argument.

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:122
> +        if (!commonClient)

Since you're already in this code, why not just making it better?

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:184
> +

Please remove an extra blank line.

[Dmitry Lomov]

(In reply to comment #6)
> (From update of attachment 103523 [details])
> Almost there. Just some more minor comments.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=103523&action=review
> 
> > Source/WebKit/chromium/ChangeLog:8
> > +        parametrizing them with relevant data from WebWorker.
> 
> typo: parameterizing

Done. 
> 
> > Source/WebKit/chromium/ChangeLog:10
> > +        * src/DatabaseObserver.cpp:Move allowDatabase from WebWorkerBase and update the caller.
> 
> Please add a whitespace after ':'.

Done.
> 
> > Source/WebKit/chromium/ChangeLog:20
> > +        * src/LocalFileSystemChromium.cpp:Move allowFileSystem and openFileSystem from WebWorkerBase and update the caller.
> 
> ditto.

Done.
> 
> > Source/WebKit/chromium/ChangeLog:30
> > +        * src/WorkerFileSystemCallbacksBridge.h:
> 
> Please also add more comment here, like:
>   Change WorkerFileSystemCallbacksBridge constructor to take WorkerLoaderProxy argument.

Done.
> 
> > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:122
> > +        if (!commonClient)
> 
> Since you're already in this code, why not just making it better?
Done.

> 
> > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:184
> > +
> 
> Please remove an extra blank line.
Done.

[Dmitry Lomov]

Created attachment 103528 [details]
Extra feedback

[Jian Li]

Comment on attachment 103528 [details]
Extra feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=103528&action=review

> Source/WebKit/chromium/src/DatabaseObserver.cpp:147
> +

Extra empty line.

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:75
> +

Extra empty line.

[Dmitry Lomov]

Created attachment 103530 [details]
Empty lines

[WebKit Review Bot]

Comment on attachment 103530 [details]
Empty lines

Rejecting attachment 103530 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 1

Last 500 characters of output:
1031bf49c725076cb9a13180c3c1151164e7e6d7
r92799 = 42efb1ad980c022006972b156e9dcde70e43ac9e
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9336822

[Adam Barth]

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Adam Barth]

You need to leave the "Reviewed by NOBODY (OOPS!)" line int he ChangeLog so that the bot knows where to insert the reviewer information.

[Dmitry Lomov]

Created attachment 103547 [details]
Fixed changelog

[WebKit Review Bot]

Comment on attachment 103547 [details]
Fixed changelog

Clearing flags on attachment: 103547

Committed r92802: <http://trac.webkit.org/changeset/92802>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.