Bug 26471 - Redundant 'change' event for <input type=file>
: Redundant 'change' event for <input type=file>
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Kent Tamura
Depends on:
  Show dependency treegraph
Reported: 2009-06-16 22:00 PDT by Kent Tamura
Modified: 2009-06-17 10:01 PDT (History)
0 users

See Also:

Proposed patch (1.73 KB, patch)
2009-06-16 22:04 PDT, Kent Tamura
fishd: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (1.52 KB, patch)
2009-06-17 00:10 PDT, Kent Tamura
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-06-16 22:00:14 PDT
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
Comment 1 Kent Tamura 2009-06-16 22:04:05 PDT
Created attachment 31399 [details]
Proposed patch
Comment 2 Darin Fisher (:fishd, Google) 2009-06-16 23:37:42 PDT
Comment on attachment 31399 [details]
Proposed patch

> Index: WebCore/ChangeLog
> +2009-06-16  Kent Tamura  <tkent@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +	Don't fire redundant 'change' events for a file upload form.

nit: please replace tabs with whitespaces

> +

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.
Comment 3 Darin Adler 2009-06-16 23:39:30 PDT
> +    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)

Comment 4 Kent Tamura 2009-06-17 00:10:36 PDT
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.
Comment 5 Darin Fisher (:fishd, Google) 2009-06-17 10:01:23 PDT
Landed as http://trac.webkit.org/changeset/44767