Bug 47044

Created attachment 69568 [details] Patch

Created attachment 69570 [details] Patch

Created attachment 69761 [details] Patch Fixed nits.

Comment on attachment 69761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69761&action=review Just the comments from my phase 1 pass: nits. > WebCore/fileapi/DOMFileSystemBase.h:48 > +class EntriesCallback; out of order. > WebCore/fileapi/DOMFileSystemBase.h:74 > + bool readDirectory(DirectoryReaderBase* reader, const String& path, PassRefPtr<EntriesCallback>, PassRefPtr<ErrorCallback>); s/src/source/ Remove param names that add no information (entry, base, reader). > WebCore/fileapi/Entry.h:45 > +class EntrySync; out of order. > WebCore/fileapi/EntryArraySync.h:51 > + static PassRefPtr<EntryArraySync> create(EntryArray* entries); remove param name. > WebCore/fileapi/EntryBase.h:28 > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ Mistaken change? > WebCore/fileapi/EntrySync.h:50 > + static PassRefPtr<EntrySync> create(EntryBase* entry); param name.

Comment on attachment 69761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69761&action=review Just a few things to address. > WebCore/fileapi/DirectoryEntry.cpp:54 > + filesystem()->scheduleCallback(errorCallback, FileError::create(error)); Please get rid of the macro. > WebCore/fileapi/Entry.cpp:51 > + filesystem()->scheduleCallback(errorCallback, FileError::create(error)); Please get rid of the macro. >> WebCore/fileapi/EntrySync.h:50 >> + static PassRefPtr<EntrySync> create(EntryBase* entry); > > param name. (remove the param name) > WebCore/fileapi/FileEntry.h:-42 > -class DOMFileSystem; Why didn't this become DOMFileSystemBase? > WebCore/platform/AsyncFileSystem.h:63 > + // This should return false if there're no pending operations. s/there're/there are/ > WebCore/workers/WorkerContext.h:51 > + class DOMFileSystemSync; out of order. > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:77 > + return false; It seems like m_bridgeForLastOperation should be cleared at the end of this. If that is true, then consider RefPtr<> bridge = m_bridge.release(); and use bridge. Now bridge will get deref'ed when this function is exited. > WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:62 > + virtual bool waitCompletion(); s/waitCompletion/waitForOperationToComplete/ > WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:82 > + RefPtr<WebKit::WorkerFileSystemCallbacksBridge> m_bridgeForLastOperation; m_bridgeForCurrentOperation?

Created attachment 69899 [details] Patch

Created attachment 69901 [details] Patch

Comment on attachment 69901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69899&action=review Just a few nits to address. > WebCore/fileapi/DOMFileSystemBase.h:72 > + bool getFile(const EntryBase* base, const String& path, PassRefPtr<Flags>, PassRefPtr<EntryCallback>, PassRefPtr<ErrorCallback>); remove "base" > WebCore/fileapi/DOMFileSystemBase.h:73 > + bool getDirectory(const EntryBase* base, const String& path, PassRefPtr<Flags>, PassRefPtr<EntryCallback>, PassRefPtr<ErrorCallback>); remove "base" > WebCore/fileapi/FileEntry.h:-42 > -class DOMFileSystem; Please consider adding this back but as Base.

Committed r69249: <http://trac.webkit.org/changeset/69249>