96728 – Make time input lang attribute aware for testing

RESOLVED FIXED 96728

Make time input lang attribute aware for testing

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

Summary Make time input lang attribute aware for testing

Keishi Hattori

Reported 2012-09-13 23:11:33 PDT

Make time input lang attribute aware.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Keishi Hattori

Comment 1 2012-09-13 23:31:08 PDT

Created attachment 164055 [details] Patch

Kent Tamura

Comment 2 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.

Kent Tamura

Comment 3 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

Kent Tamura

Comment 4 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.

yosin

Comment 5 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.

Keishi Hattori

Comment 6 2012-09-14 04:03:23 PDT

Created attachment 164097 [details] Patch

Keishi Hattori

Comment 7 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.

Kent Tamura

Comment 8 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

Kent Tamura

Comment 9 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.

Keishi Hattori

Comment 10 2012-09-14 04:44:57 PDT

Created attachment 164106 [details] Patch

Kent Tamura

Comment 11 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

Keishi Hattori

Comment 12 2012-09-14 05:00:05 PDT

Created attachment 164109 [details] Patch

Kent Tamura

Comment 13 2012-09-14 05:38:43 PDT

Comment on attachment 164109 [details] Patch ok

Kent Tamura

Comment 14 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>

Kent Tamura

Comment 15 2012-09-14 05:40:09 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Forms

Assignee

Keishi Hattori

Reported

2012-09-13 23:11 PDT

Modified

2012-09-14 05:40 PDT History

CC List

7 users Show

URL

Keywords

Depends on

96726

Blocks

96351

Dependencies

tree graph