[webkit-reviews] review denied: [Bug 44734] Add LocalFileSystem.requestFileSystem interface to DOMWindow : [Attachment 66121] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 44734: Add LocalFileSystem.requestFileSystem interface to DOMWindow
https://bugs.webkit.org/show_bug.cgi?id=44734

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

------- Additional Comments from Jian Li <jianli at chromium.org>
> WebCore/bindings/generic/RuntimeEnabledFeatures.h:141
> +    static bool requestFileSystemEnabled() { return isFileSystemEnabled; }
I still don't understand your comment. requestFileSystemEnabled() seems to be
identical to fileSystemEnabled(). By looking into the name, I do not know what
requestFileSystemEnabled is used for.

> WebCore/page/DOMWindow.cpp:742
> +    if (!document) {
Can this check be moved to the beginning of this method? That is, can we still
call m_localFileSystem->requestFileSystem if document is gone?

> WebCore/page/DOMWindow.cpp:759
> +    m_localFileSystem->requestFileSystem(document,
static_cast<AsyncFileSystem::Type>(type), size, successCallback,
errorCallback);
This could be merged with the first call of
"m_localFileSystem->requestFileSystem", like:
    if (!m_localFileSystem) {
       // Create m_localFileSystem
       ...
    }
    m_localFileSystem->requestFileSystem(document,
static_cast<AsyncFileSystem::Type>(type), size, successCallback,
errorCallback);

> WebCore/page/DOMWindow.h:86
> +    class LocalFileSystem;
Please move all these forward declarations to the right place.