Bug 46524 – Bridge all FileSystem operations on Workers to the MainThread

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

[Kinuko Yasuda]

Bridge all FileSystem operations to MainThread for Workers.

This is also necessary to support synchronous operations on workers.

[Kinuko Yasuda]

Created attachment 68771 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 68771 [details]
Patch

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

a couple of nits.

> WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:48
>  WebFileSystemCallbacksImpl::WebFileSystemCallbacksImpl(PassOwnPtr<AsyncFileSystemCallbacks> callbacks)

no need for this constructor. webkit allows default parameters.

> WebKit/chromium/src/WebFileSystemCallbacksImpl.h:55
> +    WebFileSystemCallbacksImpl(PassOwnPtr<WebCore::AsyncFileSystemCallbacks>, WebCore::ScriptExecutionContext*);

WebCore::ScriptExecutionContext* = 0, and remove the other constructor.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:63
> +    ASSERT(m_scriptExecutionContext && m_scriptExecutionContext->isWorkerContext());

you can just do ASSERT(m_scriptExecutionContext->isWorkerContext()).

[David Levin]

Comment on attachment 68771 [details]
Patch

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

> WebKit/chromium/src/WebFileSystemCallbacksImpl.h:55
> +    WebFileSystemCallbacksImpl(PassOwnPtr<WebCore::AsyncFileSystemCallbacks>, WebCore::ScriptExecutionContext*);

Why not just always have this constructor?

It feels odd to me to have two different constructors and depending on which one I call different methods may be called.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:76
> +    RefPtr<WorkerFileSystemCallbacksBridge> bridge = WorkerFileSystemCallbacksBridge::create(m_worker, m_scriptExecutionContext, new WebKit::WebFileSystemCallbacksImpl(callbacks));

I would make this a private method.

Something like createWorkerFileSystemCallbacksBridge().

Then you don't even need to put it in a local variable.

For example,
createWorkerFileSystemCallbacksBridge()->postMoveToMainThread(m_webFileSystem, srcPath, destPath, m_mode)

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:61
> +    virtual void move(const String& srcPath, const String& destPath, PassOwnPtr<AsyncFileSystemCallbacks>);

WebKit tries to avoid abbreviations.

s/src/source/
s/dest/destination/

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:54
> +    {

It would be nice to compile assert sizeof(WebKit::WebFileInfo) == sizeof(double) just to catch any change to this structure and make sure this copy method is updated appropriate (and add a comment before the compile assert explaining why it is here).

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:69
> +            WebKit::WebFileSystemEntry newEntry;
> +            WTF::String name = entries[i].name;
> +            newEntry.isDirectory = entries[i].isDirectory;
> +            newEntry.name = name.crossThreadString();
> +            newEntries[i] = newEntry;

Why not just use newEntries directly?
  newEntries[i].isDirectory = entries[i].isDirectory;
  newEntries[i].name = name.crossThreadString();

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:157
> +{

It feels like these should 
  ASSERT(!m_selfRef);

I wonder if the constructor should just be private and all of the methods just exposed as static. (Of course that would invalid my other comment about making a common method to do the construction).

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:246
> +    ASSERT(isMainThread());
> +    if (bridge->derefIfWorkerIsStopped())
> +        return;
> +    ASSERT(fileSystem);

Since these checks appear in every callback, it felt like there should be a better way to do it. I think I've got one for you.

Make a method something like this

void WorkerFileSystemCallbacksBridge::runTaskWithChecksOnMainThread(WebCore::ScriptExecutionContext* context, WorkerFileSystemCallbacksBridge* bridge, PassOwnPtr<Task> taskToRun)
{
  ASSERT(isMainThread());
  if (bridge->derefIfWorkerIsStopped())
       return;
   taskToRun->performTask(context);
}

This is a horrible method name (maybe runTaskOnMainThread).

Change calls from m_worker->dispatchTaskToMainThread to call this method

WorkerFileSystemCallbacksBridge::dispatchTaskToMainThread(PassOwnPtr<ScriptExecutionContext::Task> task) {
   m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskWithChecksOnMainThread, this, task);
}

Then remove the " if (bridge->derefIfWorkerIsStopped())" from all tasks.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:247
> +    fileSystem->move(srcPath, destPath, MainThreadFileSystemCallbacks::create(bridge, mode).leakPtr());

Please consider a method for "MainThreadFileSystemCallbacks::create(bridge, mode).leakPtr()"

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:386
> +    if (bridge->m_callbacksOnWorkerThread) {

It feels like a wrapper task would be good here too.

The contents of the wrapper task would look like:

  if (!bridge->m_callbacksOnWorkerThread)
    return;
  task->performTask();
  bridge->m_callbacksOnWorkerThread = 0;

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:129
> +    // For early-exist; this deref selfRef and returns true if the worker is already null.

Consider
// For early-exit; this deref's selfRef and returns true if the worker is already null.

[Kinuko Yasuda]

Created attachment 68811 [details]
Patch

[Kinuko Yasuda]

Thanks!  Updated the patch.

>> WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:48
>>  WebFileSystemCallbacksImpl::WebFileSystemCallbacksImpl(PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
> 
> no need for this constructor. webkit allows default parameters.

Removed.

>>> WebKit/chromium/src/WebFileSystemCallbacksImpl.h:55
>>> +    WebFileSystemCallbacksImpl(PassOwnPtr<WebCore::AsyncFileSystemCallbacks>, WebCore::ScriptExecutionContext*);
>> 
>> WebCore::ScriptExecutionContext* = 0, and remove the other constructor.
> 
> Why not just always have this constructor?
> 
> It feels odd to me to have two different constructors and depending on which one I call different methods may be called.

Removed one of the constructors.

>> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:63
>> +    ASSERT(m_scriptExecutionContext && m_scriptExecutionContext->isWorkerContext());
> 
> you can just do ASSERT(m_scriptExecutionContext->isWorkerContext()).

Fixed.

>> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:76
>> +    RefPtr<WorkerFileSystemCallbacksBridge> bridge = WorkerFileSystemCallbacksBridge::create(m_worker, m_scriptExecutionContext, new WebKit::WebFileSystemCallbacksImpl(callbacks));
> 
> I would make this a private method.
> 
> Something like createWorkerFileSystemCallbacksBridge().
> 
> Then you don't even need to put it in a local variable.
> 
> For example,
> createWorkerFileSystemCallbacksBridge()->postMoveToMainThread(m_webFileSystem, srcPath, destPath, m_mode)

Changed the WorkerFileSystemBridge's create methods to make it also post an operation request to the main thread. (Please see below)

>> WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:61
>> +    virtual void move(const String& srcPath, const String& destPath, PassOwnPtr<AsyncFileSystemCallbacks>);
> 
> WebKit tries to avoid abbreviations.
> 
> s/src/source/
> s/dest/destination/

Fixed.

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:54
>> +    {
> 
> It would be nice to compile assert sizeof(WebKit::WebFileInfo) == sizeof(double) just to catch any change to this structure and make sure this copy method is updated appropriate (and add a comment before the compile assert explaining why it is here).

Added the ASSERT.

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:69
>> +            newEntries[i] = newEntry;
> 
> Why not just use newEntries directly?
>   newEntries[i].isDirectory = entries[i].isDirectory;
>   newEntries[i].name = name.crossThreadString();

Fixed.

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:157
>> +{
> 
> It feels like these should 
>   ASSERT(!m_selfRef);
> 
> I wonder if the constructor should just be private and all of the methods just exposed as static. (Of course that would invalid my other comment about making a common method to do the construction).

Hmm.  Changed all the postXxxToMainThread methods to static methods named createForXxx that create a bridge, dispatch the operation and return the PassRefPtr (for synchronous calls).

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:246
>> +    ASSERT(fileSystem);
> 
> Since these checks appear in every callback, it felt like there should be a better way to do it. I think I've got one for you.
> 
> Make a method something like this
> 
> void WorkerFileSystemCallbacksBridge::runTaskWithChecksOnMainThread(WebCore::ScriptExecutionContext* context, WorkerFileSystemCallbacksBridge* bridge, PassOwnPtr<Task> taskToRun)
> {
>   ASSERT(isMainThread());
>   if (bridge->derefIfWorkerIsStopped())
>        return;
>    taskToRun->performTask(context);
> }
> 
> This is a horrible method name (maybe runTaskOnMainThread).
> 
> Change calls from m_worker->dispatchTaskToMainThread to call this method
> 
> WorkerFileSystemCallbacksBridge::dispatchTaskToMainThread(PassOwnPtr<ScriptExecutionContext::Task> task) {
>    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskWithChecksOnMainThread, this, task);
> }
> 
> Then remove the " if (bridge->derefIfWorkerIsStopped())" from all tasks.

Sounds good... changed.

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:247
>> +    fileSystem->move(srcPath, destPath, MainThreadFileSystemCallbacks::create(bridge, mode).leakPtr());
> 
> Please consider a method for "MainThreadFileSystemCallbacks::create(bridge, mode).leakPtr()"

Since MainThreadFileSystemCallbacks is never going to be owned by anyone (as it's self-destructed), I changed the method create() to createLeakedPtr().
(If this makes sense to you I'll also fix WebFileSystemCallbacksImpl's constructor - it's also self-destructed and there're several places where it's created with a naked new.)

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:386
>> +    if (bridge->m_callbacksOnWorkerThread) {
> 
> It feels like a wrapper task would be good here too.
> 
> The contents of the wrapper task would look like:
> 
>   if (!bridge->m_callbacksOnWorkerThread)
>     return;
>   task->performTask();
>   bridge->m_callbacksOnWorkerThread = 0;

Fixed.

>> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:129
>> +    // For early-exist; this deref selfRef and returns true if the worker is already null.
> 
> Consider
> // For early-exit; this deref's selfRef and returns true if the worker is already null.

Fixed.

[Adam Barth]

Comment on attachment 68811 [details]
Patch

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

> WebKit/chromium/src/AsyncFileSystemChromium.h:68
> +    AsyncFileSystemChromium(const String& rootPath);

One-argument constructors required "explicit" before their declarations.

[David Levin]

Comment on attachment 68811 [details]
Patch

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

Please consider doing Adam's comment and mine.

Thanks!

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:150
> +PassRefPtr<WorkerFileSystemCallbacksBridge> WorkerFileSystemCallbacksBridge::createForOpenFileSystem(WebWorkerBase* worker, ScriptExecutionContext* workerContext, WebFileSystemCallbacks* callbacks, WebCommonWorkerClient* commonClient, WebFileSystem::Type type, long long size, const String& mode)

I wouldn't bother making these be "createFor" methods and returning a class that no one ever uses.

Consider just returning void and calling the method: "WorkerFileSystemCallbacksBridge::openFileSystem".

[Kinuko Yasuda]

(In reply to comment #7)
> (From update of attachment 68811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68811&action=review
> 
> Please consider doing Adam's comment and mine.
> 
> Thanks!
> 
> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:150
> > +PassRefPtr<WorkerFileSystemCallbacksBridge> WorkerFileSystemCallbacksBridge::createForOpenFileSystem(WebWorkerBase* worker, ScriptExecutionContext* workerContext, WebFileSystemCallbacks* callbacks, WebCommonWorkerClient* commonClient, WebFileSystem::Type type, long long size, const String& mode)
> 
> I wouldn't bother making these be "createFor" methods and returning a class that no one ever uses.
> 
> Consider just returning void and calling the method: "WorkerFileSystemCallbacksBridge::openFileSystem".

Thanks for reviewing!  I'm planning to store the returned value to a local variable in synchronous cases (in a coming change set) while waiting for completion, so I'll leave it as is in this patch.  (Will make it return void if it turns out I won't use it.)

Will definitely fix the 'explicit' one.

[Eric U.]

Comment on attachment 68811 [details]
Patch

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

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:56
> +        ASSERT(sizeof(WebKit::WebFileInfo) == sizeof(double));

1) I believe this could be done with a COMPILE_ASSERT.
2) It should fail.  WebFileInfo currently has 3 fields.

[Kinuko Yasuda]

(In reply to comment #9)
> (From update of attachment 68811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68811&action=review
> 
> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:56
> > +        ASSERT(sizeof(WebKit::WebFileInfo) == sizeof(double));
> 
> 1) I believe this could be done with a COMPILE_ASSERT.
> 2) It should fail.  WebFileInfo currently has 3 fields.

Right.  I was changing it to COMPILE_ASSERT and am in the middle of fixing 2) now :)

[Kinuko Yasuda]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 68811 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68811&action=review
> > 
> > > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:56
> > > +        ASSERT(sizeof(WebKit::WebFileInfo) == sizeof(double));
> > 
> > 1) I believe this could be done with a COMPILE_ASSERT.
> > 2) It should fail.  WebFileInfo currently has 3 fields.
> 
> Right.  I was changing it to COMPILE_ASSERT and am in the middle of fixing 2) now :)

Hmm.  Actually I'm going to get rid of this ASSERT -- make sizeof(struct) work in a platform independent way could be tricky.  I'll replace the copy method with a more specific ones (like the one for WebFieSystemEntry).

[Kinuko Yasuda]

Committed r68669: <http://trac.webkit.org/changeset/68669>

[Kinuko Yasuda]

Committed r68672: <http://trac.webkit.org/changeset/68672>

[Kinuko Yasuda]

Committed r68675: <http://trac.webkit.org/changeset/68675>