Bug 26471 - Redundant 'change' event for <input type=file>
: Redundant 'change' event for <input type=file>
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-16 22:00 PST by
Modified: 2009-06-17 10:01 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-16 22:00:14 PST
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 From 2009-06-16 22:04:05 PST -------
Created an attachment (id=31399) [details]
Proposed patch
------- Comment #2 From 2009-06-16 23:37:42 PST -------
(From update of attachment 31399 [details])
> 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


> +
> +        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.
------- Comment #3 From 2009-06-16 23:39:30 PST -------
> +    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?
------- Comment #4 From 2009-06-17 00:10:36 PST -------
Created an attachment (id=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 From 2009-06-17 10:01:23 PST -------
Landed as http://trac.webkit.org/changeset/44767