[webkit-reviews] review denied: [Bug 46524] Bridge all FileSystem operations on Workers to the MainThread : [Attachment 68771] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 24 16:40:25 PDT 2010
David Levin <levin at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 46524: Bridge all FileSystem operations on Workers to the MainThread
https://bugs.webkit.org/show_bug.cgi?id=46524
Attachment 68771: Patch
https://bugs.webkit.org/attachment.cgi?id=68771&action=review
------- Additional Comments from David Levin <levin at chromium.org>
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::ScriptE
xecutionContext* 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<ScriptExec
utionContext::Task> task) {
m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskWithChecksOnMainT
hread, 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.
More information about the webkit-reviews
mailing list