[webkit-reviews] review denied: [Bug 49245] <input type=file multiple /> button should say "Choose File(s)" instead of "Choose File" : [Attachment 98917] 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 49245: <input type=file multiple /> button should say "Choose File(s)"
instead of "Choose File"
https://bugs.webkit.org/show_bug.cgi?id=49245

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>

View in context: https://bugs.webkit.org/attachment.cgi?id=98917&action=review

r- because of Chromium build failure.


> LayoutTests/ChangeLog:5
> +	   Change a label of the HTML5 file chooser button to "Choose One or
More Files"

Do you have other ideas of shorter label?
"Choose One or More Files" looks too long, and makes the file name area very
short.

> LayoutTests/fast/forms/input-file-label.html:15
> +function print(s) {
> +    document.getElementById('console').textContent += s + '\n';
> +}

You don't need this function.  You can use debug(), which is defined in
js-test-pre.js.

> LayoutTests/fast/forms/input-file-label.html:18
> +    layoutTestController.dumpAsText();

You don't need to call dumpAsText(). js-test-pre.js already called it.

> LayoutTests/fast/forms/input-file-label.html:23
> +    print((label == 'Choose File' ? 'PASS' : 'FAIL') + ': The label of a
single file chooser was "' + label + '".');

You can use testPassed() and testFailed() defined in js-test-pre.js.

> LayoutTests/fast/forms/input-file-label.html:28
> +    print((label == 'Choose One or More Files' ? 'PASS' : 'FAIL') + ': The
label of a multiple file chooser was "' + label + '".');

ditto.

> Source/WebCore/html/FileInputType.cpp:49
> +    static PassRefPtr<UploadButtonElement> create(Document*, bool multiple);


bool parameter is not good.
 - Define an enum, or
 - Introduce another factory method, like "createForMultiple()"

> Source/WebKit/chromium/public/WebLocalizedString.h:47
> +// This macro is a temporal macro to resolve the timing dependency
> +// between the following Chromium patch and WebKit patch:
> +// Chromium patch: http://codereview.chromium.org/7273024/
> +// WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=49245
> +//
> +// This macro guarantees that
> +// WebLocalizedString::FileButtonChooseMultipleFilesLabel exists
> +// in WebKit if and only if
> +// WebLocalizedString::FileButtonChooseMultipleFilesLabel exists
> +// in Chromium.
> +// See http://codereview.chromium.org/7273024/ for more details.
> +#define WE_WILL_REMOVE_THIS_MACRO_SOON

See my comment for the Chromium change.