[Webkit-unassigned] [Bug 46524] Bridge all FileSystem operations on Workers to the MainThread

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46524


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68771|review?                     |review-
               Flag|                            |




--- Comment #3 from David Levin <levin at chromium.org>  2010-09-24 16:40:25 PST ---
(From update of attachment 68771)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.