Bug 44732

Summary: Add DOMFileSystem implementation to support Entry manipulation operations
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dumi, ericu, fishd, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 44433    
Bug Blocks: 42903    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch fishd: review+, fishd: commit-queue-

Description Kinuko Yasuda 2010-08-26 16:56:48 PDT
DOMFileSystem, Entry and DirectoryEntry need implementation to support file system operations (move, copy, etc).
Comment 1 Kinuko Yasuda 2010-08-26 17:56:13 PDT
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 2 Eric U. 2010-08-26 18:19:33 PDT
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 3 Darin Fisher (:fishd, Google) 2010-08-27 14:40:16 PDT
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
Comment 4 Kinuko Yasuda 2010-08-27 17:50:57 PDT
Created attachment 65797 [details]
Patch
Comment 5 Kinuko Yasuda 2010-08-27 17:55:31 PDT
(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.
Comment 6 Kinuko Yasuda 2010-08-28 21:29:42 PDT
Created attachment 65845 [details]
Patch

Did more cleanups for firing callbacks asynchronously
Comment 7 Darin Fisher (:fishd, Google) 2010-08-29 14:40:40 PDT
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.
Comment 8 Kinuko Yasuda 2010-08-30 12:21:19 PDT
Created attachment 65938 [details]
Patch

This change depends on the the other change for bug 44732
Comment 9 Kinuko Yasuda 2010-08-30 12:22:12 PDT
(In reply to comment #8)
> This change depends on the the other change for bug 44732

Please ignore this comment.
Comment 10 Kinuko Yasuda 2010-08-30 12:33:17 PDT
Created attachment 65939 [details]
Patch
Comment 11 Darin Fisher (:fishd, Google) 2010-08-30 12:56:49 PDT
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.
Comment 12 Kinuko Yasuda 2010-08-30 13:50:40 PDT
Committed r66407: <http://trac.webkit.org/changeset/66407>