Bug 78326 - Do not reformat strings in <input type=number> on platforms using LocalizedNumberICU
Summary: Do not reformat strings in <input type=number> on platforms using LocalizedNu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 62939 80815
  Show dependency treegraph
 
Reported: 2012-02-09 21:41 PST by Kent Tamura
Modified: 2012-03-11 22:51 PDT (History)
3 users (show)

See Also:


Attachments
Patch (20.68 KB, patch)
2012-02-09 22:12 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (22.20 KB, patch)
2012-02-09 23:00 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (22.23 KB, patch)
2012-02-16 18:13 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (23.54 KB, patch)
2012-03-05 22:13 PST, Kent Tamura
morrita: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-02-09 21:41:39 PST
Do not reformat strings in <input type=number> on platforms using LocalizedNumberICU.
Note that LocalizedNumberICU is used only on Chromium port for now.
Comment 1 Kent Tamura 2012-02-09 22:12:21 PST
Created attachment 126452 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-09 22:14:59 PST
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.
Comment 3 WebKit Review Bot 2012-02-09 22:44:25 PST
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
Comment 4 Kent Tamura 2012-02-09 23:00:03 PST
Created attachment 126459 [details]
Patch 2

style errors and test_expectations.txt
Comment 5 Eric Seidel (no email) 2012-02-16 13:39:02 PST
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?
Comment 6 Kent Tamura 2012-02-16 18:13:29 PST
Created attachment 127490 [details]
Patch 3

buss
Comment 7 Hajime Morrita 2012-03-05 20:50:53 PST
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] ?
Comment 8 Kent Tamura 2012-03-05 21:45:25 PST
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.
Comment 9 Kent Tamura 2012-03-05 22:13:55 PST
Created attachment 130287 [details]
Patch 4

Apply review comments
Comment 10 Hajime Morrita 2012-03-06 00:50:36 PST
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?
Comment 11 Kent Tamura 2012-03-06 01:13:03 PST
Committed r109876: <http://trac.webkit.org/changeset/109876>
Comment 12 Kent Tamura 2012-03-06 01:15:20 PST
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.