[webkit-reviews] review denied: [Bug 63929] A 'change' event is not triggered on a multiple file form when selected files are changed : [Attachment 99694] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kentaro Hara <haraken at google.com>'s
request for review:
Bug 63929: A 'change' event is not triggered on a multiple file form when
selected files are changed
https://bugs.webkit.org/show_bug.cgi?id=63929

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99694&action=review

> LayoutTests/fast/forms/file-input-change-event.html:19
> +    var single_file_input = document.getElementById("single_file");
> +    var multiple_files_input = document.getElementById("multiple_files");

We don't have a formal JavaScript style guide, but we usually use rules similar
to C++. So variable names and function names should be like singleFileInput.

> Source/WebCore/ChangeLog:19
> +	   * html/HTMLFormControlElement.cpp:
> +	  
(WebCore::HTMLTextFormControlElement::dispatchFileInputFormControlChangeEvent):
Compares the previously selected files and the newly selected files
> +	   * html/HTMLFormControlElement.h:
> +	  
(WebCore::HTMLTextFormControlElement::setPathsAsOfLastFormControlChangeEvent):
Records the newly selected files

This approach is not acceptable.  We don't want to add type-specific data
members to HTMLInputElement.
FileInputType::filesChosen() knows both of the previously-selected files
(m_fileList) and newly-selected files. We can check the need to dispatch
'change' in filesChosen().

> Source/WebCore/html/HTMLFormControlElement.cpp:690
> +    if (pathsChanged) {
> +	   HTMLElement::dispatchChangeEvent();
> +	   setPathsAsOfLastFormControlChangeEvent(paths);
> +    }
> +    setChangedSinceLastFormControlChangeEvent(false);

dispatchChangeEvent() invokes a JavaScript code, and it can delete 'this'
object.  So you can't access 'this' after dispatchChangeEvent().
You can avoid such problem by the protector idiom.