[webkit-reviews] review denied: [Bug 76826] [chromium] Add isolated filesystem type and WebDragData::filesystem_id for drag-and-drop using File/DirectoryEntry. : [Attachment 134087] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 76826: [chromium] Add isolated filesystem type and
WebDragData::filesystem_id for drag-and-drop using File/DirectoryEntry.
https://bugs.webkit.org/show_bug.cgi?id=76826

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134087&action=review


> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:62
> +const AsyncFileSystem::Type isolatedType =
static_cast<AsyncFileSystem::Type>(WebKit::WebFileSystem::TypeIsolated);

Why isn't there a AsyncFileSystem::Type value that can be assigned here?
(Should there be one?)

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:64
> +const size_t isolatedPathPrefixLength = sizeof(isolatedPathPrefix) - 1;

This seems unused. Why have it?

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:190
> +    rootURL.append(filesystemId);

Where does filesystemId get validated?

> Source/WebKit/chromium/src/PlatformSupport.cpp:103
> +#if ENABLE(FILE_SYSTEM)

This if shouldn't be in the middle of other include's. (If it is needed, it
should be after all other include's. Alternatively, can we leave out the if
ENABLE completely and always include those header files?)

> Source/WebKit/chromium/src/WebDragData.cpp:127
> +WebString WebDragData::filesystemId() const

This part of the change along with the corresponding change in
Source/WebKit/chromium/public/platform/WebDragData.h seems detached from
everything else.

In addition, it has no implementation and requires a different approval due to
the change in a chromium/public directory. Why not pull it out of this patch?