Bug 46563 - Fix DirectoryReader's behavior to trigger only one EntriesCallback per readEntries
Summary: Fix DirectoryReader's behavior to trigger only one EntriesCallback per readEn...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 42903
  Show dependency treegraph
 
Reported: 2010-09-25 03:50 PDT by Kinuko Yasuda
Modified: 2010-09-30 13:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.88 KB, patch)
2010-09-25 04:00 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (24.16 KB, patch)
2010-09-27 21:42 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (23.62 KB, patch)
2010-09-27 21:52 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (27.38 KB, patch)
2010-09-29 10:43 PDT, Kinuko Yasuda
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-09-25 03:50:48 PDT
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.
Comment 1 Kinuko Yasuda 2010-09-25 04:00:58 PDT
Created attachment 68822 [details]
Patch
Comment 2 Kinuko Yasuda 2010-09-25 04:03:41 PDT
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 3 Adam Barth 2010-09-26 21:16:54 PDT
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?
Comment 4 Kinuko Yasuda 2010-09-27 21:42:26 PDT
Created attachment 69018 [details]
Patch
Comment 5 Kinuko Yasuda 2010-09-27 21:48:53 PDT
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?
Comment 6 Kinuko Yasuda 2010-09-27 21:52:18 PDT
Created attachment 69019 [details]
Patch

Removed duplicated ChangeLog entry.
Comment 7 Kinuko Yasuda 2010-09-29 10:43:40 PDT
Created attachment 69215 [details]
Patch
Comment 8 David Levin 2010-09-29 14:25:57 PDT
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.
Comment 9 David Levin 2010-09-29 14:28:01 PDT
Also, I suggested getting another set of eyes on js code in the test. Please do any minor changes that result from that.
Comment 10 Kinuko Yasuda 2010-09-29 18:17:02 PDT
Committed r68735: <http://trac.webkit.org/changeset/68735>