i18n of several browser messages

i18n of several browser messages

BUG=613683
TEST=Switch to language with non-Arabic numerals (e.g., Persian), view
the various messages described in 613683, check that numbers
are rendered with non-Arabic numerals.

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

[Greg Levin, 2016-05-20 20:14:57]

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

[Greg Levin, 2016-05-20 20:14:57]

Here's the next one.  Running out the door for the weekend, will find other
reviewers next week :-)

[jungshik at Google, 2016-05-20 23:58:04]

See my comments below. All of them need to use plural formats.

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/bookmarks...
File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right):

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/bookmarks...
chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:144:
base::FormatNumber(child_count))) ==
This need to use a plural format (GetPluralStringFUTF16). Actually, I caught
this in my search for |<ex>[0-9]+</ex> file:grd| in cs and included in my cl
(not yet uploaded) for http://crbug.com/613331 .

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/cri...
File chrome/browser/ui/views/critical_notification_bubble_view.cc (right):

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.cc:89:
base::FormatNumber(seconds))
This one is also caught in my search for <ex>[0-9]+</ex> and I was getting there
(but not yet). Why don't you just go ahead with the following?


This also needs to use plural format or a measure format (with seconds). 

<ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will restart in <ph
name="SECONDS">$2<ex>10</ex></ph> seconds.

In grd, the message spec has to be :

{1, plural, 
  =1{<ph name="PRODUCT_NAME">{0}<ex>Google Chrome</ex></ph> will restart in a
second.}
  other{<ph name="PRO...</ph> will restart in # seconds.}
}

Note that there are two input values to pass (one is string and the other is
integer). So, GetPluralStringFUTF16 helper cannot be used. You have to use

MessageFormatter::FormatWithNumberedArgs(GetStringUTF16(<msg id>), 
GetStringUTF16(IDS_PRODUCT_NAME), seconds);

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:564:
base::FormatNumber(i.allowed));
Hmm... both message are like "<count> from this site". 
Depending on <count>, 'from'-equivalent in other languages may inflect even
though it's not '<count> cookies from this site'. 

I think the message has to be changed to '<count> cookies from this site' and
plural formatter has to be used. 

Even if we don't change, we'd better use plural formatting to be safe: 

In grd,  

{NUM_COOKIES, plural, one{# from this site}, other{# from this site}}

In the source, GetPluralStringFUTF16() has to be used. 
  

If you search for 'plural file:grd', you'll find a lot of examples.

[Greg Levin, 2016-05-26 14:03:14]

jshin@, please have another look, and if okay, I'll add reviewers for individual
files.  Thanks!

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/bookmarks...
File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right):

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/bookmarks...
chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:144:
base::FormatNumber(child_count))) ==
On 2016/05/20 23:58:03, jshin (slow- 23-26) wrote:
> This need to use a plural format (GetPluralStringFUTF16). Actually, I caught
> this in my search for |<ex>[0-9]+</ex> file:grd| in cs and included in my cl
> (not yet uploaded) for http://crbug.com/613331 . 

Ok, reverted my change to this file in next patch.

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/cri...
File chrome/browser/ui/views/critical_notification_bubble_view.cc (right):

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/cri...
chrome/browser/ui/views/critical_notification_bubble_view.cc:89:
base::FormatNumber(seconds))
On 2016/05/20 23:58:03, jshin (slow- 23-26) wrote:
> This one is also caught in my search for <ex>[0-9]+</ex> and I was getting
there
> (but not yet). Why don't you just go ahead with the following?
> 
> 
> This also needs to use plural format or a measure format (with seconds). 
> 
> <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will restart in <ph
> name="SECONDS">$2<ex>10</ex></ph> seconds.
> 
> In grd, the message spec has to be :
> 
> {1, plural, 
>   =1{<ph name="PRODUCT_NAME">{0}<ex>Google Chrome</ex></ph> will restart in a
> second.}
>   other{<ph name="PRO...</ph> will restart in # seconds.}
> }
> 
> Note that there are two input values to pass (one is string and the other is
> integer). So, GetPluralStringFUTF16 helper cannot be used. You have to use
> 
> MessageFormatter::FormatWithNumberedArgs(GetStringUTF16(<msg id>), 
> GetStringUTF16(IDS_PRODUCT_NAME), seconds); 
> 

Done.

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2002673003/diff/1/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:564:
base::FormatNumber(i.allowed));
On 2016/05/20 23:58:04, jshin (slow- 23-26) wrote:
> Hmm... both message are like "<count> from this site". 
> Depending on <count>, 'from'-equivalent in other languages may inflect even
> though it's not '<count> cookies from this site'. 
> 
> I think the message has to be changed to '<count> cookies from this site' and
> plural formatter has to be used. 
> 
> Even if we don't change, we'd better use plural formatting to be safe: 
> 
> In grd,  
> 
> {NUM_COOKIES, plural, one{# from this site}, other{# from this site}}
> 
> In the source, GetPluralStringFUTF16() has to be used. 
>   
> 
> If you search for 'plural file:grd', you'll find a lot of examples. 

Done.

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

https://codereview.chromium.org/2002673003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:8903: +         =1 {<ph
name="PRODUCT_NAME">{0}<ex>Google Chrome</ex></ph> will restart in 1 second.}
** Presubmit Warnings **
Don't use PRODUCT_NAME placeholders in string resources. Instead, add separate
strings to google_chrome_strings.grd and chromium_strings.grd. See
http://goo.gl/6614MQ for more information.

The link doesn't say anything about needed to clean up existing cases.  Any
reason I shouldn't separate these as suggested anyway?

https://codereview.chromium.org/2002673003/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/2002673003/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:732:
l10n_util::GetStringUTF8(IDS_WEBSITE_SETTINGS_FIRST_PARTY_SITE_DATA);
As far as I can find in code search, this member is being written to, but never
read.  However, CookieInfo is used in a lot of strange generated and templatized
code.  I'm not sure if removing this member is correct; I did it to provoke a
code review from someone who knows how this stuct is used, and whether removing
this is actually okay.  But since I'm changing the way these format strings
work, I figured *something* should be done here.

[Greg Levin, 2016-05-26 14:03:21]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 14:03:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002673003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002673003/20001

[commit-bot: I haz the power, 2016-05-26 14:09:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-26 14:09:19]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Greg Levin, 2016-05-27 16:58:02]

glevin@chromium.org changed reviewers:
+ palmer@chromium.org

[Greg Levin, 2016-05-27 16:58:03]

palmer@ - I'd like you to review all the website_settings stuff, but you can
hold off until jshin@ gives the OK on the string i18n code.

In the mean time, could you specifically take a look at my comment in:
https://codereview.chromium.org/2002673003/diff/20001/chrome/browser/ui/websi...
Do you know if it's safe and reasonable to remove
WebsiteSettingsUI::CookieInfo::cookie_source?

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

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:7: #include
"base/i18n/message_formatter.h"
TODO: Remove in next patch

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:12:
#include "base/i18n/number_formatting.h"
TODO: Remove in next patch

[jungshik at Google, 2016-05-27 17:53:12]

Thank you for the update. I wonder why you moved away from using
'IDS_PRODUCT_NAME'.

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

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:89:
IDS_CRITICAL_NOTIFICATION_HEADLINE_ALTERNATE);
You can use l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) and there's no need to
keep two parallel strings in two grd files. 

Any reason to avoid using IDS_PRODUCT_NAME? perf? Want to use a shorter 'Chrome'
instead of 'Google Chrome' ?  

If you do that, instead of a convenience wrapper GetPluralStringFUTF16, you have
to use MessageFormatter directrly with two input parameters (one is for product
name and the other is seconds). That's what I suggested in my previous comment
(quoted again below):

> In grd, the message spec has to be :
> 
> {1, plural, 
>   =1{<ph name="PRODUCT_NAME">{0}<ex>Google Chrome</ex></ph> will restart in a
> second.}
>   other{<ph name="PRO...</ph> will restart in # seconds.}
> }
> 
> Note that there are two input values to pass (one is string and the other is
> integer). So, GetPluralStringFUTF16 helper cannot be used. You have to use
> 
> MessageFormatter::FormatWithNumberedArgs(GetStringUTF16(<msg id>), 
> GetStringUTF16(IDS_PRODUCT_NAME), seconds); 
> 

Done.

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:139:
message->SetText(l10n_util::GetStringUTF16(IDS_CRITICAL_NOTIFICATION_TEXT));
Any reason NOT to use a common message along with
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ?

[Greg Levin, 2016-05-27 17:58:40]

On 2016/05/27 17:53:12, jshin (slow- 23-26) wrote:
> Thank you for the update. I wonder why you moved away from using
> 'IDS_PRODUCT_NAME'.
>  [...]
> Any reason NOT to use a common message along with
> l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ?

Yes, this was generating

 ** Presubmit Warnings **
 Don't use PRODUCT_NAME placeholders in string resources. Instead, add
 separate strings to google_chrome_strings.grd and chromium_strings.grd.
 See http://goo.gl/6614MQ for more information.

and was causing the chromium_presubmit trybot to fail on Patch 2.

[jungshik at Google, 2016-05-27 18:19:15]

On 2016/05/27 17:58:40, Greg Levin wrote:
> On 2016/05/27 17:53:12, jshin (slow- 23-26) wrote:
> > Thank you for the update. I wonder why you moved away from using
> > 'IDS_PRODUCT_NAME'.
> >  [...]
> > Any reason NOT to use a common message along with
> > l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ?
> 
> Yes, this was generating
> 
>  ** Presubmit Warnings **
>  Don't use PRODUCT_NAME placeholders in string resources. Instead, add
>  separate strings to google_chrome_strings.grd and chromium_strings.grd.
>  See http://goo.gl/6614MQ for more information.
> 
> and was causing the chromium_presubmit trybot to fail on Patch 2.

I see. Thanks. 

LGTM, then.

[jungshik at Google, 2016-05-27 18:20:41]

On 2016/05/27 18:19:15, jungshik at google wrote:
> On 2016/05/27 17:58:40, Greg Levin wrote:
> > On 2016/05/27 17:53:12, jshin (slow- 23-26) wrote:
> > > Thank you for the update. I wonder why you moved away from using
> > > 'IDS_PRODUCT_NAME'.
> > >  [...]
> > > Any reason NOT to use a common message along with
> > > l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ?
> > 
> > Yes, this was generating
> > 
> >  ** Presubmit Warnings **
> >  Don't use PRODUCT_NAME placeholders in string resources. Instead, add
> >  separate strings to google_chrome_strings.grd and chromium_strings.grd.
> >  See http://goo.gl/6614MQ for more information.
> > 
> > and was causing the chromium_presubmit trybot to fail on Patch 2.
> 
> I see. Thanks. 
> 
> LGTM, then.

Now I remember a conversation I had with estade@ back then (2012) :-) Banging my
head ....

[jungshik at Google, 2016-05-27 18:21:43]

On 2016/05/27 18:20:41, jungshik at google wrote:
> On 2016/05/27 18:19:15, jungshik at google wrote:
> > On 2016/05/27 17:58:40, Greg Levin wrote:
> > > On 2016/05/27 17:53:12, jshin (slow- 23-26) wrote:
> > > > Thank you for the update. I wonder why you moved away from using
> > > > 'IDS_PRODUCT_NAME'.
> > > >  [...]
> > > > Any reason NOT to use a common message along with
> > > > l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ?
> > > 
> > > Yes, this was generating
> > > 
> > >  ** Presubmit Warnings **
> > >  Don't use PRODUCT_NAME placeholders in string resources. Instead, add
> > >  separate strings to google_chrome_strings.grd and chromium_strings.grd.
> > >  See http://goo.gl/6614MQ for more information.
> > > 
> > > and was causing the chromium_presubmit trybot to fail on Patch 2.
> > 
> > I see. Thanks. 
> > 
> > LGTM, then.
> 
> Now I remember a conversation I had with estade@ back then (2012) :-) Banging
my
> head ....

Sorry for spamming. Obviously, unnecessary header inclusion has to be taken care
of (as you noted already) before checking this in.

[Chris Palmer, 2016-05-27 18:30:29]

palmer@google.com changed reviewers:
+ palmer@google.com

[Chris Palmer, 2016-05-27 18:30:30]

lgtm

[Greg Levin, 2016-05-27 18:30:51]

glevin@chromium.org changed reviewers:
+ pkasting@google.com
- palmer@google.com

[Greg Levin, 2016-05-27 18:30:52]

pkasting@ - critical_notification_bubble_view.cc

palmer@ - all website_settings files
(and specifically commenting on cookie_source removal mentioned in previous
comment)

Please have a look!

[Peter Kasting, 2016-05-27 18:48:29]

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

[Peter Kasting, 2016-05-27 18:48:30]

LGTM

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

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:89:
IDS_CRITICAL_NOTIFICATION_HEADLINE_ALTERNATE);
On 2016/05/27 17:53:12, jungshik at google wrote:
> You can use l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) and there's no need to
> keep two parallel strings in two grd files. 
> 
> Any reason to avoid using IDS_PRODUCT_NAME?

I thought the l10n folks asked us to phase out substituting IDS_PRODUCT_NAME
into strings, and instead split things into separate strings in separate branded
vs. unbranded resource files, for better localizability.

In other words, I think Greg is doing the right thing.

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:139:
message->SetText(l10n_util::GetStringUTF16(IDS_CRITICAL_NOTIFICATION_TEXT));
On 2016/05/27 17:53:12, jungshik at google wrote:
> Any reason NOT to use a common message along with
> l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ? 

Same comment.

[Greg Levin, 2016-05-27 19:31:47]

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

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:7: #include
"base/i18n/message_formatter.h"
On 2016/05/27 16:58:03, Greg Levin wrote:
> TODO: Remove in next patch

Done.

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:89:
IDS_CRITICAL_NOTIFICATION_HEADLINE_ALTERNATE);
On 2016/05/27 18:48:29, Peter Kasting wrote:
> On 2016/05/27 17:53:12, jungshik at google wrote:
> > You can use l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) and there's no need
to
> > keep two parallel strings in two grd files. 
> > 
> > Any reason to avoid using IDS_PRODUCT_NAME?
> 
> I thought the l10n folks asked us to phase out substituting IDS_PRODUCT_NAME
> into strings, and instead split things into separate strings in separate
branded
> vs. unbranded resource files, for better localizability.
> 
> In other words, I think Greg is doing the right thing.

Acknowledged.

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/critical_notification_bubble_view.cc:139:
message->SetText(l10n_util::GetStringUTF16(IDS_CRITICAL_NOTIFICATION_TEXT));
On 2016/05/27 18:48:29, Peter Kasting wrote:
> On 2016/05/27 17:53:12, jungshik at google wrote:
> > Any reason NOT to use a common message along with
> > l10n_util::GetStringUTF16(IDS_PRODUCT_NAME) ? 
> 
> Same comment.

Acknowledged.

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2002673003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:12:
#include "base/i18n/number_formatting.h"
On 2016/05/27 16:58:03, Greg Levin wrote:
> TODO: Remove in next patch

Done.

[Greg Levin, 2016-05-27 19:32:03]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2016-05-27 19:32:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org,
pkasting@chromium.org, palmer@google.com
Link to the patchset: https://codereview.chromium.org/2002673003/#ps80001
(title: "Remove another unneeded header")

[commit-bot: I haz the power, 2016-05-27 19:32:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002673003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002673003/80001

[commit-bot: I haz the power, 2016-05-27 21:08:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-27 21:11:06]

Description was changed from

==========
i18n of several browser messages

BUG=613683
TEST=Switch to language with non-Arabic numerals (e.g., Persian), view
the various messages described in 613683, check that numbers
are rendered with non-Arabic numerals.
==========

to

==========
i18n of several browser messages

BUG=613683
TEST=Switch to language with non-Arabic numerals (e.g., Persian), view
the various messages described in 613683, check that numbers
are rendered with non-Arabic numerals.

Committed: https://crrev.com/e6ffe53e5189609c8d93784c31ce347d63eed693
Cr-Commit-Position: refs/heads/master@{#396558}
==========

[commit-bot: I haz the power, 2016-05-27 21:11:07]

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