ICU msg format support with more than one arguments

Add ICU message format support

Adopt and customize a ICU message format wrapper used at Google to meet
Chromium's need.

This will enable formatting of 'complex messages' requiring plural and/or
selector (e.g. gender or 'single vs multiple') support with more than one
parameters.

Besides, l10n_util::GetPluralStringF* is rewritten to use this API.

I'm also planning to use this API to add a similar support to Chromium's
JavaScript-based UI and extensions.

References:
MessageFormat specs:
  http://icu-project.org/apiref/icu4j/com/ibm/icu/text/MessageFormat.html
  http://icu-project.org/apiref/icu4c/classicu_1_1DecimalFormat.html#details
Examples:
  http://userguide.icu-project.org/formatparse/messages
  message_formatter_unittest.cc
  go/plurals inside Google.

BUG=481734
TEST=base_unittests --gtest_filter="MessageFormat*"

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

[jungshik at Google, 2015-07-09 23:34:35]

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

[commit-bot: I haz the power, 2015-07-09 23:37:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/200001

[commit-bot: I haz the power, 2015-07-09 23:57:15]

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

[commit-bot: I haz the power, 2015-07-09 23:57:16]

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

(exceeded global retry quota)

[jungshik at Google, 2015-07-10 00:21:43]

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

[commit-bot: I haz the power, 2015-07-10 00:22:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/220001

[commit-bot: I haz the power, 2015-07-10 02:28:44]

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

[commit-bot: I haz the power, 2015-07-10 02:28:44]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2015-07-14 20:17:02]

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

[commit-bot: I haz the power, 2015-07-14 20:18:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/240001

[commit-bot: I haz the power, 2015-07-14 20:26:09]

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

[commit-bot: I haz the power, 2015-07-14 20:26:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

(exceeded global retry quota)

[jungshik at Google, 2015-07-14 23:52:41]

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

[commit-bot: I haz the power, 2015-07-14 23:53:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/260001

[commit-bot: I haz the power, 2015-07-15 01:03:14]

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

[commit-bot: I haz the power, 2015-07-15 01:03:15]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2015-07-17 21:13:51]

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

[commit-bot: I haz the power, 2015-07-17 21:14:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/300001

[commit-bot: I haz the power, 2015-07-17 21:41:47]

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

[commit-bot: I haz the power, 2015-07-17 21:41:48]

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

[jungshik at Google, 2015-07-18 00:25:24]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-07-18 00:25:25]

The CQ bit was unchecked by jshin@chromium.org

[jungshik at Google, 2015-07-18 00:25:28]

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

[commit-bot: I haz the power, 2015-07-18 00:25:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/320001

[commit-bot: I haz the power, 2015-07-18 00:40:13]

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

[commit-bot: I haz the power, 2015-07-18 00:40:14]

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

[jungshik at Google, 2015-07-22 19:16:58]

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

[commit-bot: I haz the power, 2015-07-22 19:17:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153005/360001

[commit-bot: I haz the power, 2015-07-22 19:22:34]

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

[commit-bot: I haz the power, 2015-07-22 19:22:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[jungshik at Google, 2015-07-28 17:07:08]

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

[commit-bot: I haz the power, 2015-07-28 17:08:12]

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

[commit-bot: I haz the power, 2015-07-28 18:35:23]

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

[commit-bot: I haz the power, 2015-07-28 18:35:24]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2015-07-30 18:04:35]

jshin@chromium.org changed reviewers:
+ avi@chromium.org, reillyg@chromium.org, rsleevi@chromium.org,
thakis@chromium.org

[jungshik at Google, 2015-07-30 18:04:37]

Can you take a look? Thanks 

avi: new functions in base/i18n, ui/base/l10n
thakis : build config changes for base and ui/base, chrome/app and
chrome/browser
reillyg : extension/browser/api 
rsleevi: ssl_info* change

[Reilly Grant (use Gerrit), 2015-07-30 18:10:09]

https://codereview.chromium.org/1140153005/diff/380001/ui/base/l10n/l10n_util.h
File ui/base/l10n/l10n_util.h (right):

https://codereview.chromium.org/1140153005/diff/380001/ui/base/l10n/l10n_util...
ui/base/l10n/l10n_util.h:176: UI_BASE_EXPORT base::string16
GetSingleOrMultipleStringFUTF16(int message_id,
I'm confused why a number of methods here are "StringF" when they don't support
format args.

[Avi (use Gerrit), 2015-07-30 19:19:49]

lgtm with nits.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter.cc (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:12: 
one blank line

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:21: }
no indentation for stuff in a namespace

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:48: if (f != NULL) {
if (f)

?

At least nullptr

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:38: MessageArg() : f(NULL) {}  // for "no
argument"
nullptr

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:40: icu::Formattable *f;
Previous two lines, * goes on the type, not the variable.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter_unittest.cc (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter_unittest.cc:62: 
no need for the blank line.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter_unittest.cc:71: "=13 {I met {person} and dozen other
people in {place}.}"
... and a dozen other people ...

is proper English.

https://codereview.chromium.org/1140153005/diff/380001/extensions/extensions_...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/1140153005/diff/380001/extensions/extensions_...
extensions/extensions_strings.grd:371: multiple {The application "<ph
name="APP_NAME">{1}<ex>Chrome Dev Editor</ex></ph>" is requesting access to one
or more of your devices.}
Don't we know that it's more than one? "One of more" still is ambiguous, so if
we know, can we say "... is requesting access to your devices"?

[Reilly Grant (use Gerrit), 2015-07-30 19:22:07]

https://codereview.chromium.org/1140153005/diff/380001/extensions/extensions_...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/1140153005/diff/380001/extensions/extensions_...
extensions/extensions_strings.grd:371: multiple {The application "<ph
name="APP_NAME">{1}<ex>Chrome Dev Editor</ex></ph>" is requesting access to one
or more of your devices.}
On 2015/07/30 19:19:49, Avi wrote:
> Don't we know that it's more than one? "One of more" still is ambiguous, so if
> we know, can we say "... is requesting access to your devices"?

No, this is the message for a selection dialog that allows multiple selection.
The user could select one, or they could select more than one. The purpose of
the difference in messaging is to inform the user of their choices.

[Ryan Sleevi, 2015-07-30 23:21:51]

ssl_info LGTM % a series of API concerns & nits.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter.cc (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:42: f(new
icu::Formattable(static_cast<UDate>(t.ToJsTime()))) {}
Wrong indent/wrap

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:44: MessageArg::~MessageArg() { delete f; }
Why not use a scoped_ptr<> for this?

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:51: } else {
Don't else after a return

https://www.chromium.org/developers/coding-style#Code_Formatting

Also, prefer error conditions first

if (!f)
  return false;

++*count;
return true;

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:78: UnicodeString msgString(msg.data(),
msg.size());
msg_string

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:80: icu::MessageFormat fmt(msgString,  error);
format

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:121: UnicodeString msgString(msg.data(),
msg.size());
msg_string

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:123: icu::MessageFormat fmt(msgString, error);
format

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:39: bool def(int *count) const;
Seems like this should be documented, and perhaps renamed?

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:71: const internal::MessageArg& arg0 =
internal::MessageArg(),
I'm somewhat surprised this compiles. I thought the friend decl would allow
MessageFormatter to create internal::MessageArg(), but the default arguments are
expanded at the call site in the context of the caller, and they
wouldn't/shouldn't be allowed to call this method, right?

https://codereview.chromium.org/1140153005/diff/380001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1140153005/diff/380001/chrome/app/generated_r...
chrome/app/generated_resources.grd:2707: +         other {This server could not
prove that it is <ph
name="DOMAIN">&lt;strong&gt;{0}<ex>paypal.com</ex>&lt;/strong&gt;</ph>; its
security certificate expired # days ago. This may be caused by a
misconfiguration or an attacker intercepting your connection. Your computer's
clock is currently set to <ph name="CURRENT_TIME">{2, date, full}<ex>July 18,
2012</ex></ph>. Does that look right? If not, you should correct your system's
clock and then refresh this page.}}
This isn't correct now, is it?

The old code was TimeFormatFriendlyDate, which expands to something like
"Monday, March 6, 2008"

DateFormat.FULL (which, if I understand the MessageFormat syntax, is what this
expands to) seems like it's "Monday, March 6, 2008 AD"

It seems like LONG is more appropriate, and in line with how the description was
written (March 6, 2008)

[jungshik at Google, 2015-07-31 21:36:16]

Thanks a lot for reviewing and comments. I believe I addressed them all. Can you
take another look? Thanks again !

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter.cc (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:12: 
On 2015/07/30 19:19:48, Avi wrote:
> one blank line

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:21: }
On 2015/07/30 19:19:48, Avi wrote:
> no indentation for stuff in a namespace

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:42: f(new
icu::Formattable(static_cast<UDate>(t.ToJsTime()))) {}
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> Wrong indent/wrap

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:44: MessageArg::~MessageArg() { delete f; }
On 2015/07/30 23:21:50, Ryan Sleevi wrote:
> Why not use a scoped_ptr<> for this?

Done. Clang-chrome-style checker complained about inline ctor and default dtor
for 'a complex class' (due to a member being scoped_ptr) and I had to move them
to cc.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:48: if (f != NULL) {
On 2015/07/30 19:19:48, Avi wrote:
> if (f)
> 
> ?
> 
> At least nullptr

Done and renamed |f| to |formattable|

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:51: } else {
On 2015/07/30 23:21:50, Ryan Sleevi wrote:
> Don't else after a return
> 
> https://www.chromium.org/developers/coding-style#Code_Formatting
> 
> Also, prefer error conditions first
> 
> if (!f)
>   return false;
> 
> ++*count;
> return true;

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:78: UnicodeString msgString(msg.data(),
msg.size());
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> msg_string

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:80: icu::MessageFormat fmt(msgString,  error);
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> format

Done. Also in FormatWithNamedArgs

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:121: UnicodeString msgString(msg.data(),
msg.size());
On 2015/07/30 23:21:50, Ryan Sleevi wrote:
> msg_string

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.cc:123: icu::MessageFormat fmt(msgString, error);
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> format

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:38: MessageArg() : f(NULL) {}  // for "no
argument"
On 2015/07/30 19:19:48, Avi wrote:
> nullptr

Done. Had to move to cc when I also switched to using scoped_ptr because clang
chrome style checker does not want a 'complex ctor' inlined. 

It's also renamed 'formattable'.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:39: bool def(int *count) const;
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> Seems like this should be documented, and perhaps renamed?

Changed it to has_value().

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:40: icu::Formattable *f;
On 2015/07/30 19:19:48, Avi wrote:
> Previous two lines, * goes on the type, not the variable.

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter.h:71: const internal::MessageArg& arg0 =
internal::MessageArg(),
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> I'm somewhat surprised this compiles. I thought the friend decl would allow
> MessageFormatter to create internal::MessageArg(), but the default arguments
are
> expanded at the call site in the context of the caller, and they
> wouldn't/shouldn't be allowed to call this method, right?

I guess it's compiled because I have 'friend class MessageFormatter' instead of
'friend <stand-alone function>'. I tried to use standalone functions instead,
but never succeeded always getting complaints about a private member function
not accessible.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
File base/i18n/message_formatter_unittest.cc (right):

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter_unittest.cc:62: 
On 2015/07/30 19:19:49, Avi wrote:
> no need for the blank line.

Done.

https://codereview.chromium.org/1140153005/diff/380001/base/i18n/message_form...
base/i18n/message_formatter_unittest.cc:71: "=13 {I met {person} and dozen other
people in {place}.}"
On 2015/07/30 19:19:48, Avi wrote:
> ... and a dozen other people ...
> 
> is proper English.

Done.

https://codereview.chromium.org/1140153005/diff/380001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1140153005/diff/380001/chrome/app/generated_r...
chrome/app/generated_resources.grd:2707: +         other {This server could not
prove that it is <ph
name="DOMAIN">&lt;strong&gt;{0}<ex>paypal.com</ex>&lt;/strong&gt;</ph>; its
security certificate expired # days ago. This may be caused by a
misconfiguration or an attacker intercepting your connection. Your computer's
clock is currently set to <ph name="CURRENT_TIME">{2, date, full}<ex>July 18,
2012</ex></ph>. Does that look right? If not, you should correct your system's
clock and then refresh this page.}}
On 2015/07/30 23:21:51, Ryan Sleevi wrote:
> This isn't correct now, is it?
> 
> The old code was TimeFormatFriendlyDate, which expands to something like
> "Monday, March 6, 2008"
> 
> DateFormat.FULL (which, if I understand the MessageFormat syntax, is what this
> expands to) seems like it's "Monday, March 6, 2008 AD"
> 
> It seems like LONG is more appropriate, and in line with how the description
was
> written (March 6, 2008)

ICU API doc has an error. Full does not have era at the end (at least for
Gregorian calendar). Go to
http://www.unicode.org/cldr/charts/27/by_type/date_&_time.gregorian.html 
and use 'Find-in-page' to get to 'full' format in all the locales.  I'll ask
them to fix. :-)
  
It'd be just 'Monday, March 6, 2008'. And, that's what TimeFormatFriendlyDate
does (it uses kFull when calling ICU's message formatter and its unittest
expects that, too).

https://codereview.chromium.org/1140153005/diff/380001/extensions/extensions_...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/1140153005/diff/380001/extensions/extensions_...
extensions/extensions_strings.grd:371: multiple {The application "<ph
name="APP_NAME">{1}<ex>Chrome Dev Editor</ex></ph>" is requesting access to one
or more of your devices.}
On 2015/07/30 19:22:07, reillyg wrote:
> On 2015/07/30 19:19:49, Avi wrote:
> > Don't we know that it's more than one? "One of more" still is ambiguous, so
if
> > we know, can we say "... is requesting access to your devices"?
> 
> No, this is the message for a selection dialog that allows multiple selection.
> The user could select one, or they could select more than one. The purpose of
> the difference in messaging is to inform the user of their choices.

Yeah.... There are cases where we need to have 'two-way' branches and
translators/linguists so far have come up with something reasonable.

https://codereview.chromium.org/1140153005/diff/380001/ui/base/l10n/l10n_util.h
File ui/base/l10n/l10n_util.h (right):

https://codereview.chromium.org/1140153005/diff/380001/ui/base/l10n/l10n_util...
ui/base/l10n/l10n_util.h:176: UI_BASE_EXPORT base::string16
GetSingleOrMultipleStringFUTF16(int message_id,
On 2015/07/30 18:10:09, reillyg wrote:
> I'm confused why a number of methods here are "StringF" when they don't
support
> format args.

I'm not sure what you meants by "support format args". Anyway, in this
particular case, from a caller's pov, it's just selecting single or multiple
case string so that I'm dropping 'F' (even though from an internal
impelementation pov, it's still formatting). I also changed the comment above
the function decl.

[Reilly Grant (use Gerrit), 2015-07-31 22:00:16]

//extensions lgtm

[Ryan Sleevi, 2015-07-31 22:19:36]

A few more comments about the build system - you may want to have brettw double
check since Nico is hiding.

ssl_info LGTM

https://codereview.chromium.org/1140153005/diff/400001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1140153005/diff/400001/base/BUILD.gn#newcode993
base/BUILD.gn:993: "//third_party/icu",
I believe this moves to public_deps, brett can confirm.

https://codereview.chromium.org/1140153005/diff/400001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1140153005/diff/400001/base/base.gyp#newcode290
base/base.gyp:290: 'base',
This is where you should export the ICU headers - that is, add the two targets
to export_dependent_settings

(It looks like both bidi_line_iterator and string_compare pull in ICU headers,
so they should have added that exporting then.)

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter.cc (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.cc:19: icu::StringPiece(str.data(),
static_cast<int32_t>(str.size())));
If you can depend on //base (and it seems you can),
base/numerics/safe_conversions offers base::checked_cast<int32_t>(str.size())

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:4: // Copied from i18n/localization/formatter.h
with modifications.
Seems like there should at least be a newline between this and the copyright
declaration.

Is this path referencing an internal Google path? Do you need to confirm with
OSPO the acceptability of open-sourcing this? If so, and once done, it seems
like this comment should be deleted - there's plenty in //base that has history
in Google internal code but since stands alone.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:33: MessageArg(const Time& t);
You could forward declare base::Time and avoid the include.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:39: MessageArg();
style nits: newline between 38 and 39, newline between 40 and 41, document 40?

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:41: scoped_ptr<icu::Formattable> formattable;
You could forward declare this, but I'm guessing that's not idiomatic ICU usage?

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter_unittest.cc (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter_unittest.cc:29: }
pedantic nit: Rather than using SetUp/TearDown, it seems you could use the
ctor/dtor

See
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...
for the reasoning why this piece of advice

https://codereview.chromium.org/1140153005/diff/400001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/1140153005/diff/400001/extensions/browser/BUI...
extensions/browser/BUILD.gn:32: "//third_party/icu",
Should this code be explicitly depending on ICU? Shouldn't it be picking it up
from the i18n bits?

Is this because you include an ICU header in the base/i18n stuff? If so, then
the GN should be exporting this dependency on //base, rather than requiring you
to add it here.

[jungshik at Google, 2015-08-01 14:00:57]

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

[jungshik at Google, 2015-08-01 14:00:58]

Ryan, thanks a lot for review/comments. I took care of them. 
Can you go over one more time? 

Brett, can you take a look at build configuration changes (see Ryan's comments)
?

Thanks

https://codereview.chromium.org/1140153005/diff/400001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1140153005/diff/400001/base/BUILD.gn#newcode993
base/BUILD.gn:993: "//third_party/icu",
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> I believe this moves to public_deps, brett can confirm.

Thanks ! I didn't know about public_deps.  I was wondering why dependency was
not propagated and ended up explicitly listing icu in extensions/browser.

https://codereview.chromium.org/1140153005/diff/400001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1140153005/diff/400001/base/base.gyp#newcode290
base/base.gyp:290: 'base',
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> This is where you should export the ICU headers - that is, add the two targets
> to export_dependent_settings
> 
> (It looks like both bidi_line_iterator and string_compare pull in ICU headers,
> so they should have added that exporting then.)

Thanks again. Done.
Along with this, I tried dropping dependency on icu* from base_unittests, but it
didn't work. So, I left that alone.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter.cc (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.cc:19: icu::StringPiece(str.data(),
static_cast<int32_t>(str.size())));
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> If you can depend on //base (and it seems you can),
> base/numerics/safe_conversions offers base::checked_cast<int32_t>(str.size())

Thanks. Done

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:4: // Copied from i18n/localization/formatter.h
with modifications.
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> Seems like there should at least be a newline between this and the copyright
> declaration.
> 
> Is this path referencing an internal Google path? Do you need to confirm with
> OSPO the acceptability of open-sourcing this? If so, and once done, it seems
> like this comment should be deleted - there's plenty in //base that has
history
> in Google internal code but since stands alone.

Before making the CL, I talked to OSPO and the two authors of this class/api. I
also asked Brett what he did with string_piece. The authors greenlighted using
this. 

The line referring to the internal src file was patterned after the lines in
string_piece.{h,cc}, but I deleted per your suggestion.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:33: MessageArg(const Time& t);
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> You could forward declare base::Time and avoid the include.

Done.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:39: MessageArg();
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> style nits: newline between 38 and 39, newline between 40 and 41, document 40?

Done.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:41: scoped_ptr<icu::Formattable> formattable;
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> You could forward declare this, but I'm guessing that's not idiomatic ICU
usage?

Thanks for the suggestion. 
Nothing in ICU against that. The only thing is that I can't do

namespace icu {
class Formattable;
}

Instead, I need to do the following (which I did in an updated CL). I may look
for other headers in the cr tree (and perhaps ICU itself)  to see if others can
benefit from that (not in this CL). 

#Include "third_party/icu/source/common/unicode/uversion.h"  // to get renamed
icu namespace

U_NAMESPACE_BEGIN
class Formattable;
U_NAMESPACE_END

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter_unittest.cc (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter_unittest.cc:29: }
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> pedantic nit: Rather than using SetUp/TearDown, it seems you could use the
> ctor/dtor
> 
> See
>
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...
> for the reasoning why this piece of advice

Thanks for the suggestion. DOne

https://codereview.chromium.org/1140153005/diff/400001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/1140153005/diff/400001/extensions/browser/BUI...
extensions/browser/BUILD.gn:32: "//third_party/icu",
On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> Should this code be explicitly depending on ICU? Shouldn't it be picking it up
> from the i18n bits?
> 
> Is this because you include an ICU header in the base/i18n stuff? If so, then
> the GN should be exporting this dependency on //base, rather than requiring
you
> to add it here.

That I wanted to avoid as well, but specifying //base or //base:base_i18n didn't
work. 
Adding icu to public_deps in base/BUILD.gn for base_i18n (per your suggestion),
I was able to replace it with //base::base_i18n.  Thanks.

https://codereview.chromium.org/1140153005/diff/400001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/1140153005/diff/400001/extensions/extensions....
extensions/extensions.gyp:128: '../third_party/icu/icu.gyp:icui18n',
Removing two icu lines above still works, but I'm not sure if it's 'canonical'
to do.

[jungshik at Google, 2015-08-01 14:16:54]

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

[commit-bot: I haz the power, 2015-08-01 14:17:00]

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

[commit-bot: I haz the power, 2015-08-01 16:33:00]

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

[commit-bot: I haz the power, 2015-08-01 16:33:02]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2015-08-03 08:01:41]

LGTM % new nits :)

https://codereview.chromium.org/1140153005/diff/400001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1140153005/diff/400001/base/base.gyp#newcode290
base/base.gyp:290: 'base',
On 2015/08/01 14:00:58, jungshik at google wrote:
> On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> > This is where you should export the ICU headers - that is, add the two
targets
> > to export_dependent_settings
> > 
> > (It looks like both bidi_line_iterator and string_compare pull in ICU
headers,
> > so they should have added that exporting then.)
> 
> Thanks again. Done.
> Along with this, I tried dropping dependency on icu* from base_unittests, but
it
> didn't work. So, I left that alone. 

Right, because your unittest uses the ICU bits directly,  you want to make sure
base_unittests explicitly depends on ICU.

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
base/i18n/message_formatter.h:41: scoped_ptr<icu::Formattable> formattable;
On 2015/08/01 14:00:58, jungshik at google wrote:
> On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> > You could forward declare this, but I'm guessing that's not idiomatic ICU
> usage?
> 
> Thanks for the suggestion. 
> Nothing in ICU against that. The only thing is that I can't do
> 
> namespace icu {
> class Formattable;
> }
> 
> Instead, I need to do the following (which I did in an updated CL). I may look
> for other headers in the cr tree (and perhaps ICU itself)  to see if others
can
> benefit from that (not in this CL). 
> 
> #Include "third_party/icu/source/common/unicode/uversion.h"  // to get renamed
> icu namespace
> 
> U_NAMESPACE_BEGIN
> class Formattable;
> U_NAMESPACE_END

I guess the forward declaration doesn't really buy you much, since you have to
include uversion.h to get the U_NAMESPACE_BEGIN

Mostly, I mentioned it to see if there was a way to avoid including ICU headers
in these headers, which would let you avoid the build system issues. Since it
doesn't seem there is, it's not terribly beneficial for forward declarations, so
I wouldn't worry about it too much for existing code, and it may just make sense
to include formatter here. whatever you think is appropriate.

https://codereview.chromium.org/1140153005/diff/400001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/1140153005/diff/400001/extensions/extensions....
extensions/extensions.gyp:128: '../third_party/icu/icu.gyp:icui18n',
On 2015/08/01 14:00:58, jungshik at google wrote:
> Removing two icu lines above still works, but I'm not sure if it's 'canonical'
> to do. 

Yeah, since none of the unittests explicitly use ICU, only the base::i18n parts,
it's entirely appropriate and matches your .gn changes (GYP's version of
public_deps is export_dependent_settings )

https://codereview.chromium.org/1140153005/diff/460001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/460001/base/i18n/message_form...
base/i18n/message_formatter.h:22: class Time;
nit: should have a newline above and below this (and also line 20/22 in the
previous patchset - newlines around MessageFormatter - were also correct)

https://codereview.chromium.org/1140153005/diff/460001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/1140153005/diff/460001/extensions/browser/BUI...
extensions/browser/BUILD.gn:32: "//base:i18n",
sort order

[jungshik at Google, 2015-08-03 20:18:58]

Thanks again. Two nits are fixed in the updated PS. 

On 2015/08/03 08:01:41, Ryan Sleevi wrote:
> LGTM % new nits :)

>
https://codereview.chromium.org/1140153005/diff/400001/base/i18n/message_form...
> base/i18n/message_formatter.h:41: scoped_ptr<icu::Formattable> formattable;
> On 2015/08/01 14:00:58, jungshik at google wrote:
> > On 2015/07/31 22:19:36, Ryan Sleevi wrote:
> > > You could forward declare this, but I'm guessing that's not idiomatic ICU
> > usage?
> > 
> > Thanks for the suggestion. 
> > Nothing in ICU against that. The only thing is that I can't do
> > 
> > namespace icu {
> > class Formattable;
> > }
> > 
> > Instead, I need to do the following (which I did in an updated CL). I may
look
> > for other headers in the cr tree (and perhaps ICU itself)  to see if others
> can
> > benefit from that (not in this CL). 
> > 
> > #Include "third_party/icu/source/common/unicode/uversion.h"  // to get
renamed
> > icu namespace
> > 
> > U_NAMESPACE_BEGIN
> > class Formattable;
> > U_NAMESPACE_END
> 
> I guess the forward declaration doesn't really buy you much, since you have to
> include uversion.h to get the U_NAMESPACE_BEGIN
> 
> Mostly, I mentioned it to see if there was a way to avoid including ICU
headers
> in these headers, which would let you avoid the build system issues. Since it
> doesn't seem there is, it's not terribly beneficial for forward declarations,
so
> I wouldn't worry about it too much for existing code, and it may just make
sense
> to include formatter here. whatever you think is appropriate.

Aha... I thought of it as a way to cut down the build time. The saving may be
too small to be worth a clutter. 
Well, I'm on the fence and just left it alone in the updated CL.  Per your
comment, I'll not actively look for other cases, though.

[jungshik at Google, 2015-08-03 20:32:19]

Thanks, Ryan. 

Brett, do you have any comment on build (GN) change (see Ryan's comment 43 -
2015-07-31 22:19:36 UTC ). His suggested changes worked well. Just in case, I'm
double-checking with you. Thanks

https://codereview.chromium.org/1140153005/diff/460001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/460001/base/i18n/message_form...
base/i18n/message_formatter.h:22: class Time;
On 2015/08/03 08:01:41, Ryan Sleevi wrote:
> nit: should have a newline above and below this (and also line 20/22 in the
> previous patchset - newlines around MessageFormatter - were also correct)

Done.

https://codereview.chromium.org/1140153005/diff/460001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/1140153005/diff/460001/extensions/browser/BUI...
extensions/browser/BUILD.gn:32: "//base:i18n",
On 2015/08/03 08:01:41, Ryan Sleevi wrote:
> sort order

Done.

[brettw, 2015-08-04 17:04:19]

build changes seem correct

https://codereview.chromium.org/1140153005/diff/470001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/470001/base/i18n/message_form...
base/i18n/message_formatter.h:109: #endif
Blank line before this, need header guard comment at the end of the line

[jungshik at Google, 2015-08-04 17:13:55]

On 2015/08/04 17:04:19, brettw wrote:
> build changes seem correct

Thanks. 

> 
>
https://codereview.chromium.org/1140153005/diff/470001/base/i18n/message_form...
> File base/i18n/message_formatter.h (right):
> 
>
https://codereview.chromium.org/1140153005/diff/470001/base/i18n/message_form...
> base/i18n/message_formatter.h:109: #endif
> Blank line before this, need header guard comment at the end of the line

Fixed in the last PS (#26)

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

Brett, can you approve base/* (including build conf) changes? Thanks

On 2015/08/04 17:13:55, jungshik at google wrote:
> On 2015/08/04 17:04:19, brettw wrote:
> > build changes seem correct
> 
> Thanks. 
> 
> > 
> >
>
https://codereview.chromium.org/1140153005/diff/470001/base/i18n/message_form...
> > File base/i18n/message_formatter.h (right):
> > 
> >
>
https://codereview.chromium.org/1140153005/diff/470001/base/i18n/message_form...
> > base/i18n/message_formatter.h:109: #endif
> > Blank line before this, need header guard comment at the end of the line
> 
> Fixed in the last PS (#26)

[jungshik at Google, 2015-08-05 19:15:45]

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

[jungshik at Google, 2015-08-05 19:15:46]

Turned out that Brett is not a base/ owner.
Mark, can you take a look and approve? Thanks !

[Mark Mentovai, 2015-08-06 18:56:42]

LGTM in base.

https://codereview.chromium.org/1140153005/diff/490001/base/i18n/message_form...
File base/i18n/message_formatter.h (right):

https://codereview.chromium.org/1140153005/diff/490001/base/i18n/message_form...
base/i18n/message_formatter.h:14: // For versioned icu namespace
Kinda obvious, no?

https://codereview.chromium.org/1140153005/diff/490001/base/i18n/message_form...
base/i18n/message_formatter.h:37: MessageArg(int i);
icu::Formattable can also be created from an int64_t, is it worth providing an
interface to that too?

[jungshik at Google, 2015-08-06 21:49:56]

Thanks a lot for the review, Mark. The latest PS (being uploaded) addressed your
comments. 

On 2015/08/06 18:56:42, Mark Mentovai - out til August wrote:
> LGTM in base.
> 
>
https://codereview.chromium.org/1140153005/diff/490001/base/i18n/message_form...
> File base/i18n/message_formatter.h (right):
> 
>
https://codereview.chromium.org/1140153005/diff/490001/base/i18n/message_form...
> base/i18n/message_formatter.h:14: // For versioned icu namespace
> Kinda obvious, no?

Ok. Will remove it because it kinda gets an unnecessary attention to have a
comment between header include lines. 
 
>
https://codereview.chromium.org/1140153005/diff/490001/base/i18n/message_form...
> base/i18n/message_formatter.h:37: MessageArg(int i);
> icu::Formattable can also be created from an int64_t, is it worth providing an
> interface to that too?

I debated myself on that. Let's add it.

[jungshik at Google, 2015-08-06 21:50:11]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-08-06 21:50:11]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
reillyg@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1140153005/#ps490001
(title: "hdr guard style fix")

[jungshik at Google, 2015-08-06 21:50:18]

The CQ bit was unchecked by jshin@chromium.org

[jungshik at Google, 2015-08-06 21:52:38]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-08-06 21:52:39]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
reillyg@chromium.org, rsleevi@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1140153005/#ps510001
(title: "Mark's review comments")

[commit-bot: I haz the power, 2015-08-06 21:53:53]

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

[commit-bot: I haz the power, 2015-08-06 22:57:06]

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

[commit-bot: I haz the power, 2015-08-06 22:57:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jungshik at Google, 2015-08-06 23:37:45]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2015-08-06 23:38:14]

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

[commit-bot: I haz the power, 2015-08-07 02:43:54]

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

[commit-bot: I haz the power, 2015-08-07 02:43:56]

Try jobs failed on following builders:
  mac_chromium_10.10_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.1...)

[jungshik at Google, 2015-08-07 10:06:15]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2015-08-07 10:06:32]

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

[commit-bot: I haz the power, 2015-08-07 10:11:18]

Committed patchset #27 (id:510001)

[commit-bot: I haz the power, 2015-08-07 10:11:57]

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