node.cloneNode() should clone the value of file upload elements

node.cloneNode() should clone the value of file selected

when input element of type file with a selected file is cloned, the cloned node should
also have the same file as selected.
HTML specification states:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html
"The cloning steps for input elements must propagate the value, dirty value
flag, checkedness, and dirty checkedness flag from the node being cloned to the
copy."

The value for the input element of type fileupload is the filename.

Behaviours in major Browser:
Firefox- clones filename
Safari, IE & Opera- do not Clone filename 

BUG=71536
TEST=fast/forms/file/input-file-element-clone.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173626

[gnana, 2014-05-02 11:28:04]

Please have a look

[gnana, 2014-05-02 11:30:49]

Please have a look

[Inactive, 2014-05-06 13:26:31]

Please update your CL description to mention the corresponding specification
(http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element...)
and the behavior of other major browsers.

For the record, the HTML specification states:
"The cloning steps for input elements must propagate the value, dirty value
flag, checkedness, and dirty checkedness flag from the node being cloned to the
copy."

[Inactive, 2014-05-06 13:33:57]

https://codereview.chromium.org/267713006/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:913: setFiles(sourceElement.files());
Isn't it a problem that we are not actually cloning the FileList but merely
ref'ing it? You will then have 2 HTMLInputElement sharing the same FileList. Any
change to one HTMLInputElement's FileList will be reflected on the other
HTMLInputElement's FileList.

[Inactive, 2014-05-06 13:40:29]

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/file/input-file-element-clone-expected.txt (right):

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/file/input-file-element-clone-expected.txt:9: PASS
file2.value is "C:\\fakepath\\input-file-element-clone.html"
This is a bad sign that this PASS line is after the TEST COMPLETE line. Your
test will likely be flaky. You likely need to mark your test as asynchronous.

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/file/input-file-element-clone.html (right):

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/file/input-file-element-clone.html:10: description('Make
sure that element.cloneNode works well for Input element type file');
"Make sure that Node.cloneNode() works as expected for file input types."?

[gnana, 2014-05-07 12:39:46]

Thanks Chris! for reviewing.
I have updated the description and applied other review comments. Please have a
look.

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/file/input-file-element-clone-expected.txt (right):

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/file/input-file-element-clone-expected.txt:9: PASS
file2.value is "C:\\fakepath\\input-file-element-clone.html"
On 2014/05/06 13:40:29, Chris Dumez wrote:
> This is a bad sign that this PASS line is after the TEST COMPLETE line. Your
> test will likely be flaky. You likely need to mark your test as asynchronous.

Done.

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/file/input-file-element-clone.html (right):

https://codereview.chromium.org/267713006/diff/40001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/file/input-file-element-clone.html:10: description('Make
sure that element.cloneNode works well for Input element type file');
On 2014/05/06 13:40:29, Chris Dumez wrote:
> "Make sure that Node.cloneNode() works as expected for file input types."?

Done.

https://codereview.chromium.org/267713006/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:913: setFiles(sourceElement.files());
On 2014/05/06 13:33:58, Chris Dumez wrote:
> Isn't it a problem that we are not actually cloning the FileList but merely
> ref'ing it? You will then have 2 HTMLInputElement sharing the same FileList.
Any
> change to one HTMLInputElement's FileList will be reflected on the other
> HTMLInputElement's FileList.
yes. I have modified to clone filelist.
Done.

[Inactive, 2014-05-07 13:28:00]

Here are a few more comments from me.

I'd like tkent to review this as I am not very knowledgeable with this code.

https://codereview.chromium.org/267713006/diff/60001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/file/input-file-element-clone.html (right):

https://codereview.chromium.org/267713006/diff/60001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/file/input-file-element-clone.html:18:
shouldBeEqualToString("file2.value",
"C:\\fakepath\\input-file-element-clone.html");
Would be nice if we would drag another file onto file1 input and make sure that
file2.value does not change (but file1.value changes). Since your first patch
had this issue, I think it makes sense to cover this in your layout test.

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:914: size_t size =
sourceElement.files()->length();
nit: "unsigned" (this is what FileList::length() returns.

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:915: for (size_t i = 0; i < size; i++) {
nit: ++i
nit: Use unsigned

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:916: File* file =
sourceElement.files()->item(i);
HTMLInputElement::files() is not inlined so you might want to cache the
FileList* before the loop.

[gnana, 2014-05-07 14:22:37]

Thanks Chris!
tkent@, please have a look.

https://codereview.chromium.org/267713006/diff/60001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/file/input-file-element-clone.html (right):

https://codereview.chromium.org/267713006/diff/60001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/file/input-file-element-clone.html:18:
shouldBeEqualToString("file2.value",
"C:\\fakepath\\input-file-element-clone.html");
On 2014/05/07 13:28:00, Chris Dumez wrote:
> Would be nice if we would drag another file onto file1 input and make sure
that
> file2.value does not change (but file1.value changes). Since your first patch
> had this issue, I think it makes sense to cover this in your layout test.

Done.

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:914: size_t size =
sourceElement.files()->length();
On 2014/05/07 13:28:00, Chris Dumez wrote:
> nit: "unsigned" (this is what FileList::length() returns.

Done.

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:915: for (size_t i = 0; i < size; i++) {
On 2014/05/07 13:28:00, Chris Dumez wrote:
> nit: ++i
> nit: Use unsigned

Done.

https://codereview.chromium.org/267713006/diff/60001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:916: File* file =
sourceElement.files()->item(i);
On 2014/05/07 13:28:00, Chris Dumez wrote:
> HTMLInputElement::files() is not inlined so you might want to cache the
> FileList* before the loop.

Done.

[Inactive, 2014-05-07 14:26:46]

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/file/input-file-element-clone.html (right):

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/file/input-file-element-clone.html:12: window.onload =
function()
Is the onload really needed? If it works without, we wouldn't need to make the
test async.

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/file/input-file-element-clone.html:16: var clone =
file1.cloneNode(true);
Can you check file1.value?

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/file/input-file-element-clone.html:22:
shouldBeEqualToString("file2.value",
"C:\\fakepath\\input-file-element-clone.html");
Can you check file1.value as well?

[gnana, 2014-05-07 14:49:00]

Applied review comments.

[gnana, 2014-05-07 14:50:08]

Applied review comments.

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/file/input-file-element-clone.html (right):

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/file/input-file-element-clone.html:12: window.onload =
function()
On 2014/05/07 14:26:47, Chris Dumez wrote:
> Is the onload really needed? If it works without, we wouldn't need to make the
> test async.

Done.

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/file/input-file-element-clone.html:16: var clone =
file1.cloneNode(true);
On 2014/05/07 14:26:47, Chris Dumez wrote:
> Can you check file1.value?

Done.

https://codereview.chromium.org/267713006/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/file/input-file-element-clone.html:22:
shouldBeEqualToString("file2.value",
"C:\\fakepath\\input-file-element-clone.html");
On 2014/05/07 14:26:47, Chris Dumez wrote:
> Can you check file1.value as well?

Done.

[tkent, 2014-05-07 23:37:08]

https://codereview.chromium.org/267713006/diff/120001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/120001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:912: if (isFileUpload()) {
Please do not add input-type-specific code to HTMLInputElement. We should
introduce new virtual function to InputType, and FileInputType should override
it.

[gnana, 2014-05-08 06:23:41]

Applied the review comments.
Please have a look.

[tkent, 2014-05-08 07:57:20]

https://codereview.chromium.org/267713006/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:912: if (isFileUpload())
Please do not add type checks to HTMLInputElement.
The code change in this function should be just one line, something like:
    m_inputType->copyNonAttributeProperties(sourceElement);

[gnana, 2014-05-08 08:19:17]

Please have a look.

https://codereview.chromium.org/267713006/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/267713006/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:912: if (isFileUpload())
On 2014/05/08 07:57:20, tkent wrote:
> Please do not add type checks to HTMLInputElement.
> The code change in this function should be just one line, something like:
>     m_inputType->copyNonAttributeProperties(sourceElement);

Done.

[tkent, 2014-05-08 08:22:24]

lgtm

[commit-bot: I haz the power, 2014-05-08 08:23:07]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/267713006/160001

[commit-bot: I haz the power, 2014-05-08 09:31:29]

Change committed as 173626

[tkent, 2014-06-30 23:39:21]

A revert of this CL has been created in
https://codereview.chromium.org/364503002/ by tkent@chromium.org.

The reason for reverting is: Caused a serious regression, crbug.com/388795.
.