[MD settings] Data driven content setting subtext

[MD settings] Data driven content setting subtext

This CL removes the large computeCategoryDesc function and changes to a
data-from-html method. This saves 20% to 50% of the setup time on the
chrome://md-settings/content page (a happy side-effect reducing the JS
needed to get the status label).

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

Review-Url: https://codereview.chromium.org/2659833002
Cr-Commit-Position: refs/heads/master@{#446844}
Committed: https://chromium.googlesource.com/chromium/src/+/e9bb765477ee57a6bee2545dd3eae38b232f547e

[dschuyler, 2017-01-27 00:18:19]

Description was changed from

==========
[MD settings] Data driven content setting subtext

This CL removes the large computeCategoryDesc function and changes to a
data-from-html method. This saves 20% to 50% of the setup time on the
chrome://md-settings/content page (a happy side-effect reducing the JS
needed to get the status label).

BUG=None
==========

to

==========
[MD settings] Data driven content setting subtext

This CL removes the large computeCategoryDesc function and changes to a
data-from-html method. This saves 20% to 50% of the setup time on the
chrome://md-settings/content page (a happy side-effect reducing the JS
needed to get the status label).

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

[dschuyler, 2017-01-27 00:18:21]

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

[commit-bot: I haz the power, 2017-01-27 00:19:40]

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

[dschuyler, 2017-01-27 00:19:54]

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

[tommycli, 2017-01-27 00:36:52]

LGTM excellent change. less code more speed.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:33:
</div>
Okay now seeing the variety of formattings, maybe I'd suggest they just all do:

<div class="secondary">
  [[defaultSettingLabel_(
      default_.cookies,
      '$i18n{siteSettingsCookiesAllowed}',
      '$i18n{siteSettingsBlocked}',
      '$i18n{deleteDataPostSession}')]]
</div>

Takes up many lines but all the same.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:10: 
nit: this was on purpose i assume?

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:18: *
An object to bind default value labels to (so they are not in the |this|
Update comment to reflect that fact that they now contain permission values
instead.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:70:
defaultSettingLabel_: function(setting, allow, block, other) {
ALLOW and BLOCK have special meanings in content setting, which doesn't always
match the label params... (sometimes the allow param contains the ask string for
instance).

Maybe it would be clearer to have a function signature like:

function(permissionValue, enabledString, disabledString, otherString)

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:75:
return other;
A nitpick, but the (other === undefined || setting == ALLOW) pattern looked odd
to me. Maybe for maximal idiot proofness, something like:

if (setting == BLOCK)
  return block;
if (setting == ALLOW)
  return allow;
if (other)
  return other;

return allow;

[Dan Beam, 2017-01-27 01:03:08]

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:33:
</div>
On 2017/01/27 00:36:52, tommycli wrote:
> Okay now seeing the variety of formattings, maybe I'd suggest they just all
do:
> 
> <div class="secondary">
>   [[defaultSettingLabel_(
>       default_.cookies,
>       '$i18n{siteSettingsCookiesAllowed}',
>       '$i18n{siteSettingsBlocked}',
>       '$i18n{deleteDataPostSession}')]]
> </div>
> 
> Takes up many lines but all the same.

using $i18n{} inside of something that's parsed as a string literal may not be
safe

[dschuyler, 2017-01-27 01:49:25]

On 2017/01/27 01:03:08, Dan Beam wrote:
>
https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
> File
> chrome/browser/resources/settings/site_settings_page/site_settings_page.html
> (right):
> 
>
https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
>
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:33:
> </div>
> On 2017/01/27 00:36:52, tommycli wrote:
> > Okay now seeing the variety of formattings, maybe I'd suggest they just all
> do:
> > 
> > <div class="secondary">
> >   [[defaultSettingLabel_(
> >       default_.cookies,
> >       '$i18n{siteSettingsCookiesAllowed}',
> >       '$i18n{siteSettingsBlocked}',
> >       '$i18n{deleteDataPostSession}')]]
> > </div>
> > 
> > Takes up many lines but all the same.
> 
> using $i18n{} inside of something that's parsed as a string literal may not be
> safe

Thanks, I should have made a comment on why this looks safe. The reasons I think
it's safe (please correct me if this sounds wrong) are: the i18n comes from
controlled files (not user input); the replacements have quotes replaced so that
escaping from the string is not an available exploit; and that if an outer quote
is left off of a parameter that creates an error/failure in the UI (unlike HTML
attr strings which are allowed to not have quotes). 
Single and double quotes are escaped here:
https://cs.chromium.org/chromium/src/net/base/escape.cc?sq=package:chromium&d...

[Dan Beam, 2017-01-27 01:55:33]

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

[Dan Beam, 2017-01-27 01:55:33]

aight, let's give it a go, then

i suppose now that \n are OK and ' -> ' should hopefully disallow break
outs.  i am mildly worried of some other vector and , not being escaped.

lgtm

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:32:
'$i18n{deleteDataPostSession}')]]
indent off

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:86:
'$i18n{siteSettingsBlocked}')]]
indent off

[commit-bot: I haz the power, 2017-01-27 02:08:11]

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

[commit-bot: I haz the power, 2017-01-27 02:08:12]

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-01-27 02:09:35]

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

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

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-01-27 03:21:41]

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

[commit-bot: I haz the power, 2017-01-27 03:21:42]

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-01-27 22:27:45]

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

[commit-bot: I haz the power, 2017-01-27 22:28:18]

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

[dschuyler, 2017-01-27 22:29:17]

Removed old unit test for the function that was removed; Added new unit test for
the function that was aded.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:32:
'$i18n{deleteDataPostSession}')]]
On 2017/01/27 01:55:33, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:33:
</div>
On 2017/01/27 00:36:52, tommycli wrote:
> Okay now seeing the variety of formattings, maybe I'd suggest they just all
do:
> 
> <div class="secondary">
>   [[defaultSettingLabel_(
>       default_.cookies,
>       '$i18n{siteSettingsCookiesAllowed}',
>       '$i18n{siteSettingsBlocked}',
>       '$i18n{deleteDataPostSession}')]]
> </div>
> 
> Takes up many lines but all the same.

Done.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:33:
</div>
On 2017/01/27 01:03:08, Dan Beam wrote:
> On 2017/01/27 00:36:52, tommycli wrote:
> > Okay now seeing the variety of formattings, maybe I'd suggest they just all
> do:
> > 
> > <div class="secondary">
> >   [[defaultSettingLabel_(
> >       default_.cookies,
> >       '$i18n{siteSettingsCookiesAllowed}',
> >       '$i18n{siteSettingsBlocked}',
> >       '$i18n{deleteDataPostSession}')]]
> > </div>
> > 
> > Takes up many lines but all the same.
> 
> using $i18n{} inside of something that's parsed as a string literal may not be
> safe

Responded above.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:86:
'$i18n{siteSettingsBlocked}')]]
On 2017/01/27 01:55:33, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:10: 
On 2017/01/27 00:36:52, tommycli wrote:
> nit: this was on purpose i assume?

Acknowledged.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:18: *
An object to bind default value labels to (so they are not in the |this|
On 2017/01/27 00:36:52, tommycli wrote:
> Update comment to reflect that fact that they now contain permission values
> instead.

Done.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:70:
defaultSettingLabel_: function(setting, allow, block, other) {
On 2017/01/27 00:36:52, tommycli wrote:
> ALLOW and BLOCK have special meanings in content setting, which doesn't always
> match the label params... (sometimes the allow param contains the ask string
for
> instance).
> 
> Maybe it would be clearer to have a function signature like:
> 
> function(permissionValue, enabledString, disabledString, otherString)

I left of the String suffix since iirc, type decorations on vars is undesired.

https://codereview.chromium.org/2659833002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:75:
return other;
On 2017/01/27 00:36:52, tommycli wrote:
> A nitpick, but the (other === undefined || setting == ALLOW) pattern looked
odd
> to me. Maybe for maximal idiot proofness, something like:
> 
> if (setting == BLOCK)
>   return block;
> if (setting == ALLOW)
>   return allow;
> if (other)
>   return other;
> 
> return allow;

I left out the blank line because the whole group is related. I'm a fan of the
blank line after an unrelated condition block -- I'm sayin' this is different.

[commit-bot: I haz the power, 2017-01-28 00:08:19]

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

[commit-bot: I haz the power, 2017-01-28 00:08:20]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-01-28 00:13:05]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-01-28 00:13:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2659833002/#ps40001
(title: "unit tests")

[commit-bot: I haz the power, 2017-01-28 00:14:03]

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-01-28 00:24:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485562385176700,
"parent_rev": "50ae54449909cd460dfa06eb4bcef261363e8cb4", "commit_rev":
"e9bb765477ee57a6bee2545dd3eae38b232f547e"}

[commit-bot: I haz the power, 2017-01-28 00:24:47]

Description was changed from

==========
[MD settings] Data driven content setting subtext

This CL removes the large computeCategoryDesc function and changes to a
data-from-html method. This saves 20% to 50% of the setup time on the
chrome://md-settings/content page (a happy side-effect reducing the JS
needed to get the status label).

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

to

==========
[MD settings] Data driven content setting subtext

This CL removes the large computeCategoryDesc function and changes to a
data-from-html method. This saves 20% to 50% of the setup time on the
chrome://md-settings/content page (a happy side-effect reducing the JS
needed to get the status label).

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

Review-Url: https://codereview.chromium.org/2659833002
Cr-Commit-Position: refs/heads/master@{#446844}
Committed:
https://chromium.googlesource.com/chromium/src/+/e9bb765477ee57a6bee2545dd3ea...
==========

[commit-bot: I haz the power, 2017-01-28 00:24:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e9bb765477ee57a6bee2545dd3ea...