[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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68822|review?                     |review-
               Flag|                            |




--- Comment #3 from Adam Barth <abarth at webkit.org>  2010-09-26 21:16:54 PST ---
(From update of attachment 68822)
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?

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