Localize strings, speeds.

Localize strings, speeds.

BUG=86527
TEST=run in non-English. For European languages, during a download the decimal separators should be commas (e.g. "0,0 MB"). (The speeds are in strings files and might take a little time to run through the translation machinery.)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90092

[Avi (use Gerrit), 2011-06-20 23:27:48]

WDYT?

[Evan Stade, 2011-06-21 03:18:41]

+jshin

overall patch looks great

http://codereview.chromium.org/7189076/diff/2001/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/2001/base/i18n/number_formatting....
base/i18n/number_formatting.cc:45:
g_number_format_int.Get().number_format.get();
this is the best line of code ever

http://codereview.chromium.org/7189076/diff/2001/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/7189076/diff/2001/base/string_util.cc#newcode617
base/string_util.cc:617: double unit_amount = static_cast<double> (bytes);
don't need the space before (bytes)

http://codereview.chromium.org/7189076/diff/2001/base/string_util.cc#newcode627
base/string_util.cc:627: if (bytes != 0 && dimension > 0 && unit_amount < 100)
curlies

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings.grd
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings....
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
does this really vary by locale?

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
File ui/base/text/bytes_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.cc:79: 1024LL*1024*1024*1024*1024  //
DATA_UNITS_PEBIBYTE,
spaces around operators (also you might do 1 << 20 or 1 << 30 etc. instead of
stacking 1024 after 1024)

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formatting.h
File ui/base/text/bytes_formatting.h (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.h:35: string16 FormatBytes(int64 bytes, DataUnits
units, bool show_units);
I think there should be a version of this function that just uses NATURAL for
units, since you want that 98% of the time

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.h:38: // Ex: FormatSpeed(512, DATA_UNITS_KIBIBYTE,
true) => "0.5 KB/s"
should the k be lower case?

[Avi (use Gerrit), 2011-06-21 14:30:19]

http://codereview.chromium.org/7189076/diff/2001/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/2001/base/i18n/number_formatting....
base/i18n/number_formatting.cc:45:
g_number_format_int.Get().number_format.get();
On 2011/06/21 03:18:41, Evan Stade wrote:
> this is the best line of code ever

You're welcome. I'm here all week.

http://codereview.chromium.org/7189076/diff/2001/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/7189076/diff/2001/base/string_util.cc#newcode617
base/string_util.cc:617: double unit_amount = static_cast<double> (bytes);
On 2011/06/21 03:18:41, Evan Stade wrote:
> don't need the space before (bytes)

Done.

http://codereview.chromium.org/7189076/diff/2001/base/string_util.cc#newcode627
base/string_util.cc:627: if (bytes != 0 && dimension > 0 && unit_amount < 100)
On 2011/06/21 03:18:41, Evan Stade wrote:
> curlies

Done.

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings.grd
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings....
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
On 2011/06/21 03:18:41, Evan Stade wrote:
> does this really vary by locale?

:( Did you just propose that surely everyone in the world does this exactly the
same way we do? The fact that they don't is the whole _point_ of this CL.

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
File ui/base/text/bytes_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.cc:79: 1024LL*1024*1024*1024*1024  //
DATA_UNITS_PEBIBYTE,
On 2011/06/21 03:18:41, Evan Stade wrote:
> spaces around operators (also you might do 1 << 20 or 1 << 30 etc. instead of
> stacking 1024 after 1024)

Done.

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formatting.h
File ui/base/text/bytes_formatting.h (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.h:35: string16 FormatBytes(int64 bytes, DataUnits
units, bool show_units);
As a tweak later, perhaps; for now I think this is OK.

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.h:38: // Ex: FormatSpeed(512, DATA_UNITS_KIBIBYTE,
true) => "0.5 KB/s"
On 2011/06/21 03:18:41, Evan Stade wrote:
> should the k be lower case?

Done.

[Evan Stade, 2011-06-21 22:10:57]

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings.grd
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings....
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
On 2011/06/21 14:30:19, Avi wrote:
> On 2011/06/21 03:18:41, Evan Stade wrote:
> > does this really vary by locale?
> 
> :( Did you just propose that surely everyone in the world does this exactly
the
> same way we do? The fact that they don't is the whole _point_ of this CL.

This wasn't a helpful answer to my question, so I went and searched the web. It
appears certain alphabets such as Cyrillic, Arabic, and Hebrew do not use kB for
kilobytes (even though most Asian and Latin scripts do).

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formatting.h
File ui/base/text/bytes_formatting.h (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.h:35: string16 FormatBytes(int64 bytes, DataUnits
units, bool show_units);
On 2011/06/21 14:30:19, Avi wrote:
> As a tweak later, perhaps; for now I think this is OK.

I don't think this "tweak" will ever be more relevant than it is now.

[Avi (use Gerrit), 2011-06-21 22:20:07]

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings.grd
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings....
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
Some omit the space between the number and the label. Some (particularly France)
use "octet" rather than "byte". Most do use the same tags, but not all do...

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formatting.h
File ui/base/text/bytes_formatting.h (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/text/bytes_formattin...
ui/base/text/bytes_formatting.h:35: string16 FormatBytes(int64 bytes, DataUnits
units, bool show_units);
Working on it. Just a moment...

[Evan Stade, 2011-06-21 22:26:18]

LGTM after the change you're working on, although I still think jshin might have
some input.

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings.grd
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/2001/ui/base/strings/app_strings....
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
On 2011/06/21 22:20:07, Avi wrote:
> Some omit the space between the number and the label. Some (particularly
France)
> use "octet" rather than "byte". Most do use the same tags, but not all do...

it seems subtle that the space is important, so it's probably good to notate
that somewhere.

[Avi (use Gerrit), 2011-06-21 23:39:16]

ptal

[jungshik at Google, 2011-06-22 00:35:00]

Looks good ! Thank you for the change. 

I have a few informational comments along with one comment about testing in
non-en-US locales.

http://codereview.chromium.org/7189076/diff/11001/base/i18n/number_formatting.h
File base/i18n/number_formatting.h (right):

http://codereview.chromium.org/7189076/diff/11001/base/i18n/number_formatting...
base/i18n/number_formatting.h:15: // Ex: FormatNumber(1234567) => "1,234,567"
nit: To make it clear that this is locale-dependent, you may list the results in
two locales :

"1,234,567" in English, "1.234.567" in German

http://codereview.chromium.org/7189076/diff/11001/base/i18n/number_formatting...
base/i18n/number_formatting.h:19: // Ex: FormatDouble(1234567.0, 1) =>
"1,234,567.0"
nit : same here : 1,234,567.8 in English , 1.234.567,8 in German
(I think 1234567.8 is more interesting).

http://codereview.chromium.org/7189076/diff/11001/ui/base/strings/app_strings...
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/11001/ui/base/strings/app_strings...
ui/base/strings/app_strings.grd:385: <ph name="QUANTITY">$1<ex>42</ex></ph>
bytes
Hmm.. I'm afraid we need to use plural support here. We do use  ICU plural
formatter for strings like (x seconds left, x mins left, etc). It's done a long
while ago when we didn't have a good plural support in our "internal" string
translation flow.  So, it's rather clumsy to generalize to this case. 

Now that we have a better support internally, we can make a switch to a plural
string support used internally.  Perhaps, it's better to go ahead with the
current CL and TODO comment on that.  And, later, we can use the following CL
(once it's done): 

http://codereview.chromium.org/6736003/

The above CL was put on hold because it needs some change/test in grit - xmb -
xtb work flow. 

In short, it's fine to go ahead with this CL as it is now.

http://codereview.chromium.org/7189076/diff/11001/ui/base/strings/app_strings...
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
FYI: With floating point numbers, plural gets very 'fuzzy'. The current CLDR
treats all floating point numbers as 'other' . There's no point of trying to use
plural formats at the moment.

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
File ui/base/text/bytes_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
ui/base/text/bytes_formatting.cc:62: result =
l10n_util::GetStringFUTF16(suffix[units], result);
FYI: When plural format is used later (not now) for 'bytes',  'bytes' case has
to be special-cased to use ICU's PluralFormat API
(http://icu-project.org/apiref/icu4c/classPluralFormat.html ) because suffix
changes forms depending on the number. It might be possible to use ICU's generic
MessageFormat API for all the cases, but it seems unnecessary here.

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
File ui/base/text/bytes_formatting_unittest.cc (right):

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
ui/base/text/bytes_formatting_unittest.cc:77: }
Can you do something similar to what's done in
base/i18n/time_formatting_unittest.cc (to test in a couple of interesting
locales, e.g. French and German) ? Just testing 'number formatting' portion
should suffice for now.

[Avi (use Gerrit), 2011-06-22 16:48:24]

http://codereview.chromium.org/7189076/diff/11001/base/i18n/number_formatting.h
File base/i18n/number_formatting.h (right):

http://codereview.chromium.org/7189076/diff/11001/base/i18n/number_formatting...
base/i18n/number_formatting.h:15: // Ex: FormatNumber(1234567) => "1,234,567"
On 2011/06/22 00:35:01, Jungshik Shin wrote:
> nit: To make it clear that this is locale-dependent, you may list the results
in
> two locales :
> 
> "1,234,567" in English, "1.234.567" in German

Done.

http://codereview.chromium.org/7189076/diff/11001/base/i18n/number_formatting...
base/i18n/number_formatting.h:19: // Ex: FormatDouble(1234567.0, 1) =>
"1,234,567.0"
On 2011/06/22 00:35:01, Jungshik Shin wrote:
> nit : same here : 1,234,567.8 in English , 1.234.567,8 in German
> (I think 1234567.8 is more interesting). 

Done.

http://codereview.chromium.org/7189076/diff/11001/ui/base/strings/app_strings...
File ui/base/strings/app_strings.grd (right):

http://codereview.chromium.org/7189076/diff/11001/ui/base/strings/app_strings...
ui/base/strings/app_strings.grd:385: <ph name="QUANTITY">$1<ex>42</ex></ph>
bytes
I think I was being too clever turning this into the string "bytes" from the "B"
it was before. Reverting that change.

http://codereview.chromium.org/7189076/diff/11001/ui/base/strings/app_strings...
ui/base/strings/app_strings.grd:388: <ph name="QUANTITY">$1<ex>42.0</ex></ph> kB
On 2011/06/22 00:35:01, Jungshik Shin wrote:
> FYI: With floating point numbers, plural gets very 'fuzzy'. The current CLDR
> treats all floating point numbers as 'other' . There's no point of trying to
use
> plural formats at the moment. 

:)

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
File ui/base/text/bytes_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
ui/base/text/bytes_formatting.cc:62: result =
l10n_util::GetStringFUTF16(suffix[units], result);
Moving back to "B" for now will fix this. BTW, "bytes" is broken even in
English; I don't know what I was thinking.

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
File ui/base/text/bytes_formatting_unittest.cc (right):

http://codereview.chromium.org/7189076/diff/11001/ui/base/text/bytes_formatti...
ui/base/text/bytes_formatting_unittest.cc:77: }
I'm afraid that doing that will break as soon as the translations come in. I'm
going to add some unit tests to the number formatting utilities instead.

[Avi (use Gerrit), 2011-06-22 18:24:33]

ptal

[Evan Stade, 2011-06-22 18:42:20]

lgtm

[Avi (use Gerrit), 2011-06-22 20:07:29]

Mark: approval plz from base/ OWNER?

[Mark Mentovai, 2011-06-22 20:24:10]

LGTM. I only reviewed the changes in base.

http://codereview.chromium.org/7189076/diff/15031/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/15031/base/i18n/number_formatting...
base/i18n/number_formatting.cc:41: static
Rather than staticating these, just move them up a couple lines to put them into
the anonymous namespace.

http://codereview.chromium.org/7189076/diff/15031/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/7189076/diff/15031/base/string_util.h#newcode413
base/string_util.h:413: // FormatBytes instead; remove this.
Where is FormatBytes? It’s not in this file any longer. You should reference the
header or namespace it’s declared in to help people find it. The only reason I
was able to find it was by looking at the rest of your change.

[Avi (use Gerrit), 2011-06-22 20:27:37]

http://codereview.chromium.org/7189076/diff/15031/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/7189076/diff/15031/base/i18n/number_formatting...
base/i18n/number_formatting.cc:41: static
On 2011/06/22 20:24:11, Mark Mentovai wrote:
> Rather than staticating these, just move them up a couple lines to put them
into
> the anonymous namespace.

Done.

http://codereview.chromium.org/7189076/diff/15031/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/7189076/diff/15031/base/string_util.h#newcode413
base/string_util.h:413: // FormatBytes instead; remove this.
It's in ui/base as noted.

[Mark Mentovai, 2011-06-22 20:29:22]

http://codereview.chromium.org/7189076/diff/15031/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/7189076/diff/15031/base/string_util.h#newcode413
base/string_util.h:413: // FormatBytes instead; remove this.
Avi wrote:
> It's in ui/base as noted.

I have no clue how I missed that.

[Avi (use Gerrit), 2011-06-22 20:33:19]

rdsmith: Approval from net/ OWNER plz?

[Evan Stade, 2011-06-22 20:38:18]

I thought we decided it was OK to TBR owner approval for these kinds of api
changes

[Randy Smith (Not in Mondays), 2011-06-22 20:38:35]

On 2011/06/22 20:33:19, Avi wrote:
> rdsmith: Approval from net/ OWNER plz?

Rubber-stamp LGTM based on approval from evan, who appears to be the last person
who touched that code (I don't know net_util.cc particularly).