[Webkit-unassigned] [Bug 46524] Bridge all FileSystem operations on Workers to the MainThread
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 24 23:30:12 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=46524
--- Comment #5 from Kinuko Yasuda <kinuko at chromium.org> 2010-09-24 23:30:12 PST ---
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list