Use ICU for string pluralization in the extension permission dialog.

Use ICU for string pluralization in the extension permission dialog.

This adds support for using the correct plural form of a string for any
language. In particular, this uses the aforementioned support in the
extensions permission dialog for website access and retained file
access.

BUG=290046
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224700

[benwells, 2013-09-12 23:52:20]

lgtm

[Peter Kasting, 2013-09-12 23:57:54]

This is problematic.  Many languages have constraints around strings relating to
numbers, beyond just English's "singular" and "plural"; for example, some
languages need different word forms for multiples of ten, while others need
different forms for odds versus evens.

Could you please talk to an l10n expert about how to correctly handle _all_
languages' necessary cases before checking this in?  I pinged tc@ and asked if
he'd reply here.

[tony, 2013-09-13 00:12:37]

See go/plurals , which also links to
http://unicode.org/repos/cldr-tmp/trunk/diff/supplemental/language_plural_rul...
.

I think it's OK if you reword the sentence to be something like, "Files with
permanent access: $1."  You can try reaching out to plurals@google.com for a
expert advice.

We do use ICU, so it's possible to use their plural formatting, but it's kind of
a pain to set up in grit (it actually doesn't look like we do this anymore).

[benwells, 2013-09-16 19:32:35]

On 2013/09/13 00:12:37, tony wrote:
> See go/plurals , which also links to
>
http://unicode.org/repos/cldr-tmp/trunk/diff/supplemental/language_plural_rul...
> .
> 
> I think it's OK if you reword the sentence to be something like, "Files with
> permanent access: $1."  You can try reaching out to mailto:plurals@google.com
for a
> expert advice.
> 
> We do use ICU, so it's possible to use their plural formatting, but it's kind
of
> a pain to set up in grit (it actually doesn't look like we do this anymore).

Sorry about the delay. Sam brought this up before putting up this patch.

It seems like we have three options:
1. Have a plural insensitive string. E.g. what was here before or Tony's
suggestion
2. Handle the simple plural case - that is, singular and multiple - with
different strings. For locales where this doesn't work we can fall back to 1. by
using the same string for the singular and multiple cases.
3. Go all out and use ICU.

It would be a shame to only do 1. because it is too hard to do 3.

2. Seems like the sweet spot. Is there a way we can do that, e.g. by expanding
the instructions for translators?

[Peter Kasting, 2013-09-16 20:25:04]

On 2013/09/16 19:32:35, benwells wrote:
> It seems like we have three options:
> 1. Have a plural insensitive string. E.g. what was here before or Tony's
> suggestion
> 2. Handle the simple plural case - that is, singular and multiple - with
> different strings. For locales where this doesn't work we can fall back to 1.
by
> using the same string for the singular and multiple cases.
> 3. Go all out and use ICU.
> 
> It would be a shame to only do 1. because it is too hard to do 3.
> 
> 2. Seems like the sweet spot. Is there a way we can do that, e.g. by expanding
> the instructions for translators?

2 is not a sweet spot.  2 is _worse_ for many languages than 1.  You're creating
problems instead of solving them, so don't pick 2 no matter what you do.

We already have to deal with this problem in Chrome strings many other places,
e.g. "n seconds remaining until your download completes".  Find out how we solve
the problem there.

[Sam McNally, 2013-09-16 22:31:30]

On 2013/09/16 20:25:04, Peter Kasting wrote:
> On 2013/09/16 19:32:35, benwells wrote:
> > It seems like we have three options:
> > 1. Have a plural insensitive string. E.g. what was here before or Tony's
> > suggestion
> > 2. Handle the simple plural case - that is, singular and multiple - with
> > different strings. For locales where this doesn't work we can fall back to
1.
> by
> > using the same string for the singular and multiple cases.
> > 3. Go all out and use ICU.
> > 
> > It would be a shame to only do 1. because it is too hard to do 3.
> > 
> > 2. Seems like the sweet spot. Is there a way we can do that, e.g. by
expanding
> > the instructions for translators?
> 
> 2 is not a sweet spot.  2 is _worse_ for many languages than 1.  You're
creating
> problems instead of solving them, so don't pick 2 no matter what you do.
> 
> We already have to deal with this problem in Chrome strings many other places,
> e.g. "n seconds remaining until your download completes".  Find out how we
solve
> the problem there.

It isn't pretty, but it's done.

[benwells, 2013-09-19 01:22:03]

+tony for review

Wow. I'll defer review to tc who obviously knows more about this than I do.

Sam: thanks for putting in the effort to get this right.
Peter: thanks for speaking up and showing us the path to the correct solution.

[tony, 2013-09-19 17:00:39]

https://codereview.chromium.org/23591040/diff/32001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23591040/diff/32001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: +        Access your data on <ph
name="NUMBER_OF_OTHER_WEBSITES">$1<ex>5</ex></ph> websites
Will this work in all languages?

https://codereview.chromium.org/23591040/diff/32001/chrome/browser/extensions...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/23591040/diff/32001/chrome/browser/extensions...
chrome/browser/extensions/extension_install_prompt.cc:352: const
icu::UnicodeString kKeywords[] = {
This is basically copied from TimeFormatter::BuildFormats().  Please move this
into a helper function in ui/base/l10n/.  Maybe in l10n_util?

[Sam McNally, 2013-09-20 04:10:55]

https://codereview.chromium.org/23591040/diff/32001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23591040/diff/32001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: +        Access your data on <ph
name="NUMBER_OF_OTHER_WEBSITES">$1<ex>5</ex></ph> websites
On 2013/09/19 17:00:40, tony wrote:
> Will this work in all languages?

Done.

https://codereview.chromium.org/23591040/diff/32001/chrome/browser/extensions...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/23591040/diff/32001/chrome/browser/extensions...
chrome/browser/extensions/extension_install_prompt.cc:352: const
icu::UnicodeString kKeywords[] = {
On 2013/09/19 17:00:40, tony wrote:
> This is basically copied from TimeFormatter::BuildFormats().  Please move this
> into a helper function in ui/base/l10n/.  Maybe in l10n_util?

Done.

[tony, 2013-09-20 16:56:43]

This looks great.  LGTM, with some minor comments.

https://codereview.chromium.org/23591040/diff/74001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23591040/diff/74001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4062: +               desc="A line of
explanatory text that precedes the list of files the app has permanent access
to, showing how the number of files. This is shown when an app has persistent
access to files. NUMBER_ONE is one or one-like numbers : 1 (many European and
most Indian languages), 1 and 0 (French, Brazilian Portuguese and Hindi),
1,21,31, .. (Russian, Ukrainian, Croatian, Serbian, Latvian, Lithuanian), or 1,
101, 201, .. (Slovenian). Do NOT translate this for CJK, Vietnamese, Turkish and
Kannada">
NUMBER_ONE should be replaced with NUMBER_OF_FILES (or change the <ph> name to
NUMBER_ONE).  Same with all the other messages.

https://codereview.chromium.org/23591040/diff/74001/ui/base/l10n/l10n_util_in...
File ui/base/l10n/l10n_util_internal.h (right):

https://codereview.chromium.org/23591040/diff/74001/ui/base/l10n/l10n_util_in...
ui/base/l10n/l10n_util_internal.h:6: // expose ICU types in their interface.
Nit: I have a slight preference for a file name like l10n_util_plurals.h over
internal, which doesn't really say what's in the file.  FWIW, we expose ICU in
l10n_util_collator.h already.  Alternately, in a follow up change, we could move
the non-exported icu functions in l10n_util_collator.h into
l10n_util_internal.h.

https://codereview.chromium.org/23591040/diff/74001/ui/base/l10n/l10n_util_in...
ui/base/l10n/l10n_util_internal.h:25: scoped_ptr<icu::PluralFormat>
BuildPluralFormat(std::vector<int> message_ids);
|message_ids| should be a const ref.

[Sam McNally, 2013-09-22 22:53:03]

https://codereview.chromium.org/23591040/diff/74001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23591040/diff/74001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4062: +               desc="A line of
explanatory text that precedes the list of files the app has permanent access
to, showing how the number of files. This is shown when an app has persistent
access to files. NUMBER_ONE is one or one-like numbers : 1 (many European and
most Indian languages), 1 and 0 (French, Brazilian Portuguese and Hindi),
1,21,31, .. (Russian, Ukrainian, Croatian, Serbian, Latvian, Lithuanian), or 1,
101, 201, .. (Slovenian). Do NOT translate this for CJK, Vietnamese, Turkish and
Kannada">
On 2013/09/20 16:56:44, tony wrote:
> NUMBER_ONE should be replaced with NUMBER_OF_FILES (or change the <ph> name to
> NUMBER_ONE).  Same with all the other messages.

Done.

https://codereview.chromium.org/23591040/diff/74001/ui/base/l10n/l10n_util_in...
File ui/base/l10n/l10n_util_internal.h (right):

https://codereview.chromium.org/23591040/diff/74001/ui/base/l10n/l10n_util_in...
ui/base/l10n/l10n_util_internal.h:6: // expose ICU types in their interface.
On 2013/09/20 16:56:44, tony wrote:
> Nit: I have a slight preference for a file name like l10n_util_plurals.h over
> internal, which doesn't really say what's in the file.  FWIW, we expose ICU in
> l10n_util_collator.h already.  Alternately, in a follow up change, we could
move
> the non-exported icu functions in l10n_util_collator.h into
> l10n_util_internal.h.

Done.

https://codereview.chromium.org/23591040/diff/74001/ui/base/l10n/l10n_util_in...
ui/base/l10n/l10n_util_internal.h:25: scoped_ptr<icu::PluralFormat>
BuildPluralFormat(std::vector<int> message_ids);
On 2013/09/20 16:56:44, tony wrote:
> |message_ids| should be a const ref.

Done.

[commit-bot: I haz the power, 2013-09-22 23:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/23591040/92001

[commit-bot: I haz the power, 2013-09-23 10:30:56]

Change committed as 224700

[jungshik at Google, 2013-09-23 19:33:33]

Retroactive LGTM for the last patch set :-)

On 2013/09/13 00:12:37, tony wrote:
> See go/plurals , which also links to
>
http://unicode.org/repos/cldr-tmp/trunk/diff/supplemental/language_plural_rul...

> We do use ICU, so it's possible to use their plural formatting, but it's kind
of
> a pain to set up in grit (it actually doesn't look like we do this anymore).

Tony, what problem do you see with handling plurals the way it's done internally
at Google as described in go/plural? Chrome 'pioneered' plural handling before
any of Google products did and we decided to use multi-message approaches (6
msgs total) as is done here, but I think we eventually have to move to the
Google internal way because our translators are now familiar with that approach.
More importantly, I believe it'll scale better for other strings (that need
proper plural handling). 

Well, I'd better take this offline. I'll send you an email.