Bug 45440 – [FileSystem] Add File and FileWriter accessor methods in FileEntry

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Add FileEntry.file() and FileEntry.createWrite() methods in FileEntry.
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#the-fileentry-interface

This also requires two new interfaces, FileCallback and FileWriterCallback.
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#idl-def-FileCallback
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#idl-def-FileWriterCallback

[Kinuko Yasuda]

Created attachment 66999 [details]
Patch

[Eric U.]

Comment on attachment 66999 [details]
Patch

> diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> index 0f57413269abc4d3fce9c87aa1427903fd553860..d51bb2ccb621b657a17cc45f41b94b68036fbed4 100644
> --- a/WebCore/GNUmakefile.am
> +++ b/WebCore/GNUmakefile.am
> @@ -3021,10 +3021,14 @@ webcore_built_sources += \
>         DerivedSources/WebCore/JSEntryCallback.h \
>         DerivedSources/WebCore/JSErrorCallback.cpp \
>         DerivedSources/WebCore/JSErrorCallback.h \
> +       DerivedSources/WebCore/JSFileCallback.cpp \
> +       DerivedSources/WebCore/JSFileCallback.h \
>         DerivedSources/WebCore/JSFileEntry.cpp \
>         DerivedSources/WebCore/JSFileEntry.h \
>         DerivedSources/WebCore/JSFileSystemCallback.cpp \
>         DerivedSources/WebCore/JSFileSystemCallback.h \

This needs an if ENABLE_FILE_WRITER guard.

> +       DerivedSources/WebCore/JSFileWriterCallback.cpp \
> +       DerivedSources/WebCore/JSFileWriterCallback.h \
>         DerivedSources/WebCore/JSFlags.cpp \
>         DerivedSources/WebCore/JSFlags.h \
>         DerivedSources/WebCore/JSMetadata.cpp \
> @@ -3048,11 +3052,13 @@ webcore_sources += \
>         WebCore/fileapi/EntryArray.h \
>         WebCore/fileapi/EntryCallback.h \
>         WebCore/fileapi/ErrorCallback.h \
> +       WebCore/fileapi/FileCallback.h \
>         WebCore/fileapi/FileEntry.cpp \
>         WebCore/fileapi/FileEntry.h \
>         WebCore/fileapi/FileSystemCallback.h \
>         WebCore/fileapi/FileSystemCallbacks.cpp \
>         WebCore/fileapi/FileSystemCallbacks.h \

Same here.

> +       WebCore/fileapi/FileWriterCallback.h \
>         WebCore/fileapi/Flags.h \
>         WebCore/fileapi/LocalFileSystem.cpp \
>         WebCore/fileapi/LocalFileSystem.h \
> diff --git a/WebCore/WebCore.gypi b/WebCore/WebCore.gypi
> index c7597946ad0d8997512b89f128a6b9ef7e6220a9..00b6456e912459e10260a0d5f2dbad104dd6fa96 100644
> --- a/WebCore/WebCore.gypi
> +++ b/WebCore/WebCore.gypi
> @@ -96,6 +96,7 @@
>              'fileapi/EntryCallback.idl',
>              'fileapi/ErrorCallback.idl',
>              'fileapi/File.idl',
> +            'fileapi/FileCallback.idl',
>              'fileapi/FileEntry.idl',
>              'fileapi/FileError.idl',
>              'fileapi/FileException.idl',
> @@ -104,6 +105,7 @@
>              'fileapi/FileReaderSync.idl',
>              'fileapi/FileSystemCallback.idl',
>              'fileapi/FileWriter.idl',
> +            'fileapi/FileWriterCallback.idl',
>              'fileapi/Flags.idl',
>              'fileapi/Metadata.idl',
>              'fileapi/MetadataCallback.idl',
> @@ -1446,6 +1448,7 @@
>              'fileapi/ErrorCallback.h',
>              'fileapi/File.cpp',
>              'fileapi/File.h',
> +            'fileapi/FileCallback.h',
>              'fileapi/FileEntry.cpp',
>              'fileapi/FileEntry.h',
>              'fileapi/FileError.h',
> @@ -1466,6 +1469,7 @@
>              'fileapi/FileThreadTask.h',
>              'fileapi/FileWriter.cpp',
>              'fileapi/FileWriter.h',
> +            'fileapi/FileWriterCallback.h',
>              'fileapi/FileWriterClient.h',
>              'fileapi/Flags.h',
>              'fileapi/LocalFileSystem.h',

> diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> index ea6e90145a76071ad62bfa0527dca7e89919870e..2310c3bc056abd2215b8f1ccca5d6d4a4eda31df 100644
> --- a/WebCore/WebCore.pro
> +++ b/WebCore/WebCore.pro
> @@ -2621,6 +2621,7 @@ contains(DEFINES, ENABLE_FILE_SYSTEM=1) {
>          fileapi/EntryArray.h \
>          fileapi/EntryCallback.h \
>          fileapi/ErrorCallback.h \
> +        fileapi/FileCallback.h \

Should FileWriterCallback.h go in here too?
Do we need a nested guard for ENABLE_FILE_WRITER=1?

>          fileapi/FileEntry.h \
>          fileapi/FileSystemCallback.h \
>          fileapi/FileSystemCallbacks.h \






> diff --git a/WebCore/fileapi/FileEntry.idl b/WebCore/fileapi/FileEntry.idl
> index af3b8075ae3751d552fe282529e75d1ad5307c62..41b9a918afdefb1dc1953c93ccc17e970348c304 100644
> --- a/WebCore/fileapi/FileEntry.idl
> +++ b/WebCore/fileapi/FileEntry.idl
> @@ -34,5 +34,9 @@ module storage {
>          GenerateNativeConverter,
>          GenerateToJS
>      ] FileEntry : Entry {
> +#if defined(ENABLE_FILE_WRITER) && ENABLE_FILE_WRITER
> +        void createWriter(in [Callback] FileWriterCallback successCallback, in [Optional, Callback] ErrorCallback errorCallback);
> +#endif
> +        void file(in [Callback] FileCallback successCallback, in [Optional, Callback] ErrorCallback errorCallback);
>      };
>  }
> diff --git a/WebCore/fileapi/FileWriterCallback.h b/WebCore/fileapi/FileWriterCallback.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..3f9e746fa6e0098b5f8bfb16f4da0d7071e7f3f7
> --- /dev/null
> +++ b/WebCore/fileapi/FileWriterCallback.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef FileWriterCallback_h
> +#define FileWriterCallback_h
> +
> +#if ENABLE(FILE_SYSTEM)

I think you want to guard on both FILE_WRITER and FILE_SYSTEM here.

> +
> +#include <wtf/RefCounted.h>
> +
> +namespace WebCore {
> +
> +class FileWriter;
> +
> +class FileWriterCallback : public RefCounted<FileWriterCallback> {
> +public:
> +    virtual ~FileWriterCallback() { }
> +    virtual bool handleEvent(FileWriter*) = 0;
> +};
> +
> +} // namespace
> +
> +#endif // ENABLE(FILE_SYSTEM)
> +
> +#endif // FileWriterCallback_h
> diff --git a/WebCore/fileapi/FileWriterCallback.idl b/WebCore/fileapi/FileWriterCallback.idl
> new file mode 100644
> index 0000000000000000000000000000000000000000..ba77891fde3b778b2412cc1f26e5117e655cbac8
> --- /dev/null
> +++ b/WebCore/fileapi/FileWriterCallback.idl
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +module fileapi {
> +    interface [
> +        Conditional=FILE_SYSTEM&FILE_WRITER,
> +        Callback
> +    ] FileWriterCallback {
> +        boolean handleEvent(in FileWriter fileWriter);
> +    };
> +}


I'm reviewing this patch by diffing it against a nearly-identical one [for FileWriterCallback] in my own repo ;'>.
The only thing missing is a new FileWriterCallbacks derived from FileSystemCallbacksBase.  Would that make this CL too big?

[Jian Li]

Comment on attachment 66999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66999&action=prettypatch

> WebCore/WebCore.vcproj/WebCore.vcproj:4826
> +				<FileConfiguration
The FileConfiguration parts should not be included because they're used to exclude the file. I think we still want the file to be included in the build target, though the feature is not enabled for Windows.

> WebCore/fileapi/FileWriterCallback.idl:33
> +        Conditional=FILE_SYSTEM&FILE_WRITER,
This condition "FILE_SYSTEM&FILE_WRITER" seems not to be consistent with "ENABLE(FILE_SYSTEM)" used in FileWriterCallback.h.

[Kinuko Yasuda]

Created attachment 67117 [details]
Patch

[Kinuko Yasuda]

Thanks for your review,

I moved all the FileWriterCallback stuff in the ENABLE_FILE_WRITER guard.
They'll be included in the build target if FILE_WRITER is enabled regardless of FILE_SYSTEM flag, but it has ENABLE guard in the source code too so there should be no problem.  (Also FileWriterCallback itself does not rely on other FILE_SYSTEM files.)

Eric, yeah I was afraid you may have similar code in your env.  Feeling sorry about the duplicated effort.
In this patch I also added FileWriter.cpp and idl in CMakeLists.txt with ENABLE_FILE_WRITER guard -- was that ok?  Let me know if you had intentionally excluded them.

(In reply to comment #2)
> (From update of attachment 66999 [details])
> > diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> > index 0f57413269abc4d3fce9c87aa1427903fd553860..d51bb2ccb621b657a17cc45f41b94b68036fbed4 100644
> > --- a/WebCore/GNUmakefile.am
> > +++ b/WebCore/GNUmakefile.am
> > @@ -3021,10 +3021,14 @@ webcore_built_sources += \
> 
> This needs an if ENABLE_FILE_WRITER guard.

> > +       DerivedSources/WebCore/JSFileWriterCallback.cpp \
> > +       DerivedSources/WebCore/JSFileWriterCallback.h \
> > +       WebCore/fileapi/FileWriterCallback.h \

Moved these three in the ENABLE_FILE_WRITER guard.

> > diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> > index ea6e90145a76071ad62bfa0527dca7e89919870e..2310c3bc056abd2215b8f1ccca5d6d4a4eda31df 100644
> > --- a/WebCore/WebCore.pro
> > +++ b/WebCore/WebCore.pro
> > @@ -2621,6 +2621,7 @@ contains(DEFINES, ENABLE_FILE_SYSTEM=1) {
> >          fileapi/EntryArray.h \
> >          fileapi/EntryCallback.h \
> >          fileapi/ErrorCallback.h \
> > +        fileapi/FileCallback.h \
> 
> Should FileWriterCallback.h go in here too?
> Do we need a nested guard for ENABLE_FILE_WRITER=1?

Moved it in a separate (not nested) ENABLE_FILE_WRITER guard.

> > +#if ENABLE(FILE_SYSTEM)
> 
> I think you want to guard on both FILE_WRITER and FILE_SYSTEM here.

Fixed.

(In reply to comment #3)
> (From update of attachment 66999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66999&action=prettypatch
> 
> > WebCore/WebCore.vcproj/WebCore.vcproj:4826
> > +				<FileConfiguration
> The FileConfiguration parts should not be included because they're used to exclude the file. I think we still want the file to be included in the build target, though the feature is not enabled for Windows.

Removed the FileConfiguration parts.  (Maybe I should remove them for other FS stuff too?)

> > WebCore/fileapi/FileWriterCallback.idl:33
> > +        Conditional=FILE_SYSTEM&FILE_WRITER,
> This condition "FILE_SYSTEM&FILE_WRITER" seems not to be consistent with "ENABLE(FILE_SYSTEM)" used in FileWriterCallback.h.

Fixed.

[Eric U.]

(In reply to comment #5)
> Thanks for your review,
> 
> I moved all the FileWriterCallback stuff in the ENABLE_FILE_WRITER guard.
> They'll be included in the build target if FILE_WRITER is enabled regardless of FILE_SYSTEM flag, but it has ENABLE guard in the source code too so there should be no problem.  (Also FileWriterCallback itself does not rely on other FILE_SYSTEM files.)

OK.

> Eric, yeah I was afraid you may have similar code in your env.  Feeling sorry about the duplicated effort.

No worries; the overlap is small.  I'll add in the other bit once this lands.

> In this patch I also added FileWriter.cpp and idl in CMakeLists.txt with ENABLE_FILE_WRITER guard -- was that ok?  Let me know if you had intentionally excluded them.

No, that was just an oversight on my part.  Thanks!

> (In reply to comment #2)
> > (From update of attachment 66999 [details] [details])
> > > diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> > > index 0f57413269abc4d3fce9c87aa1427903fd553860..d51bb2ccb621b657a17cc45f41b94b68036fbed4 100644
> > > --- a/WebCore/GNUmakefile.am
> > > +++ b/WebCore/GNUmakefile.am
> > > @@ -3021,10 +3021,14 @@ webcore_built_sources += \
> > 
> > This needs an if ENABLE_FILE_WRITER guard.
> 
> > > +       DerivedSources/WebCore/JSFileWriterCallback.cpp \
> > > +       DerivedSources/WebCore/JSFileWriterCallback.h \
> > > +       WebCore/fileapi/FileWriterCallback.h \
> 
> Moved these three in the ENABLE_FILE_WRITER guard.
> 
> > > diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> > > index ea6e90145a76071ad62bfa0527dca7e89919870e..2310c3bc056abd2215b8f1ccca5d6d4a4eda31df 100644
> > > --- a/WebCore/WebCore.pro
> > > +++ b/WebCore/WebCore.pro
> > > @@ -2621,6 +2621,7 @@ contains(DEFINES, ENABLE_FILE_SYSTEM=1) {
> > >          fileapi/EntryArray.h \
> > >          fileapi/EntryCallback.h \
> > >          fileapi/ErrorCallback.h \
> > > +        fileapi/FileCallback.h \
> > 
> > Should FileWriterCallback.h go in here too?
> > Do we need a nested guard for ENABLE_FILE_WRITER=1?
> 
> Moved it in a separate (not nested) ENABLE_FILE_WRITER guard.
> 
> > > +#if ENABLE(FILE_SYSTEM)
> > 
> > I think you want to guard on both FILE_WRITER and FILE_SYSTEM here.
> 
> Fixed.
> 
> (In reply to comment #3)
> > (From update of attachment 66999 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=66999&action=prettypatch
> > 
> > > WebCore/WebCore.vcproj/WebCore.vcproj:4826
> > > +				<FileConfiguration
> > The FileConfiguration parts should not be included because they're used to exclude the file. I think we still want the file to be included in the build target, though the feature is not enabled for Windows.
> 
> Removed the FileConfiguration parts.  (Maybe I should remove them for other FS stuff too?)
> 
> > > WebCore/fileapi/FileWriterCallback.idl:33
> > > +        Conditional=FILE_SYSTEM&FILE_WRITER,
> > This condition "FILE_SYSTEM&FILE_WRITER" seems not to be consistent with "ENABLE(FILE_SYSTEM)" used in FileWriterCallback.h.
> 
> Fixed.

[Kinuko Yasuda]

Created attachment 67122 [details]
Patch

rebased.

[Jian Li]

Comment on attachment 67122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67122&action=prettypatch

> WebCore/ChangeLog:8
> +        Adding FileEntry.file() and FileEntry.createWrite() methods in FileEntry.
You can remove "FileEntry." prefix to simplify the sentence. Also, better to have "Added" for "Adding".

> WebCore/fileapi/FileWriterCallback.idl:39
>  
Extra line at the end of the file?

[Kinuko Yasuda]

Committed r67159: <http://trac.webkit.org/changeset/67159>