Bug 46563

Created attachment 68822 [details] Patch

Note that this patch is only for a work-around fix and we'll need more complete fix in WebKit API, browser code and WebCore code to make it work with chunk reading.

Comment on attachment 68822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68822&action=review > WebCore/ChangeLog:8 > + No new tests; will update the layout-test patch (not yet submitted) for this fix. Usually we include the updated layout tests in the same patch so the reviewer can see the effect of your patch and verify that everything is properly tested. > WebCore/fileapi/DOMFileSystem.h:93 > // Schedule a callback. This should not cross threads (should be called on the same context thread). > template <typename CB, typename CBArg> > static void scheduleCallback(ScriptExecutionContext*, PassRefPtr<CB>, PassRefPtr<CBArg>); > > + template <typename CB, typename CBArg> > + void scheduleCallback(PassRefPtr<CB> callback, PassRefPtr<CBArg> callbackArg) > + { > + scheduleCallback(scriptExecutionContext(), callback, callbackArg); > + } This seems super general. Why is this definition trapped inside the fileapi directory? > WebCore/fileapi/DirectoryReader.cpp:53 > + if (m_hasMore) > + m_fileSystem->readDirectory(this, m_fullPath, entriesCallback, errorCallback); We prefer early return. > WebCore/fileapi/DirectoryReader.cpp:55 > + // If there're no more entries, dispatch a callback with an empty array. This comment is redundant with the code. > WebCore/fileapi/DirectoryReader.h:54 > + DOMFileSystem* filesystem() { return m_fileSystem.get(); } DOMFileSystem* filesystem() const > WebCore/fileapi/DirectoryReader.h:59 > + void setHasMore(bool hasMore) { m_hasMore = hasMore; } Why do we have a private setter? Is this function called anywhere? > WebCore/fileapi/FileSystemCallbacks.cpp:167 > + m_directoryReader->setHasMore(hasMore); I see. It's called here. By why do you have a private setter if this class is a friend? We should either get rid of the setter or make the setting public and make this class not a friend. > WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:79 > + delete this; Hum... delete this is generally a sign that ownership isn't worked out properly... Can we use OwnPtr instead? How do we know this doesn't leak?

Created attachment 69018 [details] Patch

Thanks for your review, (In reply to comment #3) > (From update of attachment 68822 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68822&action=review > > > WebCore/ChangeLog:8 > > + No new tests; will update the layout-test patch (not yet submitted) for this fix. > > Usually we include the updated layout tests in the same patch so the reviewer can see the effect of your patch and verify that everything is properly tested. I added a layout test for this one. > > WebCore/fileapi/DOMFileSystem.h:93 > > // Schedule a callback. This should not cross threads (should be called on the same context thread). > > template <typename CB, typename CBArg> > > static void scheduleCallback(ScriptExecutionContext*, PassRefPtr<CB>, PassRefPtr<CBArg>); > > > > + template <typename CB, typename CBArg> > > + void scheduleCallback(PassRefPtr<CB> callback, PassRefPtr<CBArg> callbackArg) > > + { > > + scheduleCallback(scriptExecutionContext(), callback, callbackArg); > > + } > > This seems super general. Why is this definition trapped inside the fileapi directory? Because there hadn't been many APIs that use callbacks this much? (afaik database is the only other one that uses callbacks.) I added FIXME comment to put it in a more generic place. Or if you have any suggestions I'll move it there. > > WebCore/fileapi/DirectoryReader.cpp:53 > > + if (m_hasMore) > > + m_fileSystem->readDirectory(this, m_fullPath, entriesCallback, errorCallback); > We prefer early return. Fixed. > > WebCore/fileapi/DirectoryReader.cpp:55 > > + // If there're no more entries, dispatch a callback with an empty array. > This comment is redundant with the code. Removed. > > WebCore/fileapi/DirectoryReader.h:54 > > + DOMFileSystem* filesystem() { return m_fileSystem.get(); } > DOMFileSystem* filesystem() const Fixed. > > WebCore/fileapi/DirectoryReader.h:59 > > + void setHasMore(bool hasMore) { m_hasMore = hasMore; } > Why do we have a private setter? Is this function called anywhere? > > WebCore/fileapi/FileSystemCallbacks.cpp:167 > > + m_directoryReader->setHasMore(hasMore); > I see. It's called here. By why do you have a private setter if this class is a friend? We should either get rid of the setter or make the setting public and make this class not a friend. Changed the setter to public. > > WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:79 > > + delete this; > > Hum... delete this is generally a sign that ownership isn't worked out properly... Can we use OwnPtr instead? How do we know this doesn't leak? WebFileSystemCallbacksImpl implements WebFileSystemCallbacks, whose instances are passed to outside the WebKit API (i.e. chromium) to get called back eventually. The API's comment explicitly states that the caller must fire any of the callbacks (to avoid leaks). That's why I made it self-destructed upon callbacks -- does that make sense?

Created attachment 69019 [details] Patch Removed duplicated ChangeLog entry.

Created attachment 69215 [details] Patch

Comment on attachment 69215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69215&action=review PLease consider fixing the following before landing this change. > LayoutTests/fast/filesystem/resources/fs-test-util.js:18 > +// FIXME: replace this with the API's one once the backend fully supports removeRecursively. I had a hard time understanding this comment. Is this the same idea? // FIXME: replace this code with the equivalent File API method once the it fully supports removeRecursively. > WebCore/fileapi/DirectoryReader.h:45 > +class EntriesCallbacks; Please sort. > WebCore/fileapi/FileSystemCallbacks.cpp:167 > + m_directoryReader->setHasMore(hasMore); Why is this call only done if m_successCallback is set? It seems like it should always be done.

Also, I suggested getting another set of eyes on js code in the test. Please do any minor changes that result from that.

Committed r68735: <http://trac.webkit.org/changeset/68735>