[i18n] components directory to $i18n{}

[i18n] components directory to $i18n{}

This CL changes i18n-content and i18n-value (the old way) to
$i18n{} (the new way). There should be no functionality or UI
changes apparent to the user.

TBR=stanisc@chromium.org,dbeam@chromium.org,fgorski@chromium.org,thestig@chromium.org,hirono@chromium.org,mattreynolds@chromium.org,juliatuttle@chromium.org,bengr@chromium.org,treib@chromium.org
BUG=677338
Review-Url: https://codereview.chromium.org/2908353003
Cr-Commit-Position: refs/heads/master@{#479896}
Committed: https://chromium.googlesource.com/chromium/src/+/9ab6f1801d197cda79b2f1dbf8f81d4f8d5f4884

[dschuyler, 2017-05-30 22:30:51]

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

[commit-bot: I haz the power, 2017-05-30 22:31:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-30 22:38:03]

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

[commit-bot: I haz the power, 2017-05-30 22:38:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-30 22:41:12]

Patchset #1 (id:1) has been deleted

[dschuyler, 2017-05-30 22:41:24]

dschuyler@chromium.org changed reviewers:
+ tommycli@chromium.org

[dschuyler, 2017-05-30 22:41:24]

https://codereview.chromium.org/2908353003/diff/20001/components/flags_ui/res...
File components/flags_ui/resources/apple_flags.html (right):

https://codereview.chromium.org/2908353003/diff/20001/components/flags_ui/res...
components/flags_ui/resources/apple_flags.html:95:
<div>$i18n{flagsRestartNotice}</div>
The other changes in this CL are from a script (i.e. machine generated changes).
This file is an exception because I changed some indention and wrapping by hand.
Such as here where the indentation was odd in the source file.

[tommycli, 2017-05-30 22:48:55]

lgtm

https://codereview.chromium.org/2908353003/diff/20001/components/flags_ui/res...
File components/flags_ui/resources/apple_flags.html (right):

https://codereview.chromium.org/2908353003/diff/20001/components/flags_ui/res...
components/flags_ui/resources/apple_flags.html:95:
<div>$i18n{flagsRestartNotice}</div>
On 2017/05/30 22:41:24, dschuyler wrote:
> The other changes in this CL are from a script (i.e. machine generated
changes).
> This file is an exception because I changed some indention and wrapping by
hand.
> Such as here where the indentation was odd in the source file.

Acknowledged.

[commit-bot: I haz the power, 2017-05-30 23:32:08]

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

[commit-bot: I haz the power, 2017-05-30 23:32:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dschuyler, 2017-05-31 21:00:01]

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

[commit-bot: I haz the power, 2017-05-31 21:01:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-31 21:10:53]

PTAL

https://codereview.chromium.org/2908353003/diff/40001/components/supervised_u...
File
components/supervised_user_error_page/supervised_user_error_page_unittest.cc
(left):

https://codereview.chromium.org/2908353003/diff/40001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:86:
EXPECT_THAT(result, testing::HasSubstr(html));
This was previously testing that BuildHtml and GetRawDataResource would load the
same IDR_SUPERVISED_USER_BLOCK_INTERSTITIAL_HTML text. That assumption doesn't
hold in the the $i18n{} world, because the i18n is inlined immediately (that's
the power of $i18n{}, i.e. why it exists). I considered what it would mean to
keep this comparison, but it would end up being a second call to BuildHtml()
(i.e. testing that "BuildHtml(...) == BuildHtml(...)" which isn't a valuable
comparison.

[commit-bot: I haz the power, 2017-05-31 21:39:31]

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

[commit-bot: I haz the power, 2017-05-31 21:39:33]

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

[dschuyler, 2017-06-10 01:34:16]

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

[commit-bot: I haz the power, 2017-06-10 01:35:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-06-10 01:37:33]

PTAL.

Yay for buildbot tests. They found that 	ui/base/webui/jstemplate_builder.cc
needed to be updated to fill in the $i18n data.

[commit-bot: I haz the power, 2017-06-10 03:50:58]

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

[commit-bot: I haz the power, 2017-06-10 03:50:59]

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

[tommycli, 2017-06-12 18:49:20]

https://codereview.chromium.org/2908353003/diff/40001/components/supervised_u...
File
components/supervised_user_error_page/supervised_user_error_page_unittest.cc
(left):

https://codereview.chromium.org/2908353003/diff/40001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:86:
EXPECT_THAT(result, testing::HasSubstr(html));
On 2017/05/31 21:10:53, dschuyler wrote:
> This was previously testing that BuildHtml and GetRawDataResource would load
the
> same IDR_SUPERVISED_USER_BLOCK_INTERSTITIAL_HTML text. That assumption doesn't
> hold in the the $i18n{} world, because the i18n is inlined immediately (that's
> the power of $i18n{}, i.e. why it exists). I considered what it would mean to
> keep this comparison, but it would end up being a second call to BuildHtml()
> (i.e. testing that "BuildHtml(...) == BuildHtml(...)" which isn't a valuable
> comparison.

Agreed, let's just axe that above EXPECT clause. ^

Thanks for the thorough explanation.

[tommycli, 2017-06-12 18:52:05]

https://codereview.chromium.org/2908353003/diff/40001/components/supervised_u...
File
components/supervised_user_error_page/supervised_user_error_page_unittest.cc
(left):

https://codereview.chromium.org/2908353003/diff/40001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:86:
EXPECT_THAT(result, testing::HasSubstr(html));
On 2017/06/12 18:49:20, tommycli wrote:
> On 2017/05/31 21:10:53, dschuyler wrote:
> > This was previously testing that BuildHtml and GetRawDataResource would load
> the
> > same IDR_SUPERVISED_USER_BLOCK_INTERSTITIAL_HTML text. That assumption
doesn't
> > hold in the the $i18n{} world, because the i18n is inlined immediately
(that's
> > the power of $i18n{}, i.e. why it exists). I considered what it would mean
to
> > keep this comparison, but it would end up being a second call to BuildHtml()
> > (i.e. testing that "BuildHtml(...) == BuildHtml(...)" which isn't a valuable
> > comparison.
> 
> Agreed, let's just axe that above EXPECT clause. ^
> 
> Thanks for the thorough explanation.

I looked into it some more out of curiosity. Previous test was pretty much
tautological anyways. No loss in removal IMO.

[dschuyler, 2017-06-13 00:13:52]

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

[commit-bot: I haz the power, 2017-06-13 00:14:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 01:31:02]

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

[commit-bot: I haz the power, 2017-06-13 01:31:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dschuyler, 2017-06-13 23:54:58]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-06-13 23:54:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2908353003/#ps80001
(title: "handle blank cohort name")

[commit-bot: I haz the power, 2017-06-13 23:55:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-06-13 23:56:59]

On 2017/06/13 23:55:43, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

PS4 still lgtm

[commit-bot: I haz the power, 2017-06-14 00:05:20]

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

[commit-bot: I haz the power, 2017-06-14 00:05:21]

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

[dschuyler, 2017-06-14 00:57:55]

Description was changed from

==========
[i18n] components directory to $i18n{}

This CL changes i18n-content and i18n-value (the old way) to
$i18n{} (the new way). There should be no functionality or UI
changes apparent to the user.

BUG=677338
==========

to

==========
[i18n] components directory to $i18n{}

This CL changes i18n-content and i18n-value (the old way) to
$i18n{} (the new way). There should be no functionality or UI
changes apparent to the user.

TBR=stanisc@chromium.org,dbeam@chromium.org,fgorski@chromium.org,thestig@chro...
BUG=677338
==========

[dschuyler, 2017-06-14 00:58:19]

Hi, The TBR list is for OWNER(s). Here are the files for each owner:

stanisc:
  components/sync/driver/resources/index.html
dbeam:
  chrome/browser/ui/webui/version_ui.cc
  components/flags_ui/resources/apple_flags.html
  components/flags_ui/resources/flags.html
  components/version_ui/resources/about_version.html
fgorski:
  components/gcm_driver/resources/gcm_internals.html
thestig:
  chrome/browser/ui/webui/version_ui.cc
  components/crash/core/browser/resources/crashes.html
hirono:
  components/chrome_apps/webstore_widget/app/main.html
mattreynolds:
  components/physical_web/webui/resources/physical_web.html
juliatuttle:
  components/neterror/resources/neterror.html
bengr:
  components/dom_distiller/webui/resources/about_dom_distiller.html
treib:
 
components/supervised_user_error_page/resources/supervised_user_block_interstitial.html
  components/supervised_user_error_page/supervised_user_error_page_unittest.cc
msarda:
  components/signin/core/browser/resources/signin_index.html

[dschuyler, 2017-06-14 00:59:54]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-06-14 01:00:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 01:11:24]

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

[commit-bot: I haz the power, 2017-06-14 01:11:27]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[dschuyler, 2017-06-15 18:39:04]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-06-15 18:39:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 19:40:54]

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

[commit-bot: I haz the power, 2017-06-15 19:40:55]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dschuyler, 2017-06-16 00:51:54]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-06-16 00:52:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 01:48:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497574314406490,
"parent_rev": "be338602c635fd6472f07ee1665dfccb7a83833a", "commit_rev":
"6f7277fab4ecbda684b4e84468cae467bd6a67dc"}

[commit-bot: I haz the power, 2017-06-16 01:48:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497574314406490,
"parent_rev": "34394dc6fb5ce9a6c351589deff9236fc0564d65", "commit_rev":
"61956e98cc0ca5f7277cbdc402d871ba80b4b7eb"}

[commit-bot: I haz the power, 2017-06-16 01:48:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497574314406490,
"parent_rev": "193916d38226076b7231d0595708d11e1f3bd88a", "commit_rev":
"9ab6f1801d197cda79b2f1dbf8f81d4f8d5f4884"}

[commit-bot: I haz the power, 2017-06-16 01:49:12]

Description was changed from

==========
[i18n] components directory to $i18n{}

This CL changes i18n-content and i18n-value (the old way) to
$i18n{} (the new way). There should be no functionality or UI
changes apparent to the user.

TBR=stanisc@chromium.org,dbeam@chromium.org,fgorski@chromium.org,thestig@chro...
BUG=677338
==========

to

==========
[i18n] components directory to $i18n{}

This CL changes i18n-content and i18n-value (the old way) to
$i18n{} (the new way). There should be no functionality or UI
changes apparent to the user.

TBR=stanisc@chromium.org,dbeam@chromium.org,fgorski@chromium.org,thestig@chro...
BUG=677338

Review-Url: https://codereview.chromium.org/2908353003
Cr-Commit-Position: refs/heads/master@{#479896}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ab6f1801d197cda79b2f1dbf8f8...
==========

[commit-bot: I haz the power, 2017-06-16 01:49:13]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9ab6f1801d197cda79b2f1dbf8f8...

[Lei Zhang, 2017-06-17 06:06:25]

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

[Lei Zhang, 2017-06-17 06:06:26]

Didn't we just talk about actually adding the TBR'd reviewers in the review tool
last week?
https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#...

[Lei Zhang, 2017-06-17 06:23:15]

https://codereview.chromium.org/2908353003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/version_ui.cc (right):

https://codereview.chromium.org/2908353003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/version_ui.cc:161: if (!update_cohort_name.empty()) {
How about:

base::string16 display_cohort_name;
if (!update_cohort_name.empty()) {
  display_cohort_name = l10n_util::GetStringFUTF16(IDS_VERSION_UI_COHORT_NAME,
                                                   update_cohort_name);
}
html_source->AddString(version_ui::kUpdateCohortName, display_cohort_name);


I noted as is, one branch is using base::string16 and the other is using
std::string.

https://codereview.chromium.org/2908353003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/version_ui.cc:166:
html_source->AddString(version_ui::kUpdateCohortName, std::string());
Is this because $i18n{update_cohort_name} cannot tolerate a non-existent string,
whereas i18n-content="update_cohort_name" can?

[dschuyler, 2017-06-19 19:24:53]

https://codereview.chromium.org/2908353003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/version_ui.cc (right):

https://codereview.chromium.org/2908353003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/version_ui.cc:166:
html_source->AddString(version_ui::kUpdateCohortName, std::string());
On 2017/06/17 06:23:15, Lei Zhang wrote:
> Is this because $i18n{update_cohort_name} cannot tolerate a non-existent
string,
> whereas i18n-content="update_cohort_name" can?

It is by design that $i18n won't allow the mistake of leaving out a translation
string, while i18n-content provides no such developer support.