[i18n] $i18nPolymer to backslash escape

[i18n] $i18nPolymer to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escape single quotes, double quotes, and
backslashes.

BUG=688254
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2681623002
Cr-Commit-Position: refs/heads/master@{#450407}
Committed: https://chromium.googlesource.com/chromium/src/+/0e79f6961d70e8f90eb87ecf355efbb7a55b777c

[dschuyler, 2017-02-07 01:25:26]

Description was changed from

==========
[i18n] $i18nStr to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escpae single quotes, double quotes, and
backslashes.

BUG=688254
==========

to

==========
[i18n] $i18nStr to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escpae single quotes, double quotes, and
backslashes.

BUG=688254
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-02-07 01:25:27]

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

[commit-bot: I haz the power, 2017-02-07 01:26:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-02-07 02:32:00]

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

[commit-bot: I haz the power, 2017-02-07 02:32:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-02-07 02:34:30]

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

[dschuyler, 2017-02-07 02:35:03]

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

[commit-bot: I haz the power, 2017-02-07 02:35:09]

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-02-07 04:34:24]

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

[commit-bot: I haz the power, 2017-02-07 04:34:25]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-02-08 05:33:18]

can we just not do this '$i18n*{...}' thing instead?

[dschuyler, 2017-02-08 19:23:26]

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

[commit-bot: I haz the power, 2017-02-08 19:24:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-02-08 19:25:23]

> can we just not do this '$i18n*{...}' thing instead?

If we change to something else, there will be trade-offs of course.

Is there something specific that you'd like to see improved?

[commit-bot: I haz the power, 2017-02-08 21:56:52]

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

[commit-bot: I haz the power, 2017-02-08 21:56:53]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-02-10 22:24:51]

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

[commit-bot: I haz the power, 2017-02-10 22:25:22]

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-02-10 23:48:26]

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

[commit-bot: I haz the power, 2017-02-10 23:48:27]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-02-11 00:50:34]

Description was changed from

==========
[i18n] $i18nStr to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escpae single quotes, double quotes, and
backslashes.

BUG=688254
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[i18n] $i18nPolymer to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escape single quotes, double quotes, and
backslashes.

BUG=688254
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-02-11 00:51:12]

We chatted in person about changing this to i18nPolymer, this recent patch makes
that change.

[Dan Beam, 2017-02-11 01:07:16]

how do we end up in situations where

  '$i18nPolymer{msgId}'

doesn't expand to

  '\"moo\" said the cow'

which shows up for a user as

  \"moo\" said the cow

same thing regarding:

  "$i18nPolymer{msgId}"

being transformed into

  "don\'t do it"

and showing as

  don\'t do it

in a message?

[dschuyler, 2017-02-11 01:30:54]

On 2017/02/11 01:07:16, Dan Beam wrote:
> how do we end up in situations where
> 
>   '$i18nPolymer{msgId}'
> 
> doesn't expand to
> 
>   '\"moo\" said the cow'
> 
> which shows up for a user as
> 
>   \"moo\" said the cow
> 
> same thing regarding:
> 
>   "$i18nPolymer{msgId}"
> 
> being transformed into
> 
>   "don\'t do it"
> 
> and showing as
> 
>   don\'t do it
> 
> in a message?

I tested the specific strings above and they look correct.
Screen shot at: http://imgur.com/a/BS29J

[Dan Beam, 2017-02-11 02:35:28]

lgtm thanks for sticking with me on tracking down why this works

[dschuyler, 2017-02-13 19:27:31]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-02-13 19:28:20]

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-02-13 21:09:35]

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

[commit-bot: I haz the power, 2017-02-13 21:09:41]

Failed to apply patch for
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:34
  error:
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:
patch does not apply

Patch:      
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
Index:
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
diff --git
a/chrome/browser/resources/settings/site_settings_page/site_settings_page.html
b/chrome/browser/resources/settings/site_settings_page/site_settings_page.html
index
5cd0d26ecd8c679eecb52264881ebd351935352b..65e4dc4a777d1c4ffdbe1f0dc7a157e586a8fef8
100644
---
a/chrome/browser/resources/settings/site_settings_page/site_settings_page.html
+++
b/chrome/browser/resources/settings/site_settings_page/site_settings_page.html
@@ -34,9 +34,9 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.cookies,
-              '$i18n{siteSettingsCookiesAllowed}',
-              '$i18n{siteSettingsBlocked}',
-              '$i18n{deleteDataPostSession}')]]
+              '$i18nPolymer{siteSettingsCookiesAllowed}',
+              '$i18nPolymer{siteSettingsBlocked}',
+              '$i18nPolymer{deleteDataPostSession}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -50,8 +50,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.location,
-              '$i18n{siteSettingsAskBeforeAccessing}',
-              '$i18n{siteSettingsBlocked}')]]
+              '$i18nPolymer{siteSettingsAskBeforeAccessing}',
+              '$i18nPolymer{siteSettingsBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -66,8 +66,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.mediaStreamCamera,
-              '$i18n{siteSettingsAskBeforeAccessing}',
-              '$i18n{siteSettingsBlocked}')]]
+              '$i18nPolymer{siteSettingsAskBeforeAccessing}',
+              '$i18nPolymer{siteSettingsBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -81,8 +81,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.mediaStreamMic,
-              '$i18n{siteSettingsAskBeforeAccessing}',
-              '$i18n{siteSettingsBlocked}')]]
+              '$i18nPolymer{siteSettingsAskBeforeAccessing}',
+              '$i18nPolymer{siteSettingsBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -97,8 +97,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.notifications,
-              '$i18n{siteSettingsAskBeforeSending}',
-              '$i18n{siteSettingsBlocked}')]]
+              '$i18nPolymer{siteSettingsAskBeforeSending}',
+              '$i18nPolymer{siteSettingsBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -113,8 +113,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.javascript,
-              '$i18n{siteSettingsAllowed}',
-              '$i18n{siteSettingsBlocked}')]]
+              '$i18nPolymer{siteSettingsAllowed}',
+              '$i18nPolymer{siteSettingsBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -128,9 +128,9 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.plugins,
-              '$i18n{siteSettingsFlashAllow}',
-              '$i18n{siteSettingsFlashBlock}',
-              '$i18n{siteSettingsFlashAskBefore}')]]
+              '$i18nPolymer{siteSettingsFlashAllow}',
+              '$i18nPolymer{siteSettingsFlashBlock}',
+              '$i18nPolymer{siteSettingsFlashAskBefore}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -144,8 +144,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.images,
-              '$i18n{siteSettingsShowAll}',
-              '$i18n{siteSettingsDontShowImages}')]]
+              '$i18nPolymer{siteSettingsShowAll}',
+              '$i18nPolymer{siteSettingsDontShowImages}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -159,8 +159,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.popups,
-              '$i18n{siteSettingsAllowed}',
-              '$i18n{siteSettingsBlocked}')]]
+              '$i18nPolymer{siteSettingsAllowed}',
+              '$i18nPolymer{siteSettingsBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -175,8 +175,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.backgroundSync,
-              '$i18n{siteSettingsAllowRecentlyClosedSites}',
-              '$i18n{siteSettingsBackgroundSyncBlocked}')]]
+              '$i18nPolymer{siteSettingsAllowRecentlyClosedSites}',
+              '$i18nPolymer{siteSettingsBackgroundSyncBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -191,8 +191,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.multipleAutomaticDownloads,
-              '$i18n{siteSettingsAutoDownloadAsk}',
-              '$i18n{siteSettingsAutoDownloadBlock}')]]
+              '$i18nPolymer{siteSettingsAutoDownloadAsk}',
+              '$i18nPolymer{siteSettingsAutoDownloadBlock}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -207,8 +207,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.ppapiBroker,
-              '$i18n{siteSettingsUnsandboxedPluginsAsk}',
-              '$i18n{siteSettingsUnsandboxedPluginsBlock}')]]
+              '$i18nPolymer{siteSettingsUnsandboxedPluginsAsk}',
+              '$i18nPolymer{siteSettingsUnsandboxedPluginsBlock}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>
@@ -223,8 +223,8 @@
         <div class="secondary">
           [[defaultSettingLabel_(
               default_.registerProtocolHandler,
-              '$i18n{siteSettingsHandlersAsk}',
-              '$i18n{siteSettingsHandlersBlocked}')]]
+              '$i18nPolymer{siteSettingsHandlersAsk}',
+              '$i18nPolymer{siteSettingsHandlersBlocked}')]]
         </div>
       </div>
       <button class="subpage-arrow" is="paper-icon-button-light"></button>

[dschuyler, 2017-02-13 21:53:53]

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

[commit-bot: I haz the power, 2017-02-13 21:54:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-02-14 00:25:54]

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
ui/base/template_expressions.cc:23: for (const auto& c : in_string) {
nit: a ref is probably bigger than a char

[commit-bot: I haz the power, 2017-02-14 00:31:09]

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

[commit-bot: I haz the power, 2017-02-14 00:31:09]

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

[dschuyler, 2017-02-14 00:38:35]

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
ui/base/template_expressions.cc:23: for (const auto& c : in_string) {
On 2017/02/14 00:25:54, Dan Beam wrote:
> nit: a ref is probably bigger than a char

I expect that internally a ref uses several bytes -- as many as a pointer.
Though sizeof will still say that c is the the size of a char (1 byte).
I'm not sure what would be implied if it were bigger than a char (or whether the
internal physical size or the external logical size should be used for the
measurement). Is there something I should change?

[Dan Beam, 2017-02-14 00:58:46]

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
ui/base/template_expressions.cc:23: for (const auto& c : in_string) {
On 2017/02/14 00:38:35, dschuyler wrote:
> On 2017/02/14 00:25:54, Dan Beam wrote:
> > nit: a ref is probably bigger than a char
> 
> I expect that internally a ref uses several bytes -- as many as a pointer.
> Though sizeof will still say that c is the the size of a char (1 byte).
> I'm not sure what would be implied if it were bigger than a char (or whether
the
> internal physical size or the external logical size should be used for the
> measurement). Is there something I should change?

can you just do

for (const char c : in_string) {

isn't that likely to be smaller than a pointer-backed ref?

[dschuyler, 2017-02-14 02:26:22]

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

[commit-bot: I haz the power, 2017-02-14 02:27:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-02-14 02:30:12]

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2681623002/diff/100001/ui/base/template_expre...
ui/base/template_expressions.cc:23: for (const auto& c : in_string) {
On 2017/02/14 00:58:46, Dan Beam wrote:
> On 2017/02/14 00:38:35, dschuyler wrote:
> > On 2017/02/14 00:25:54, Dan Beam wrote:
> > > nit: a ref is probably bigger than a char
> > 
> > I expect that internally a ref uses several bytes -- as many as a pointer.
> > Though sizeof will still say that c is the the size of a char (1 byte).
> > I'm not sure what would be implied if it were bigger than a char (or whether
> the
> > internal physical size or the external logical size should be used for the
> > measurement). Is there something I should change?
> 
> can you just do
> 
> for (const char c : in_string) {
> 
> isn't that likely to be smaller than a pointer-backed ref?

Done.

[Dan Beam, 2017-02-14 02:34:45]

still lgtm

https://codereview.chromium.org/2681623002/diff/120001/ui/base/template_expre...
File ui/base/template_expressions_unittest.cc (right):

https://codereview.chromium.org/2681623002/diff/120001/ui/base/template_expre...
ui/base/template_expressions_unittest.cc:60: substitutions["doubleSample"] =
"\"mood\" said the cow";
nit: moo

[dschuyler, 2017-02-14 04:01:36]

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

[commit-bot: I haz the power, 2017-02-14 04:01:53]

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-02-14 05:34:55]

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

[commit-bot: I haz the power, 2017-02-14 05:34:57]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-02-14 17:59:36]

https://codereview.chromium.org/2681623002/diff/120001/ui/base/template_expre...
File ui/base/template_expressions_unittest.cc (right):

https://codereview.chromium.org/2681623002/diff/120001/ui/base/template_expre...
ui/base/template_expressions_unittest.cc:60: substitutions["doubleSample"] =
"\"mood\" said the cow";
On 2017/02/14 02:34:45, Dan Beam wrote:
> nit: moo

Done.

[dschuyler, 2017-02-14 17:59:51]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-02-14 17:59:51]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2681623002/#ps140001
(title: "moo")

[commit-bot: I haz the power, 2017-02-14 18:00:19]

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-02-14 18:25:37]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487095191369060,
"parent_rev": "4873867f1022c6c96abc86f862b038d13f8be327", "commit_rev":
"0e79f6961d70e8f90eb87ecf355efbb7a55b777c"}

[commit-bot: I haz the power, 2017-02-14 18:26:14]

Description was changed from

==========
[i18n] $i18nPolymer to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escape single quotes, double quotes, and
backslashes.

BUG=688254
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[i18n] $i18nPolymer to backslash escape

This CL improves security and properly displays i18n strings containing
quotes. This will backslash escape single quotes, double quotes, and
backslashes.

BUG=688254
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2681623002
Cr-Commit-Position: refs/heads/master@{#450407}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e79f6961d70e8f90eb87ecf355e...
==========

[commit-bot: I haz the power, 2017-02-14 18:26:16]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0e79f6961d70e8f90eb87ecf355e...