[webkit-reviews] review granted: [Bug 88293] Support File/DirectoryEntry access for <input type=file> if FileSystem API is enabled : [Attachment 147531] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 88293: Support File/DirectoryEntry access for <input type=file> if
FileSystem API is enabled
https://bugs.webkit.org/show_bug.cgi?id=88293

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147531&action=review


Seems fine.  Please make sure you update all the build files.

> Source/WebCore/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: fast/forms/file/input-file-entries.html

Please link to the spec here.

> Source/WebCore/ChangeLog:12
> +	   * Modules/filesystem/HTMLInputElementFileSystem.cpp: Added.
> +	   * Modules/filesystem/HTMLInputElementFileSystem.h: Added.
> +	   * Modules/filesystem/HTMLInputElementFileSystem.idl: Added.

Should we be updating the other build systems with these files? It looks like
the other build files (GNUmakefile.list.am, Target.pri, etc) list filesystem
files even if the feature isn't enabled.

> Source/WebCore/ChangeLog:17
> +	   * html/FileInputType.cpp:
> +	   (WebCore::FileInputType::receiveDroppedFiles):
> +	   (WebCore::FileInputType::droppedFileSystemId): Added.
> +	   * html/FileInputType.h:

Nit: Can you add a few comments in this section about the changes you're
making?

> Source/WebCore/html/InputType.cpp:680
> +String InputType::droppedFileSystemId()
> +{
> +    ASSERT_NOT_REACHED();
> +    return String();

Nit: Should we just make this pure virtual? I guess there are already other
methods that have ASSERT_NOT_REACHED(), so this is OK.

> LayoutTests/fast/forms/file/input-file-entries.html:37
> +if (layoutTestController) {

window.layoutTestController or this will raise an exception in Chrome/Safari.

> LayoutTests/fast/forms/file/input-file-entries.html:59
> +    eventSender.mouseMoveTo(10, 10);

Can we compute the middle of the file control using clientWidth/clientHeight
and offsetWidth/offsetHeight?