MD Settings: Make Protected Content toggle change label text on toggle

MD Settings: Make Protected Content toggle change label text on toggle

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

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

[tommycli, 2017-02-07 00:30:59]

Description was changed from

==========
MD Settings: Make Protected Content toggle change label text on toggle

BUG=687783
==========

to

==========
MD Settings: Make Protected Content toggle change label text on toggle

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

[tommycli, 2017-02-07 00:32:08]

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

[tommycli, 2017-02-07 00:32:09]

dschuyler: PTAL, I took a page from your book!

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

https://codereview.chromium.org/2677413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.js:183:
getStringTernary_: function(value, trueLabel, falseLabel) {
This function is available on all platforms because the call will also be on all
platforms soon. (currently only on cros/win)

See:

https://codereview.chromium.org/2677003004/

Also, I struggled to come up with a good name for this function, but this was
the best I could do...

[tommycli, 2017-02-07 00:35:04]

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

[commit-bot: I haz the power, 2017-02-07 00:36: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 01:51:41]

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

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

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-02-07 21:46:31]

LGTM with some small alterations.

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

https://codereview.chromium.org/2677413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:450:
'$i18n{siteSettingsBlocked}')]]">
I'll want to change these to $i18nStr{} after this CL lands:
https://codereview.chromium.org/2681623002/

Could you do me the favor of adding a TODO (you can make it TODO(dschuyler) if
you prefer) on line 447 like:
// TODO(dschuyler): change i18n to i18nStr.

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

https://codereview.chromium.org/2677413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.js:183:
getStringTernary_: function(value, trueLabel, falseLabel) {
On 2017/02/07 00:32:09, tommycli wrote:
> This function is available on all platforms because the call will also be on
all
> platforms soon. (currently only on cros/win)
> 
> See:
> 
> https://codereview.chromium.org/2677003004/
> 
> Also, I struggled to come up with a good name for this function, but this was
> the best I could do...

Is the name from the ? : ternary operator? Maybe add a note in the comment
around line 177 like: 
* Works like a ternary operator. E.g. (value) ? trueLabel : falseLabel;

Also how about 
(offering an alternative name, not requiring that it change to this):
selectString(bool, string, string)

[tommycli, 2017-02-07 22:00:07]

Thanks!

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

https://codereview.chromium.org/2677413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:450:
'$i18n{siteSettingsBlocked}')]]">
On 2017/02/07 21:46:31, dschuyler wrote:
> I'll want to change these to $i18nStr{} after this CL lands:
> https://codereview.chromium.org/2681623002/
> 
> Could you do me the favor of adding a TODO (you can make it TODO(dschuyler) if
> you prefer) on line 447 like:
> // TODO(dschuyler): change i18n to i18nStr.

Done.

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

https://codereview.chromium.org/2677413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.js:183:
getStringTernary_: function(value, trueLabel, falseLabel) {
On 2017/02/07 21:46:31, dschuyler wrote:
> On 2017/02/07 00:32:09, tommycli wrote:
> > This function is available on all platforms because the call will also be on
> all
> > platforms soon. (currently only on cros/win)
> > 
> > See:
> > 
> > https://codereview.chromium.org/2677003004/
> > 
> > Also, I struggled to come up with a good name for this function, but this
was
> > the best I could do...
> 
> Is the name from the ? : ternary operator? Maybe add a note in the comment
> around line 177 like: 
> * Works like a ternary operator. E.g. (value) ? trueLabel : falseLabel;
> 
> Also how about 
> (offering an alternative name, not requiring that it change to this):
> selectString(bool, string, string)

Done.

[tommycli, 2017-02-07 22:01:34]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-02-07 22:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2677413002/#ps40001
(title: "update parens")

[commit-bot: I haz the power, 2017-02-07 22:03: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-02-07 23:10:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486504894008930,
"parent_rev": "2537528e220f768c47177abbb4c6b13d4d047117", "commit_rev":
"ed4f8466dbb2bdc88bfee21b9ecabb16132b1316"}

[commit-bot: I haz the power, 2017-02-07 23:11:39]

Description was changed from

==========
MD Settings: Make Protected Content toggle change label text on toggle

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

to

==========
MD Settings: Make Protected Content toggle change label text on toggle

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

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

[commit-bot: I haz the power, 2017-02-07 23:11:39]

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