Bug 100128

Summary: [Chromium] Convert Chromium template string to an LDML date format
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: jshin, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 100129    
Bug Blocks: 97877    
Attachments:
Description Flags
Patch
none
Patch 2 tony: review+

Kent Tamura
Reported 2012-10-23 08:48:15 PDT
Need to convert a Chromium template string like "Week $2, $1" to an LDML date format like "'Week 'ww, yyyy"
Attachments
Patch (2.25 KB, patch)
2012-10-24 00:17 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Patch 2 (2.27 KB, patch)
2012-10-25 18:58 PDT, Kent Tamura 		tony: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2012-10-24 00:17:05 PDT
Created attachment 170333 [details] Patch
Kent Tamura
Comment 2 2012-10-24 00:17:28 PDT
See https://codereview.chromium.org/11270005 too.
Tony Chang
Comment 3 2012-10-24 13:43:05 PDT
Comment on attachment 170333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170333&action=review > Source/WebKit/chromium/src/LocalizedStrings.cpp:272 > + Nit: extra blank line? > Source/WebKit/chromium/src/LocalizedStrings.cpp:281 > + unsigned literalStart = 0; > + unsigned length = templ.length(); > + for (unsigned i = 0; i < length; ++i) { > + if (templ[i] == '$' && i + 1 < length && (templ[i + 1] == '1' || templ[i + 1] == '2')) { > + if (literalStart < i) As I mentioned on the Chromium side, we should just put ww and yyyy in the template format. Then we can remove this code. This probably isn't likely, but is it a problem if "ww" or "yyyy" appears in the translation? Do we just match the first occurrence?
Kent Tamura
Comment 4 2012-10-24 20:57:09 PDT
Comment on attachment 170333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170333&action=review >> Source/WebKit/chromium/src/LocalizedStrings.cpp:272 >> + > > Nit: extra blank line? will remove. >> Source/WebKit/chromium/src/LocalizedStrings.cpp:281 >> + if (literalStart < i) > > As I mentioned on the Chromium side, we should just put ww and yyyy in the template format. Then we can remove this code. > > This probably isn't likely, but is it a problem if "ww" or "yyyy" appears in the translation? Do we just match the first occurrence? For LDML date format pattern, we need to quote any latin alphabet. If we didn't quote a string "Week" in a literal part, a letter 'W' would be recognized as week-of-month designator, 'ee' would be 2-digits day-of-week, 'k' would be 1-to-24 hour.
Kent Tamura
Comment 5 2012-10-25 18:58:44 PDT
Created attachment 170785 [details] Patch 2 Remove a blank line. Add a comment
Tony Chang
Comment 6 2012-10-26 10:00:51 PDT
Comment on attachment 170785 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=170785&action=review OK > Source/WebKit/chromium/src/LocalizedStrings.cpp:281 > + for (unsigned i = 0; i < length; ++i) { > + if (templ[i] == '$' && i + 1 < length && (templ[i + 1] == '1' || templ[i + 1] == '2')) { I think you could just make the loop go to length - 1 and remove the i + 1 < length check.
Kent Tamura
Comment 7 2012-10-26 18:46:01 PDT
Comment on attachment 170785 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=170785&action=review >> Source/WebKit/chromium/src/LocalizedStrings.cpp:281 >> + if (templ[i] == '$' && i + 1 < length && (templ[i + 1] == '1' || templ[i + 1] == '2')) { > > I think you could just make the loop go to length - 1 and remove the i + 1 < length check. ok, I'll change the for-loop condition to "i + 1 < length" and remove "i + 1 < length" here
Kent Tamura
Comment 8 2012-10-26 18:54:46 PDT
Committed r132718: <http://trac.webkit.org/changeset/132718>
Note You need to log in before you can comment on or make changes to this bug.