|
|
Created:
3 years, 10 months ago by dschuyler Modified:
3 years, 10 months ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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
Patch Set 1 #Patch Set 2 : unit tests #Patch Set 3 : comment #Patch Set 4 : clean up #Patch Set 5 : i18nStr to i18nPolymer #Patch Set 6 : merge with master #
Total comments: 4
Patch Set 7 : nit #
Total comments: 2
Patch Set 8 : moo #
Messages
Total messages: 52 (35 generated)
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 ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can we just not do this '$i18n*{...}' thing instead?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ==========
We chatted in person about changing this to i18nPolymer, this recent patch makes that change.
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?
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
lgtm thanks for sticking with me on tracking down why this works
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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>
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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?
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?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
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
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
The CQ bit was checked by dschuyler@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487095191369060, "parent_rev": "4873867f1022c6c96abc86f862b038d13f8be327", "commit_rev": "0e79f6961d70e8f90eb87ecf355efbb7a55b777c"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0e79f6961d70e8f90eb87ecf355e... |