Bug 78326 – Do not reformat strings in <input type=number> on platforms using LocalizedNumberICU

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Do not reformat strings in <input type=number> on platforms using LocalizedNumberICU.
Note that LocalizedNumberICU is used only on Chromium port for now.

[Kent Tamura]

Created attachment 126452 [details]
Patch

[WebKit Review Bot]

Attachment 126452 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/tests/LocalizedNumberICUTest.cpp:33:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/platform/text/LocalizedNumberICU.cpp:33:  Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/text/LocalizedNumberICU.cpp:213:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 126452 [details]
Patch

Attachment 126452 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11447568

New failing tests:
fast/speech/input-appearance-numberandspeech.html

[Kent Tamura]

Created attachment 126459 [details]
Patch 2

style errors and test_expectations.txt

[Eric Seidel (no email)]

Comment on attachment 126459 [details]
Patch 2

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

> Source/WebCore/ChangeLog:8
> +        We had buss such as stripping leading zeros, dropping lower digits

bugs?

[Kent Tamura]

Created attachment 127490 [details]
Patch 3

buss

[Hajime Morrita]

Comment on attachment 127490 [details]
Patch 3

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

> Source/WebCore/platform/text/LocalizedNumberICU.cpp:173
> +        if (matches(input, 0, prefix) && matches(input, endIndex - suffix.length(), suffix)) {

isn't it possible to underflow |endIndex - suffix.length()|?

> Source/WebCore/platform/text/LocalizedNumberICU.cpp:203
> +        for (; symbolIndex < DecimalSymbolsSize; ++symbolIndex) {

I hope these have named as a functions.

> Source/WebCore/platform/text/LocalizedNumberICU.h:62
> +    String m_decimalSymbols[12];

nit: Could be m_decimalSymbols[DecimalSymbolsSize] ?

[Kent Tamura]

Comment on attachment 127490 [details]
Patch 3

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

>> Source/WebCore/platform/text/LocalizedNumberICU.cpp:173
>> +        if (matches(input, 0, prefix) && matches(input, endIndex - suffix.length(), suffix)) {
> 
> isn't it possible to underflow |endIndex - suffix.length()|?

it's possible. Thank you for pointing out.
I'll fix it and two instances below.

>> Source/WebCore/platform/text/LocalizedNumberICU.cpp:203
>> +        for (; symbolIndex < DecimalSymbolsSize; ++symbolIndex) {
> 
> I hope these have named as a functions.

What are 'these' pointing? Do you mean this 'for' loop should be a separated function?

>> Source/WebCore/platform/text/LocalizedNumberICU.h:62
>> +    String m_decimalSymbols[12];
> 
> nit: Could be m_decimalSymbols[DecimalSymbolsSize] ?

Will fix.

[Kent Tamura]

Created attachment 130287 [details]
Patch 4

Apply review comments

[Hajime Morrita]

Comment on attachment 130287 [details]
Patch 4

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

ok.

> Source/WebCore/platform/text/LocalizedNumberICU.cpp:142
> +template <class StringType> bool matches(const String& text, unsigned position, const StringType& part)

Can be static or ALWAYS_INLINE to save namespace pollution?

[Kent Tamura]

Committed r109876: <http://trac.webkit.org/changeset/109876>

[Kent Tamura]

Thank you!

(In reply to comment #10)
> > Source/WebCore/platform/text/LocalizedNumberICU.cpp:142
> > +template <class StringType> bool matches(const String& text, unsigned position, const StringType& part)
> 
> Can be static or ALWAYS_INLINE to save namespace pollution?

I added static.