Bug 96728 - Make time input lang attribute aware for testing
Summary: Make time input lang attribute aware for testing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on: 96726
Blocks: 96351
  Show dependency treegraph
 
Reported: 2012-09-13 23:11 PDT by Keishi Hattori
Modified: 2012-09-14 05:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (13.32 KB, patch)
2012-09-13 23:31 PDT, Keishi Hattori
tkent: review-
Details | Formatted Diff | Diff
Patch (15.70 KB, patch)
2012-09-14 04:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (16.78 KB, patch)
2012-09-14 04:44 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (16.73 KB, patch)
2012-09-14 05:00 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-09-13 23:11:33 PDT
Make time input lang attribute aware.
Comment 1 Keishi Hattori 2012-09-13 23:31:08 PDT
Created attachment 164055 [details]
Patch
Comment 2 Kent Tamura 2012-09-13 23:47:19 PDT
Comment on attachment 164055 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6369
> +    if (locale.isEmpty() || !RuntimeEnabledFeatures::langAttributeAwareFormControlUIEnabled())
> +        return getLocalizer(defaultLanguage());

We don't need to use recursive call, which makes code readability worse.

AtomicString localeKey = locale;
if (locale.isEmpty() || !RuntimeF...)
    localeKey = defaultLanguage();
LocaleToLocalizerMap::AddResult result = m_localizerCache.add(localeKey, nullptr);
....

> Source/WebCore/dom/Document.cpp:6370
> +    LocaleToLocalizerMap::AddResult result = m_localizerCache.add(locale.impl(), nullptr);

Should use AtomicString as a key.
Using (Atomic)StringImpl as a HashMap key is dangerous. A key AtomicStringImpl can be destructed outside of the HashMap.

> Source/WebCore/dom/Document.cpp:6372
> +        result.iterator->second = Localizer::create(locale);

We need the default implementation of Localizer::create() for platforms other than ICU/Mac/Windows.
Comment 3 Kent Tamura 2012-09-14 00:08:23 PDT
We need to complete the followings before proceeding this bug:
 - Introduce LocalizerNone.cpp
 - Fix an issue that Localizer::create("broken-locale-identifier") doesn't return the browser locale
Comment 4 Kent Tamura 2012-09-14 00:09:16 PDT
(In reply to comment #3)
>  - Fix an issue that Localizer::create("broken-locale-identifier") doesn't return the browser locale

This is about LocaleMac.mm.
Comment 5 yosin 2012-09-14 00:12:06 PDT
Comment on attachment 164055 [details]
Patch

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

> LayoutTests/fast/forms/time/time-localization.html:1
> +<!DOCTYPE html>

This test should be named time-multiple-fields-localization.html. This test checks multiple fields time input UI.

> LayoutTests/fast/forms/time/time-localization.html:23
> +for (var i = 0; i < inputs.length; i++) {

nit: ++i
In C++, we usually use ++i instead of i++, prefix operator is usually faster than postfix operator.
Comment 6 Keishi Hattori 2012-09-14 04:03:23 PDT
Created attachment 164097 [details]
Patch
Comment 7 Keishi Hattori 2012-09-14 04:04:26 PDT
Comment on attachment 164055 [details]
Patch

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

>> LayoutTests/fast/forms/time/time-localization.html:1
>> +<!DOCTYPE html>
> 
> This test should be named time-multiple-fields-localization.html. This test checks multiple fields time input UI.

Done.

>> LayoutTests/fast/forms/time/time-localization.html:23
>> +for (var i = 0; i < inputs.length; i++) {
> 
> nit: ++i
> In C++, we usually use ++i instead of i++, prefix operator is usually faster than postfix operator.

Done.

>> Source/WebCore/dom/Document.cpp:6369
>> +        return getLocalizer(defaultLanguage());
> 
> We don't need to use recursive call, which makes code readability worse.
> 
> AtomicString localeKey = locale;
> if (locale.isEmpty() || !RuntimeF...)
>     localeKey = defaultLanguage();
> LocaleToLocalizerMap::AddResult result = m_localizerCache.add(localeKey, nullptr);
> ....

Done.

>> Source/WebCore/dom/Document.cpp:6370
>> +    LocaleToLocalizerMap::AddResult result = m_localizerCache.add(locale.impl(), nullptr);
> 
> Should use AtomicString as a key.
> Using (Atomic)StringImpl as a HashMap key is dangerous. A key AtomicStringImpl can be destructed outside of the HashMap.

Done.
Comment 8 Kent Tamura 2012-09-14 04:08:01 PDT
Comment on attachment 164097 [details]
Patch

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

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-localization.html:24
> +    debug(getUserAgentShadowTextContent(inputs[i]));

How about
    debug(inputs[i].lang + ': ' + getUserAgentShadowTextContent(inputs[i]));
?

> LayoutTests/platform/chromium-mac/fast/forms/time-multiple-fields/time-multiple-fields-localization-expected.txt:5
> +

Please show a message that the following lines depend on locale data in the system.

> LayoutTests/platform/chromium-mac/fast/forms/time-multiple-fields/time-multiple-fields-localization-expected.txt:20
> +TEST COMPLETE
> +EN
> +AR
> +FR
> +JA
> +KO
> +CN

Showing languages after "TEST COMPLETE" doesn't make much sense.

> Source/WebCore/ChangeLog:34
> +        (DateTimeEditElement):
> +
> +2012-09-13  Keishi Hattori  <keishi@webkit.org>
> +
> +        Refactor time format related methods on LocaleWin/Mac/ICU so that they override Localizer methods

Two ChangeLog entries.

> Source/WebKit/chromium/ChangeLog:4
> +2012-09-13  Keishi Hattori  <keishi@webkit.org>
> +
> +        Refactor time format related methods on LocaleWin/Mac/ICU so that they override Localizer methods
> +        https://bugs.webkit.org/show_bug.cgi?id=96726

Unrelated ChangeLog entry
Comment 9 Kent Tamura 2012-09-14 04:10:07 PDT
(In reply to comment #8)
> > LayoutTests/platform/chromium-mac/fast/forms/time-multiple-fields/time-multiple-fields-localization-expected.txt:5
> > +
> 
> Please show a message that the following lines depend on locale data in the system.

Also, I expect Chromium-win and Chromium-linux need their own test result. So we'd better add TextExpectation entry.
Comment 10 Keishi Hattori 2012-09-14 04:44:57 PDT
Created attachment 164106 [details]
Patch
Comment 11 Kent Tamura 2012-09-14 04:55:53 PDT
Comment on attachment 164106 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=96726

96726 -> 96728
Comment 12 Keishi Hattori 2012-09-14 05:00:05 PDT
Created attachment 164109 [details]
Patch
Comment 13 Kent Tamura 2012-09-14 05:38:43 PDT
Comment on attachment 164109 [details]
Patch

ok
Comment 14 Kent Tamura 2012-09-14 05:40:04 PDT
Comment on attachment 164109 [details]
Patch

Clearing flags on attachment: 164109

Committed r128594: <http://trac.webkit.org/changeset/128594>
Comment 15 Kent Tamura 2012-09-14 05:40:09 PDT
All reviewed patches have been landed.  Closing bug.