[Webkit-unassigned] [Bug 46563] Fix DirectoryReader's behavior to trigger only one EntriesCallback per readEntries

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46563





--- Comment #5 from Kinuko Yasuda <kinuko at chromium.org>  2010-09-27 21:48:53 PST ---
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.