Fix DirectoryReader's behavior to trigger only one EntriesCallback per readEntries

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Current implementation assumes one DirectoryReader.readEntries() may triger multiple EntriesCallbacks (for each entries chunk) ending with an empty Entry array, but what the spec says is that one readEntries should yield only one EntriesCallback and we must maintain the status between multiple readEntries.
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#the-directoryreader-interface

Currently the browser implementation doesn't support chunk reading, but we still trigger EntriesCallback twice for each readEntries() -- one for returning entries and the other one for an empty array to indicate the end.   We should fix (a work-around fix for now) this behavior at least to make it conform to the spec.

[Kinuko Yasuda]

Created attachment 68822 [details]
Patch

[Kinuko Yasuda]

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.

[Adam Barth]

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?

[Kinuko Yasuda]

Created attachment 69018 [details]
Patch

[Kinuko Yasuda]

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?

[Kinuko Yasuda]

Created attachment 69019 [details]
Patch

Removed duplicated ChangeLog entry.

[Kinuko Yasuda]

Created attachment 69215 [details]
Patch

[David Levin]

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.

[David Levin]

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

[Kinuko Yasuda]

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