Bug 44732

Created attachment 65653 [details] Patch This patch depends on unsubmitted AsyncFileSystem interface (bug 44433), but I'm uploading this as this includes a lot of spec-oriented details that are independent from other part.

Comment on attachment 65653 [details] Patch WebCore/storage/DOMFileSystem.cpp:102 + if (newName.isEmpty() && DOMFilePath::getDirectory(src->fullPath()) == parent->fullPath()) You also need to verify that newName is different than the existing name. WebCore/storage/DirectoryEntry.h:57 + void getFile(const String& fullPath, PassRefPtr<Flags> options = 0, PassRefPtr<EntryCallback> successCallback = 0, PassRefPtr<ErrorCallback> errorCallback = 0); You've got fullPath here and path in some of the other files [e.g. DirectoryEntry.cpp]. If you want to emphasize that these are full paths, let's do it everywhere.

Comment on attachment 65653 [details] Patch WebCore/storage/DOMFileSystem.cpp:157 + errorCallback->handleEvent(FileError::create(INVALID_MODIFICATION_ERR).get()); should this be dispatched asynchronously? it seems like all events should be dispatched asynchronously for consistency. plus nesting JS -> C++ -> JS can sometimes be problematic. otherwise LGTM

Created attachment 65797 [details] Patch

(In reply to comment #2) > (From update of attachment 65653 [details]) > WebCore/storage/DOMFileSystem.cpp:102 > + if (newName.isEmpty() && DOMFilePath::getDirectory(src->fullPath()) == parent->fullPath()) > You also need to verify that newName is different than the existing name. Fixed. > WebCore/storage/DirectoryEntry.h:57 > + void getFile(const String& fullPath, PassRefPtr<Flags> options = 0, PassRefPtr<EntryCallback> successCallback = 0, PassRefPtr<ErrorCallback> errorCallback = 0); > You've got fullPath here and path in some of the other files [e.g. DirectoryEntry.cpp]. If you want to emphasize that these are full paths, let's do it everywhere. getFile and getDirectory can take either absolute path or relative path, so they must be |path|... fixed. (In reply to comment #3) > (From update of attachment 65653 [details]) > WebCore/storage/DOMFileSystem.cpp:157 > + errorCallback->handleEvent(FileError::create(INVALID_MODIFICATION_ERR).get()); > should this be dispatched asynchronously? it seems like all events should be > dispatched asynchronously for consistency. plus nesting JS -> C++ -> JS can > sometimes be problematic. Changed to post a task that dispatches an error callback.

Created attachment 65845 [details] Patch Did more cleanups for firing callbacks asynchronously

Comment on attachment 65845 [details] Patch > WebCore/platform/AsyncFileSystemCallbacks.h:46 > + virtual void didSucceed(bool fireImmediate = true) = 0; i guess the fireImmediate parameter exists because these callbacks can sometimes be run synchronously from the AsyncFileSystem methods? maybe that should not be the case? it'd be nice if this layer promised not to callback synchronously. > :184 > + m_asyncFileSystem->createDirectory(absolutePath, flags->isExclusive(), new EntryCallbacks(successCallback, errorCallback, scriptExecutionContext(), this, absolutePath, true)); since you need to allocate EntryCallbacks with exactly the same arguments in both cases here, how about allocating them once above the if statement? EntryCallbacks* callbacks = new EntryCallbacks(...); if (flags && flags->isCreate()) m_asyncFileSystem->createDirectory(..., callbacks); else m_asyncFileSystem->directoryExists(..., callbacks); That should improve readability a bit. It'd also be nice to do this anywhere else that it is applicable. > :83 > + class DispatchCallbackTask : public ScriptExecutionContext::Task { some of this machinery for callbacks seems like it is not specific to DOMFileSystem. is it the case that we don't have code like this elsewhere in webcore? maybe it is a FIXME to break this out into a generic place? i haven't had a chance to fully review this patch. this is just some early feedback.

Created attachment 65938 [details] Patch This change depends on the the other change for bug 44732

(In reply to comment #8) > This change depends on the the other change for bug 44732 Please ignore this comment.

Created attachment 65939 [details] Patch

Comment on attachment 65939 [details] Patch glad to see that fireImmediates was not necessary afterall. > :82 > + // FIXME: move this to a more generic place. nit: can this class be made private? > :70 > + // FIXME: clean up this. please make this FIXME comment more descriptive. it is unclear what you want to clean up.

Committed r66407: <http://trac.webkit.org/changeset/66407>