[Webkit-unassigned] [Bug 49245] <input type=file multiple /> button should say "Choose File(s)" instead of "Choose File"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49245


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99072|review?                     |review-
               Flag|                            |




--- Comment #15 from Kent Tamura <tkent at chromium.org>  2011-06-29 18:15:56 PST ---
(From update of attachment 99072)
View in context: https://bugs.webkit.org/attachment.cgi?id=99072&action=review

> LayoutTests/platform/chromium/test_expectations.txt:227
> +BUGWK49245 SKIP : fast/forms/input-file-label.html = FAIL
> +
> +// This test fails because the label of a multiple file chooser button is updated.
> +// We need to update expected.txt and expected.png.
> +BUGWK49245 SKIP : fast/forms/input-file-re-render.html = IMAGE+TEXT FAIL

Do not SKIP them.  We should run failing tests unless they are very slow because we'd like to confirm they cause no crashes and let buildbots generate test results.

> LayoutTests/platform/gtk/Skipped:1538
> +# This test fails because the label of a multiple file chooser button is updated.
> +# We need to update expected.txt and expected.png.
> +fast/forms/input-file-re-render.html

I don't think we should skip a test which needs rebaseline.
If we skip it, we can't get test results from buildbots and one who has no failing platforms can't correct the expectations.

> Source/WebCore/ChangeLog:9
> +        Changed a label of the HTML5 file chooser button to "Choose Files",
> +        since it enables us to choose multiple files.

We don't need to repeat the summary sentence.

"since ..." is unclear.  We should write reasons/benefit of the change.
"We should notify capability of multiple files to users."?

> Source/WebCore/ChangeLog:36
> +        * English.lproj/Localizable.strings:
> +        * html/FileInputType.cpp:
> +        (WebCore::UploadButtonElement::create):
> +        (WebCore::UploadButtonElement::createForMultiple):
> +        (WebCore::FileInputType::createShadowSubtree):
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::parseMappedAttribute):
> +        * platform/DefaultLocalizationStrategy.cpp:
> +        (WebCore::DefaultLocalizationStrategy::fileButtonChooseMultipleFilesLabel):
> +        * platform/DefaultLocalizationStrategy.h:
> +        * platform/LocalizationStrategy.h:
> +        * platform/LocalizedStrings.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/LocalizedStrings.h:
> +        * platform/brew/LocalizedStringsBrew.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/efl/LocalizedStringsEfl.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/gtk/LocalizedStringsGtk.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/haiku/LocalizedStringsHaiku.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/wx/LocalizedStringsWx.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):

You had better write summaries of changes fo each of files/functions.

> Source/WebCore/html/HTMLInputElement.cpp:734
> +    } else if (attr->name() == multipleAttr) {
> +        m_inputType->destroyShadowSubtree();
> +        m_inputType->createShadowSubtree();
> +        setNeedsValidityCheck();

Type=email doesn't need to destroy and create the shadow tree for 'multiple' attribute change. This code is inefficient.
So, I recommend you introduce a member function for InputType, and override it in FileInputType.

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:131
> +    return WEB_UI_STRING("Choose Files", "title for file button used in HTML5 file chooser");

The description string should mention:
 - The message is for multiple files.
 - The message should be short as possible.

> Source/WebKit/chromium/ChangeLog:15
> +        * public/WebLocalizedString.h:
> +        * src/LocalizedStrings.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):

You need to update WebKit/chromium/DEPS so that chromium_rev >= 91051.

> Source/WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:177
> +    return QCoreApplication::translate("QWebPage", "Choose Files", "title for file button used in HTML5 file chooser");

ditto for the description.

> Source/WebKit/wince/WebCoreSupport/PlatformStrategiesWinCE.cpp:144
> +    return UI_STRING("Choose Files", "title for file button used in HTML5 file chooser");

ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.