Bug 46524

Created attachment 68771 [details] Patch

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

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.

Created attachment 68811 [details] Patch

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.

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.

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

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

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.

(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 :)

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

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

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

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