Make time input lang attribute aware.
Created attachment 164055 [details] Patch
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.
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
(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 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.
Created attachment 164097 [details] Patch
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 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
(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.
Created attachment 164106 [details] Patch
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
Created attachment 164109 [details] Patch
Comment on attachment 164109 [details] Patch ok
Comment on attachment 164109 [details] Patch Clearing flags on attachment: 164109 Committed r128594: <http://trac.webkit.org/changeset/128594>
All reviewed patches have been landed. Closing bug.