Bug 100128 – [Chromium] Convert Chromium template string to an LDML date format

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

Need to convert a Chromium template string like "Week $2, $1" to an LDML date format like "'Week 'ww, yyyy"

[Kent Tamura]

Created attachment 170333 [details]
Patch

[Kent Tamura]

See https://codereview.chromium.org/11270005 too.

[Tony Chang]

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 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]

Created attachment 170785 [details]
Patch 2

Remove a blank line. Add a comment

[Tony Chang]

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 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]

Committed r132718: <http://trac.webkit.org/changeset/132718>