Summary: | Do not reformat strings in <input type=number> on platforms using LocalizedNumberICU | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||
Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, morrita, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 62939, 80815 | ||||||||||||
Attachments: |
|
Description
Kent Tamura
2012-02-09 21:41:39 PST
Created attachment 126452 [details]
Patch
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 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 Created attachment 126459 [details]
Patch 2
style errors and test_expectations.txt
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? Created attachment 127490 [details]
Patch 3
buss
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 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. Created attachment 130287 [details]
Patch 4
Apply review comments
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? Committed r109876: <http://trac.webkit.org/changeset/109876> 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. |