[webkit-reviews] review denied: [Bug 40872] experimental directory upload : [Attachment 60374] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied John Gregg <johnnyg at google.com>'s
request for review:
Bug 40872: experimental directory upload
https://bugs.webkit.org/show_bug.cgi?id=40872

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

------- Additional Comments from Jian Li <jianli at chromium.org>
WebCore/ChangeLog:5
 +	    Experimental feature of directory attribute on <input type="file">
which
Please first have the title, like the bug title and then follow by the bug
link. After that, you can give the full description. Like:
  Experimental directory upload.
  https://bugs.webkit.org/show_bug.cgi?id=40872

  Experimental feature of directory attribute on ...

Do you have the test to cover this new feature?

WebCore/html/File.cpp:50
 +  File::File(const String& relativePath, const String& filePath)
Please move this constructor above File::Init().

WebCore/html/File.h:53
 +	const String& path() const;
Do we need to expose this? I am not seeing any reference to it.
If you do want to expose this, please add the comment to document this path is
relative path and also change Blob::path() to Blob::fullPath() to avoid the
confusion.

WebCore/html/File.idl:34
 +	    readonly attribute DOMString path;
Better to use Conditional attribute instead of "#if defined". Please also
comment that we only expose relative path.

WebCore/html/HTMLInputElement.idl:44
 +	    attribute [Reflect] boolean webkitdirectory;
Better to use Conditional attribute.

WebCore/html/HTMLInputElement.cpp:1973
 +	// and the paths provided here share a single root directory.
The comment "the paths provided here share a single root directory" seems not
to reflect what is coded here.

WebCore/html/HTMLInputElement.cpp:1977
 +	    for (int i = 0; i < size; i++) {
We can start from 1.