[webkit-reviews] review denied: [Bug 46563] Fix DirectoryReader's behavior to trigger only one EntriesCallback per readEntries : [Attachment 68822] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 46563: Fix DirectoryReader's behavior to trigger only one EntriesCallback
per readEntries
https://bugs.webkit.org/show_bug.cgi?id=46563

Attachment 68822: Patch
https://bugs.webkit.org/attachment.cgi?id=68822&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?