i18n of Copyright line to use locally correct numeric glyphs

i18n of Copyright line to use locally correct numeric glyphs

BUG=613583
TEST=Switch to language with non-Arabic numerals (e.g., Persian), open
Settings > About or chrome://settings to view Copyright line. Check that
year is rendered with non-Arabic numerals.

Committed: https://crrev.com/8d231d3546984ff7d4b22a8a97bd162b599f7e9f
Cr-Commit-Position: refs/heads/master@{#396527}

[Greg Levin, 2016-05-20 16:54:51]

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

[Greg Levin, 2016-05-20 16:54:52]

jshin@, could you take a first look at this?  It's straight-forward, but I'm
wondering if I should use a time-formatting function, rather than just
FormatNumber(year).

I'd actually like to create a helper function somewhere, like

string16 GetCopyrightLine();

since all 4 of these use identical Time::Exploded code to fill in the
IDS_ABOUT_VERSION_COPYRIGHT format string.  But where's a good place to put a
l10n function with access to IDS_ strings?  ui/base/l10n/l10n_util.cc?

[jungshik at Google, 2016-05-21 00:18:15]

On 2016/05/20 16:54:52, Greg Levin wrote:
> jshin@, could you take a first look at this?  It's straight-forward, but I'm
> wondering if I should use a time-formatting function, rather than just
> FormatNumber(year).
> 
> I'd actually like to create a helper function somewhere, like
> 
> string16 GetCopyrightLine();
> 
> since all 4 of these use identical Time::Exploded code to fill in the
> IDS_ABOUT_VERSION_COPYRIGHT format string.  But where's a good place to put a
> l10n function with access to IDS_ strings?  ui/base/l10n/l10n_util.cc?

base/i18n does not have an access to IDS_* (string bundle). So, l10n_util.cc
would be the best fit.

PS#1 looks good to me, but if you wanna add a helper, go ahead. 

BTW, I guess that the last file (Mac file) does not use GetStringFUTF16()
because chrome/tools/mac_helpers cannot use ui/base/l10n.

[jungshik at Google, 2016-05-21 00:32:01]

On 2016/05/20 16:54:52, Greg Levin wrote:
> jshin@, could you take a first look at this?  It's straight-forward, but I'm
> wondering if I should use a time-formatting function, rather than just
> FormatNumber(year).

There are a few different ways. One of them is to use MessageFormatter() and
pass base::Time::Now() (no exploding/year extraction).
with the following message in grd. 

{0, Copyright <ph name="YEAR">{0,date,y}</ph> The Chromium Authors. All rights
reserved.}

i18n::base::MessagFormatter::FormatWithNumberedArgs(l10n_util::GetStringUTF16(IDS_...),
base::Time::Now());

[jungshik at Google, 2016-05-21 00:32:53]

On 2016/05/21 00:32:01, jshin (jungshik at google) wrote:
> On 2016/05/20 16:54:52, Greg Levin wrote:
> > jshin@, could you take a first look at this?  It's straight-forward, but I'm
> > wondering if I should use a time-formatting function, rather than just
> > FormatNumber(year).
> 
> There are a few different ways. One of them is to use MessageFormatter() and
> pass base::Time::Now() (no exploding/year extraction).
> with the following message in grd. 
> 
> {0, Copyright <ph name="YEAR">{0,date,y}</ph> The Chromium Authors. All rights
> reserved.}

Oops. I did too many plurals.  The msg should read :

Copyright <ph name="YEAR">{0,date,y}</ph> The Chromium Authors. All rights
reserved.


> 
>
i18n::base::MessagFormatter::FormatWithNumberedArgs(l10n_util::GetStringUTF16(IDS_...),
> base::Time::Now());

[jungshik at Google, 2016-05-21 00:33:39]

On 2016/05/21 00:32:53, jshin (jungshik at google) wrote:
> On 2016/05/21 00:32:01, jshin (jungshik at google) wrote:
> > On 2016/05/20 16:54:52, Greg Levin wrote:
> > > jshin@, could you take a first look at this?  It's straight-forward, but
I'm
> > > wondering if I should use a time-formatting function, rather than just
> > > FormatNumber(year).
> > 
> > There are a few different ways. One of them is to use MessageFormatter() and
> > pass base::Time::Now() (no exploding/year extraction).
> > with the following message in grd. 
> > 
> > {0, Copyright <ph name="YEAR">{0,date,y}</ph> The Chromium Authors. All
rights
> > reserved.}
> 
> Oops. I did too many plurals.  The msg should read :
> 
> Copyright <ph name="YEAR">{0,date,y}</ph> The Chromium Authors. All rights
> reserved.

Copyright <ph name="YEAR">{0,date,y}<ex>2016</ex></ph> The Chromium Authors. All
rights
reserved.

[Greg Levin, 2016-05-23 21:33:55]

glevin@chromium.org changed reviewers:
+ michaelpg@chromium.org, thestig@chromium.org

[Greg Levin, 2016-05-23 21:33:56]

michaelpg@ - chrome/browser/ui/webui/*
thestig@ - chrome/tools/mac_helpers/infoplist_strings_util.mm

Please have a look!

jshin@ -

> base/i18n does not have an access to IDS_* (string bundle). So,
> l10n_util.cc would be the best fit.
> PS#1 looks good to me, but if you wanna add a helper, go ahead.

Unfortunately, after I looked into it further, these IDS_s are chromium /
google_chrome dual strings, which don't appear to be accessible in
ui/base/l10n/.  I didn't see a solution that was worth the effort, so I'm not
doing a helper function after all :-(  However...

> There are a few different ways. One of them is to use
> MessageFormatter() and pass base::Time::Now() (no exploding/year
> extraction).

...this is pretty much what I wanted out of the utility function: avoiding extra
variables, and basically forcing identical use across all applications of
IDS_ABOUT_VERSION_COPYRIGHT.  Thanks!! :-)

[Greg Levin, 2016-05-23 21:34:52]

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

[commit-bot: I haz the power, 2016-05-23 21:35:34]

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

[Lei Zhang, 2016-05-23 21:36:42]

thestig@chromium.org changed reviewers:
+ shrike@chromium.org

[Lei Zhang, 2016-05-23 21:36:42]

I defer to shrike@ for chrome/tools/mac_helpers/infoplist_strings_util.mm, and
we should probably separately add chrome/tools/mac_helpers/OWNERS.

[commit-bot: I haz the power, 2016-05-23 21:42:29]

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

[commit-bot: I haz the power, 2016-05-23 21:42:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[shrike, 2016-05-23 23:30:39]

On 2016/05/23 21:36:42, Lei Zhang wrote:
> I defer to shrike@ for chrome/tools/mac_helpers/infoplist_strings_util.mm, and
> we should probably separately add chrome/tools/mac_helpers/OWNERS.

Hi thestig@,

I'm not sure what you asking of me for the first file, or what's you're
suggesting with the second file.

[Lei Zhang, 2016-05-23 23:34:53]

On 2016/05/23 23:30:39, shrike wrote:
> On 2016/05/23 21:36:42, Lei Zhang wrote:
> > I defer to shrike@ for chrome/tools/mac_helpers/infoplist_strings_util.mm,
and
> > we should probably separately add chrome/tools/mac_helpers/OWNERS.
> 
> Hi thestig@,
> 
> I'm not sure what you asking of me for the first file, or what's you're
> suggesting with the second file.

shrike:

1) Can you review the chrome/tools/mac_helpers/infoplist_strings_util.mm changes
or find someone who can?
2) There's no chrome/tools/mac_helpers/OWNERS file, so if one make changes in
that directory, they have to ask chrome/OWNERS for a review. From the
chrome/OWNERS perspective, there should be some OWNERS for
chrome/tools/mac_helpers.

[jungshik at Google, 2016-05-25 07:44:42]

On 2016/05/23 21:33:56, Greg Levin wrote:
> michaelpg@ - chrome/browser/ui/webui/*
> thestig@ - chrome/tools/mac_helpers/infoplist_strings_util.mm
> 
> Please have a look!
> 
> jshin@ -
> 
> > base/i18n does not have an access to IDS_* (string bundle). So,
> > l10n_util.cc would be the best fit.
> > PS#1 looks good to me, but if you wanna add a helper, go ahead.
> 
> Unfortunately, after I looked into it further, these IDS_s are chromium /
> google_chrome dual strings, which don't appear to be accessible in
> ui/base/l10n/.  I didn't see a solution that was worth the effort, so I'm not
> doing a helper function after all :-(  However...

Really?  Those 'dual' strings must be accessible from ui/base/l10n/..
> 
> > There are a few different ways. One of them is to use
> > MessageFormatter() and pass base::Time::Now() (no exploding/year
> > extraction).
> 
> ...this is pretty much what I wanted out of the utility function: avoiding
extra
> variables, and basically forcing identical use across all applications of
> IDS_ABOUT_VERSION_COPYRIGHT.  Thanks!! :-)

Anyway, this should work, too.  LGTM !

[jungshik at Google, 2016-05-25 09:09:36]

Mac build should be fixed.

https://codereview.chromium.org/1996993002/diff/60001/chrome/tools/mac_helper...
File chrome/tools/mac_helpers/infoplist_strings_util.mm (right):

https://codereview.chromium.org/1996993002/diff/60001/chrome/tools/mac_helper...
chrome/tools/mac_helpers/infoplist_strings_util.mm:262: copyright_format,
base::Time::Now()));
This should be |base::SysNSStringToUTF16(copyright_format)|.

[jungshik at Google, 2016-05-25 09:10:10]

On 2016/05/25 07:44:42, jshin (slow- 23-26) wrote:
> On 2016/05/23 21:33:56, Greg Levin wrote:
> > michaelpg@ - chrome/browser/ui/webui/*
> > thestig@ - chrome/tools/mac_helpers/infoplist_strings_util.mm
> > 
> > Please have a look!
> > 
> > jshin@ -
> > 
> > > base/i18n does not have an access to IDS_* (string bundle). So,
> > > l10n_util.cc would be the best fit.
> > > PS#1 looks good to me, but if you wanna add a helper, go ahead.
> > 
> > Unfortunately, after I looked into it further, these IDS_s are chromium /
> > google_chrome dual strings, which don't appear to be accessible in
> > ui/base/l10n/.  I didn't see a solution that was worth the effort, so I'm
not
> > doing a helper function after all :-(  However...
> 
> Really?  Those 'dual' strings must be accessible from ui/base/l10n/..

Oh. I misunderstood what you meant. You're absolutely right.

[Greg Levin, 2016-05-26 15:19:17]

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

[commit-bot: I haz the power, 2016-05-26 15:19:47]

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

[Greg Levin, 2016-05-26 15:25:58]

https://codereview.chromium.org/1996993002/diff/60001/chrome/tools/mac_helper...
File chrome/tools/mac_helpers/infoplist_strings_util.mm (right):

https://codereview.chromium.org/1996993002/diff/60001/chrome/tools/mac_helper...
chrome/tools/mac_helpers/infoplist_strings_util.mm:262: copyright_format,
base::Time::Now()));
On 2016/05/25 09:09:36, jshin (slow- 23-26) wrote:
> This should be |base::SysNSStringToUTF16(copyright_format)|.

Done.

[commit-bot: I haz the power, 2016-05-26 16:52:45]

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

[commit-bot: I haz the power, 2016-05-26 16:52:46]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-26 16:52:58]

Dry run: None

[shrike, 2016-05-26 17:48:20]

LGTM chrome/tools/mac_helpers/infoplist_strings_util.mm

[Lei Zhang, 2016-05-26 19:21:34]

On 2016/05/23 21:33:56, Greg Levin wrote:
> michaelpg@ - chrome/browser/ui/webui/*
> thestig@ - chrome/tools/mac_helpers/infoplist_strings_util.mm

Once michaelpg reviews his portion, I'll stamp the entire CL.

[michaelpg, 2016-05-27 16:45:19]

lgtm w/ a nit for the include (which was missing in the previous versions of the
files too)

https://codereview.chromium.org/1996993002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/1996993002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.cc:17: #include
"base/i18n/message_formatter.h"
here and the other impl files: nit: import time.h as well

[Greg Levin, 2016-05-27 17:41:27]

Okay, thestig@, your turn :-)

https://codereview.chromium.org/1996993002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/1996993002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.cc:17: #include
"base/i18n/message_formatter.h"
On 2016/05/27 16:45:19, michaelpg wrote:
> here and the other impl files: nit: import time.h as well

Done.

[Lei Zhang, 2016-05-27 17:42:27]

rs lgtm

[Greg Levin, 2016-05-27 17:43:25]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2016-05-27 17:43:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org,
shrike@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1996993002/#ps100001
(title: "Add time.h includes, as per review (also merge)")

[commit-bot: I haz the power, 2016-05-27 17:43:51]

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

[commit-bot: I haz the power, 2016-05-27 19:02:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-27 19:04:41]

Description was changed from

==========
i18n of Copyright line to use locally correct numeric glyphs

BUG=613583
TEST=Switch to language with non-Arabic numerals (e.g., Persian), open
Settings > About or chrome://settings to view Copyright line. Check that
year is rendered with non-Arabic numerals.
==========

to

==========
i18n of Copyright line to use locally correct numeric glyphs

BUG=613583
TEST=Switch to language with non-Arabic numerals (e.g., Persian), open
Settings > About or chrome://settings to view Copyright line. Check that
year is rendered with non-Arabic numerals.

Committed: https://crrev.com/8d231d3546984ff7d4b22a8a97bd162b599f7e9f
Cr-Commit-Position: refs/heads/master@{#396527}
==========

[commit-bot: I haz the power, 2016-05-27 19:04:42]

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