Bug 45440

Created attachment 66999 [details] Patch

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?

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.

Created attachment 67117 [details] Patch

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.

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

Created attachment 67122 [details] Patch rebased.

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?

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