[webkit-reviews] review denied: [Bug 63929] A 'change' event is not triggered on a multiple file form when selected files are changed : [Attachment 99774] 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 99774: Patch
https://bugs.webkit.org/attachment.cgi?id=99774&action=review

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

> LayoutTests/fast/forms/file-input-change-event.html:93
> +function SingleFileSelected() {
> +    onChangeCalled = true;
> +}
> +
> +function MultipleFilesSelected() {
> +    onChangeCalled = true;
> +}
> +
> +function moveMouseToCenterOfElement(element) {

Inconsistent names.
SingleFileSelected -> singleFileSelected
MultipleFileSelected -> multipleFileSelected

> Source/WebCore/html/FileInputType.cpp:310
> +	   // This call may cause destruction of this instance and thus must
always be last in the function.
> +	   input->HTMLElement::dispatchChangeEvent();
> +    }
> +    input->setChangedSinceLastFormControlChangeEvent(false);
>  }

The comment is lame. "This call" isn't on the last of the function.
In this case, you don't use 'this' after the dispatchChangeEvent() call.  So
it's safe about 'this'.

However 'input' might be deleted by dispatchChangeEvent().  You need to protect
'input' by replacing:
 HTMLInputElement* input = element();
at the beginning of the function with
 RefPtr<HTMLInputElement> input = element();