Summary: | Make time input lang attribute aware for testing | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | gustavo, mifenton, philn, tkent, webkit.review.bot, xan.lopez, yosin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 96726 | ||||||||||||
Bug Blocks: | 96351 | ||||||||||||
Attachments: |
|
Description
Keishi Hattori
2012-09-13 23:11:33 PDT
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. |