Bug 96728

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

96726

Bug Blocks:

96351

Attachments:

Description	Flags
Patch	tkent: review-
Patch	none
Patch	none
Patch	none

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