Use the new ICU plural syntax to fix the strings in the hung pages dialog.

Use the new ICU plural syntax to fix the strings in the hung pages dialog.

BUG=43122
TEST=hang page(s) and verify strings

Committed: https://crrev.com/df73e8c2f83cf17d7c737482b225d42db83f258e
Cr-Commit-Position: refs/heads/master@{#327434}

[jungshik at Google, 2015-04-28 18:08:18]

jshin@chromium.org changed reviewers:
+ jshin@chromium.org

[jungshik at Google, 2015-04-28 18:08:19]

Thank you for quickly coming up with a CL for Mac as well as adding a wrapper
for the plural message API.

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7577: +        other {Pages Unresponsive}}
Did you mean '# Pages Unresponsive' for other? Or, do you want NOT to show the
number of pages in the title? If possible, I'd not show the number in title, but
... 

I'm not sure how ICU will respond when '#' is missing. I have to test. 
And another question is how translators will deal with this string if they don't
see '#' (number). They're used to see ICU syntax plural strings with '#', but
they may not have seen them without '#'. I'll ask l10n-infra folks at Google.

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7582: +        other {The following pages
have become unresponsive. You can wait for them to become responsive or kill
them.}}
same here

[Avi (use Gerrit), 2015-04-28 18:21:59]

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7577: +        other {Pages Unresponsive}}
On 2015/04/28 18:08:18, Jungshik at google wrote:
> Did you mean '# Pages Unresponsive' for other?

The phrasing is intentional here; it felt better without the number.

> I'm not sure how ICU will respond when '#' is missing. I have to test.

In my tests, ICU works fine if you don't have that # in there.

> And another question is how translators will deal with this string if they
don't
> see '#' (number). They're used to see ICU syntax plural strings with '#', but
> they may not have seen them without '#'. I'll ask l10n-infra folks at Google. 

Thanks. I don't have much of an idea, though.

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7582: +        other {The following pages
have become unresponsive. You can wait for them to become responsive or kill
them.}}
On 2015/04/28 18:08:18, Jungshik at google wrote:
> same here

same here :)

[jungshik at Google, 2015-04-28 19:24:50]

Thank you for the reply. I'll get back here after talking to another l10n-infra
folk.

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7577: +        other {Pages Unresponsive}}
On 2015/04/28 18:21:59, Avi wrote:
> On 2015/04/28 18:08:18, Jungshik at google wrote:
> > Did you mean '# Pages Unresponsive' for other?
> 
> The phrasing is intentional here; it felt better without the number.
> 

Yes, I agree that it feels more natural without the number in English, but see
below. 

> > I'm not sure how ICU will respond when '#' is missing. I have to test.
> 
> In my tests, ICU works fine if you don't have that # in there.

Yeah, I've also confirmed it. 

> 
> > And another question is how translators will deal with this string if they
> don't
> > see '#' (number). They're used to see ICU syntax plural strings with '#',
but
> > they may not have seen them without '#'. I'll ask l10n-infra folks at
Google. 
> 
> Thanks. I don't have much of an idea, though.

I've just talked to a l10n-infra folk. He told me that there are messages like
this in Google's string database. So, Google's translators/linguists have seen
and dealt with them. 

A thorny issue is that what is perfectly natural for English can be really
awkward for languages like Russian. That is, when number-dependent plural forms
are used without an explicit number, it sounds very bad in Russian (and possibly
other languages).  Some people even believe that we should ban this kind of
string (plural msg without a number). 

It's not clear what to do about this. I'm gonna talk to another person once I
grab him.

[Avi (use Gerrit), 2015-04-28 19:59:07]

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7577: +        other {Pages Unresponsive}}
On 2015/04/28 19:24:50, Jungshik at google wrote:
> It's not clear what to do about this. I'm gonna talk to another person once I
> grab him. 

For English it feels right to not have a number there. Can I indicate to the
translators that they have the freedom to put the # in for the title if they
feel it works better?

[jungshik at Google, 2015-04-28 21:57:55]

On 2015/04/28 19:59:07, Avi wrote:
>
https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1107273002/diff/20001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:7577: +        other {Pages Unresponsive}}
> On 2015/04/28 19:24:50, Jungshik at google wrote:
> > It's not clear what to do about this. I'm gonna talk to another person once
I
> > grab him. 
> 
> For English it feels right to not have a number there. Can I indicate to the
> translators that they have the freedom to put the # in for the title if they
> feel it works better?

I hope they'll read the description :-) Anyway, I guess it would not hurt and I
like the idea.  (see below)

Update: Good news. My worry about Russian turned out to be overblown. Russian
does have a 'generic plural' (that can be used
when # is absent). Therefore, as long as translators do the right thing by using
'generic plural' for all plural cases, we're fine.
 
Some languages may not have 'generic plural' and users may end up seeing 'Paga'
for 23 and 'Pagi' for 25 (but without '23' or '25' shown). 

If linguists read your description, they will add back '#' as they see fit. 

BTW, I heard that there are quite a lot of plural messages without '#' (number
place holder) in Google's translation string db.  Our l10n infra folks will talk
to lead/head linguists to add instructions on 'plural message without #' so that
actual translators would know what to do. That way, developers (we) wouldn't
have to add to description what to do for each message. 

Thanks !

[Avi (use Gerrit), 2015-04-28 22:43:07]

avi@chromium.org changed reviewers:
+ mark@chromium.org, pkasting@chromium.org

[Avi (use Gerrit), 2015-04-28 22:43:07]

pkasting: Views ui and general
mark: All the Cocoa stuff
jungshik: The localization stuff

[Peter Kasting, 2015-04-28 23:02:51]

LGTM

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:240: info_label_(new
views::Label()),
Init this with nullptr here and allocate it in Init() so we don't leak in case
somehow control flow never reaches Init().

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:417: // Do an estimate here so we
can calculate some sizing information.
Why do we need to do this?  It doesn't seem like we need to calculate anything
size-related right now, as we re-Layout() once we have the right string.

I would just omit the SetText() call unless it actually matters for something.

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:446: // Add the label with a
preferred width of 1, this way it doesn't effect the
Nit: While here: can you fix "effect" to be "affect"?

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.h (right):

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.h:159: views::Label* info_label_;
Nit: Place this above |hung_pages_table_| to match the object init/layout order
in Init().

While here, I would also move |frozen_icon_| up to be above both of these.

[Avi (use Gerrit), 2015-04-29 00:18:18]

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:240: info_label_(new
views::Label()),
On 2015/04/28 23:02:51, Peter Kasting wrote:
> Init this with nullptr here and allocate it in Init() so we don't leak in case
> somehow control flow never reaches Init().

Done.

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:417: // Do an estimate here so we
can calculate some sizing information.
On 2015/04/28 23:02:51, Peter Kasting wrote:
> Why do we need to do this?  It doesn't seem like we need to calculate anything
> size-related right now, as we re-Layout() once we have the right string.
> 
> I would just omit the SetText() call unless it actually matters for something.

I have no idea. Scott wrote this code.

Scott?

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:446: // Add the label with a
preferred width of 1, this way it doesn't effect the
On 2015/04/28 23:02:51, Peter Kasting wrote:
> Nit: While here: can you fix "effect" to be "affect"?

Done.

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.h (right):

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.h:159: views::Label* info_label_;
On 2015/04/28 23:02:51, Peter Kasting wrote:
> Nit: Place this above |hung_pages_table_| to match the object init/layout
order
> in Init().
> 
> While here, I would also move |frozen_icon_| up to be above both of these.

Done.

[Peter Kasting, 2015-04-29 00:20:08]

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:417: // Do an estimate here so we
can calculate some sizing information.
On 2015/04/29 00:18:17, Avi wrote:
> On 2015/04/28 23:02:51, Peter Kasting wrote:
> > Why do we need to do this?  It doesn't seem like we need to calculate
anything
> > size-related right now, as we re-Layout() once we have the right string.
> > 
> > I would just omit the SetText() call unless it actually matters for
something.
> 
> I have no idea. Scott wrote this code.

You mean Scott wrote the old code here, or Scott wrote the code in your patch?

The old code was fine because we set the string once and never touched it again,
so setting it here was appropriate.

[Avi (use Gerrit), 2015-04-29 00:27:05]

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/1107273002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:417: // Do an estimate here so we
can calculate some sizing information.
On 2015/04/29 00:20:08, Peter Kasting wrote:
> You mean Scott wrote the old code here, or Scott wrote the code in your patch?

I mean Scott wrote the Views changes in this patch.

I don't know why he changed the code this way. Does a views::GridLayout require
a contained views::Label to have some text?

[Avi (use Gerrit), 2015-04-29 00:32:59]

On 2015/04/28 21:57:55, Jungshik at google wrote:
> I hope they'll read the description :-) Anyway, I guess it would not hurt and
I
> like the idea.  (see below)

I edited the descriptions; let me know what you think.

[jungshik at Google, 2015-04-29 00:33:41]

LGTM for plural changes.

[Mark Mentovai, 2015-04-29 00:40:24]

Mac stuff LGTM

[Avi (use Gerrit), 2015-04-29 01:00:32]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-04-29 01:00:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1107273002/#ps100001
(title: "grd descriptions")

[commit-bot: I haz the power, 2015-04-29 01:04:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1107273002/100001

[Peter Kasting, 2015-04-29 01:11:59]

The CQ bit was unchecked by pkasting@chromium.org

[Peter Kasting, 2015-04-29 01:12:47]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-04-29 01:14:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1107273002/100001

[commit-bot: I haz the power, 2015-04-29 04:19:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-29 04:20:00]

Patchset 6 (id:??) landed as
https://crrev.com/df73e8c2f83cf17d7c737482b225d42db83f258e
Cr-Commit-Position: refs/heads/master@{#327434}