Bug 26471 – Redundant 'change' event for <input type=file>

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

The current implementation of FileChooser.cpp produces some redundant 'change' events in a case that the selected file is not changed.

It makes a problem only in Chrome for now, and it is possible to resolve the problem by changing the Chrome code.  However to change the WebKit code is safer and reasonable.
See http://crbug.com/14162

[Kent Tamura]

Created attachment 31399 [details]
Proposed patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 31399 [details]
Proposed patch

> Index: WebCore/ChangeLog
...
> [phone]  Kent Tamura  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +	Don't fire redundant 'change' events for a file upload form.

nit: please replace tabs with whitespaces


> +
> +        WARNING: NO TEST CASES ADDED OR CHANGED

nit: you can delete this line.

it is also good practice to include a link to this bug in this ChangeLog entry.


> +        * platform/FileChooser.cpp:
> +        (WebCore::FileChooser::chooseFiles): Do nothing if there are no existing selected files and no incoming selected files, or if the size of both of existing selected files and incoming selected files is 1 and filenames are identical.

nit: this can probably be shortened a bit.  it is good to mention what you changed, but it
would probably be enough to say "suppress change event if an empty path is chosen when
m_filenames is empty."  <-  just documents what you added


Otherwise, this change looks good to me.  I think this is good to add to
WebCore since it may similarly benefit other ports.

[Darin Adler]

> +    if (m_filenames.isEmpty() && filenames.isEmpty())
> +        return;
> +    if (m_filenames.size() == 1 && filenames.size() == 1 && m_filenames[0] == filenames[0])
> +        return;

How about just:

    if (m_filenames == filenames)

here?

[Kent Tamura]

Created attachment 31403 [details]
Proposed patch (rev.2)

Thank you for the comments.  I have revised the patch.
I confirmed "m_filenames == filenames" worked fine.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/44767