[HBD] Update Old Options Strings for HBD

[HBD] Update Old Options Strings for HBD

Updates Plugins => Flash in Options Content Settings.

Also updates the radio button text as:

HBD On
  Allow sites to run Flash
  Ask first before allowing sites to run Flash (recommended)
  Block sites from running Flash

HBD Off
  Allow sites to run Flash
  Detect and run only important Flash content (recommended)
  Block sites from running Flash

UI Approved. See bug.

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

Committed: https://crrev.com/ca3d26ddd65faea66834880a93a9f609dbdd00d2
Cr-Commit-Position: refs/heads/master@{#419945}

[tommycli, 2016-08-31 00:21:20]

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

[commit-bot: I haz the power, 2016-08-31 00:22:06]

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, 2016-08-31 01:17:00]

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

[commit-bot: I haz the power, 2016-08-31 01:17:01]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-08-31 16:09:12]

Description was changed from

==========
[HBD] Update Old Options Strings for HBD

BUG=622922
==========

to

==========
[HBD] Update Old Options Strings for HBD

Updates Plugins => Flash in Options Content Settings.

Also updates the radio button text as:

HBD On
  Allow sites to run Flash
  Ask first before allowing sites to run Flash (recommended)
  Block sites from running Flash

HBD Off
  Allow sites to run Flash
  Detect and run only important Flash content (recommended)
  Block sites from running Flash

UI Approved. See bug.

BUG=622922
==========

[tommycli, 2016-08-31 16:09:12]

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

[tommycli, 2016-08-31 16:09:54]

dbeam: PTAL. Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=622922#c66

[Dan Beam, 2016-09-01 06:15:16]

https://codereview.chromium.org/2296073002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/2296073002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:8079: <message name="IDS_PLUGIN_TAB_LABEL"
desc="Label for Plugins tab on Content Settings dialog">
should we rename IDS_PLUGIN to IDS_FLASH that you're changing?

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:514: // DETECT. Once
this feature is finalized, then we migrate the setting to ASK.
wat

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:527: }
please tell me there's any other way to do this... that's less cray

[tommycli, 2016-09-01 17:53:28]

Description was changed from

==========
[HBD] Update Old Options Strings for HBD

Updates Plugins => Flash in Options Content Settings.

Also updates the radio button text as:

HBD On
  Allow sites to run Flash
  Ask first before allowing sites to run Flash (recommended)
  Block sites from running Flash

HBD Off
  Allow sites to run Flash
  Detect and run only important Flash content (recommended)
  Block sites from running Flash

UI Approved. See bug.

BUG=622922
==========

to

==========
[HBD] Update Old Options Strings for HBD

Updates Plugins => Flash in Options Content Settings.

Also updates the radio button text as:

HBD On
  Allow sites to run Flash
  Ask first before allowing sites to run Flash (recommended)
  Block sites from running Flash

HBD Off
  Allow sites to run Flash
  Detect and run only important Flash content (recommended)
  Block sites from running Flash

UI Approved. See bug.

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

[tommycli, 2016-09-01 17:59:29]

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

[tommycli, 2016-09-01 17:59:33]

dbeam: Thanks!

https://codereview.chromium.org/2296073002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/2296073002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:8079: <message name="IDS_PLUGIN_TAB_LABEL"
desc="Label for Plugins tab on Content Settings dialog">
On 2016/09/01 06:15:15, Dan Beam wrote:
> should we rename IDS_PLUGIN to IDS_FLASH that you're changing?

Done. Though I did not change the Javascript-side i18n keys, since those are
tied to the content type names.
(https://cs.chromium.org/chromium/src/chrome/browser/resources/options/content...)

Eventually we want to rename CONTENT_TYPE_PLUGINS => CONTENT_TYPE_FLASH
everywhere, but that's beyond the scope of this patch.

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:514: // DETECT. Once
this feature is finalized, then we migrate the setting to ASK.
On 2016/09/01 06:15:15, Dan Beam wrote:
> wat

Acknowledged.

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:527: }
On 2016/09/01 06:15:15, Dan Beam wrote:
> please tell me there's any other way to do this... that's less cray

This is unfortunate. The story is:

ASK was originally Left Click to Play. Since we weren't ready to kill Left C2P
yet, and because DETECT is weaker than ASK, we created a new DETECT setting.
Legacy ASK users are mapped to BLOCK.

Now HBD is semantically closest to ASK, but in terms of security danger, it's
closest to DETECT, since we're exempting sites with any Site Engagement
currently.

Long story short: We aren't ready to start migrating peoples' content settings
yet, especially while it's a feature flag. Once HBD ships and it's done, we will
clean up the content settings.

https://codereview.chromium.org/2296073002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings.html (left):

https://codereview.chromium.org/2296073002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.html:209: <div
id="disable-plugins-container">
This is also no longer needed, since chrome://plugins is gone in 55.

[commit-bot: I haz the power, 2016-09-01 17:59:45]

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

[tommycli, 2016-09-01 18:04:25]

one more comment

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:527: }
On 2016/09/01 17:59:33, tommycli wrote:
> On 2016/09/01 06:15:15, Dan Beam wrote:
> > please tell me there's any other way to do this... that's less cray
> 
> This is unfortunate. The story is:
> 
> ASK was originally Left Click to Play. Since we weren't ready to kill Left C2P
> yet, and because DETECT is weaker than ASK, we created a new DETECT setting.
> Legacy ASK users are mapped to BLOCK.
> 
> Now HBD is semantically closest to ASK, but in terms of security danger, it's
> closest to DETECT, since we're exempting sites with any Site Engagement
> currently.
> 
> Long story short: We aren't ready to start migrating peoples' content settings
> yet, especially while it's a feature flag. Once HBD ships and it's done, we
will
> clean up the content settings.

I could just also change IDS_PLUGIN_ASK_RECOMMEND_RADIO to
IDS_PLUGIN_HTML_BY_DEFAULT_RECOMMENDED_RADIO, if that would prevent confusion.

I just called it ASK because the string starts with "Ask first..." and
eventually we want to migrate the DETECT setting to ASK. (After the SEI
threshold is high. See schedule here:
https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0...)

[Dan Beam, 2016-09-01 18:19:12]

i'm really not trying to be a pain, but...

is there a way we could just make a new ContentSetting called like
PREFER_NATIVE_CONTENT or something?

is it sane to think this pattern will continue (detecting important content,
phasing out older APIs, promoting our browser-y, standard-y solution)?

[tommycli, 2016-09-01 18:30:39]

On 2016/09/01 18:19:12, Dan Beam wrote:
> i'm really not trying to be a pain, but...
> 
> is there a way we could just make a new ContentSetting called like
> PREFER_NATIVE_CONTENT or something?
> 
> is it sane to think this pattern will continue (detecting important content,
> phasing out older APIs, promoting our browser-y, standard-y solution)?

Yeah we could make a new Content Setting. But I think once the SEI threshold is
ramped up to 100, it would be indistinguishable from how ASK works for Camera
and Microphone, so I do think ASK makes sense in the long term.

It would even be arguable to make it ASK sooner, but I don't think it's
appropriate immediately.

[commit-bot: I haz the power, 2016-09-01 19:24:42]

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

[commit-bot: I haz the power, 2016-09-01 19:24:43]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-09-09 01:14:40]

i would find somebody that has worked more on content settings to say this is a
good idea

[tommycli, 2016-09-13 17:55:19]

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

[tommycli, 2016-09-13 17:55:20]

raymes: Can you PTAL at this Plugins Content Settings change?

The plan is to map the DETECT setting to the ASK strings, and eventually (once
HTML5 by default is shipped for good), migrate all the DETECT settings to the
ASK setting.

Thanks,

Tommy

[raymes, 2016-09-14 01:48:05]

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:527: }
I agree it's confusing but I can't really think of a better way. The only thing
I can think of that may help would be to have some functions in a util class
which abstract the indirection we're adding everywhere, e.g.:

GetPluginsDetectImportantContentString
GetDetectExceptionString

This wouldn't help in all the cases though, e.g. with some of the cases
https://codereview.chromium.org/2291323003. So I don't feel strongly about it. 

I don't know this code so well, so if you want an extra pair of eyes you may
want to add bauerb.

[tommycli, 2016-09-14 20:55:16]

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:527: }
On 2016/09/14 01:48:05, raymes wrote:
> I agree it's confusing but I can't really think of a better way. The only
thing
> I can think of that may help would be to have some functions in a util class
> which abstract the indirection we're adding everywhere, e.g.:
> 
> GetPluginsDetectImportantContentString
> GetDetectExceptionString
> 
> This wouldn't help in all the cases though, e.g. with some of the cases
> https://codereview.chromium.org/2291323003. So I don't feel strongly about it.

> 
> I don't know this code so well, so if you want an extra pair of eyes you may
> want to add bauerb. 

Okay sounds good, I'll add bauerb. I don't want to add any more abstractions
unless it's going to be used in multiple places. I think this is the best
(temporary) solution.

[tommycli, 2016-09-14 20:56:17]

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

[tommycli, 2016-09-14 20:56:22]

bauerb: PTAL

Can you provide an extra set of eyes, this patch rewrites the DETECT strings to
the ASK strings for the HBD feature flag.

Eventually we plan to transition back to the actual ASK setting, but it would be
complex and unprecedented to actually use different content settings based on a
feature flag right now.

Thanks!

[tommycli, 2016-09-16 15:47:09]

On 2016/09/14 20:56:22, tommycli wrote:
> bauerb: PTAL
> 
> Can you provide an extra set of eyes, this patch rewrites the DETECT strings
to
> the ASK strings for the HBD feature flag.
> 
> Eventually we plan to transition back to the actual ASK setting, but it would
be
> complex and unprecedented to actually use different content settings based on
a
> feature flag right now.
> 
> Thanks!

bauerb: ping, thanks!

[Bernhard Bauer, 2016-09-16 16:30:32]

Sorry! LGTM.

[tommycli, 2016-09-16 16:33:35]

On 2016/09/16 16:30:32, Bernhard (slow until Sep 27) wrote:
> Sorry! LGTM.

No problem Bernhard. Thanks for taking a look!

[tommycli, 2016-09-16 16:34:07]

On 2016/09/16 16:33:35, tommycli wrote:
> On 2016/09/16 16:30:32, Bernhard (slow until Sep 27) wrote:
> > Sorry! LGTM.
> 
> No problem Bernhard. Thanks for taking a look!

dbeam/raymes: Ready for another look. Thanks!

[raymes, 2016-09-19 01:24:36]

lgtm

[tommycli, 2016-09-19 16:28:08]

On 2016/09/19 01:24:36, raymes wrote:
> lgtm

thanks!

[tommycli, 2016-09-19 16:28:19]

dbeam: PTAL, thanks!

[tommycli, 2016-09-20 21:31:29]

On 2016/09/19 16:28:19, tommycli wrote:
> dbeam: PTAL, thanks!

dbeam: ping, thanks!

[Dan Beam, 2016-09-20 22:53:20]

lgtm

https://codereview.chromium.org/2296073002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:516:
{"pluginsDetectImportantContent", IDS_FLASH_ASK_RECOMMENDED_RADIO},
make this ternary if possible or otherwise de-dupe common code / make it easier
for the reader to tell these keys are mutually exclusive without careful
inspection

[tommycli, 2016-09-20 23:39:04]

dbeam: thanks!

https://codereview.chromium.org/2296073002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/2296073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:516:
{"pluginsDetectImportantContent", IDS_FLASH_ASK_RECOMMENDED_RADIO},
On 2016/09/20 22:53:20, Dan Beam wrote:
> make this ternary if possible or otherwise de-dupe common code / make it
easier
> for the reader to tell these keys are mutually exclusive without careful
> inspection

Done.

[tommycli, 2016-09-20 23:39:14]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-09-20 23:39:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
raymes@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2296073002/#ps60001
(title: "Shorten strings adding using ternary")

[commit-bot: I haz the power, 2016-09-20 23:40:02]

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, 2016-09-21 02:05:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-21 02:07:23]

Description was changed from

==========
[HBD] Update Old Options Strings for HBD

Updates Plugins => Flash in Options Content Settings.

Also updates the radio button text as:

HBD On
  Allow sites to run Flash
  Ask first before allowing sites to run Flash (recommended)
  Block sites from running Flash

HBD Off
  Allow sites to run Flash
  Detect and run only important Flash content (recommended)
  Block sites from running Flash

UI Approved. See bug.

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

to

==========
[HBD] Update Old Options Strings for HBD

Updates Plugins => Flash in Options Content Settings.

Also updates the radio button text as:

HBD On
  Allow sites to run Flash
  Ask first before allowing sites to run Flash (recommended)
  Block sites from running Flash

HBD Off
  Allow sites to run Flash
  Detect and run only important Flash content (recommended)
  Block sites from running Flash

UI Approved. See bug.

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

Committed: https://crrev.com/ca3d26ddd65faea66834880a93a9f609dbdd00d2
Cr-Commit-Position: refs/heads/master@{#419945}
==========

[commit-bot: I haz the power, 2016-09-21 02:07:24]

Patchset 4 (id:??) landed as
https://crrev.com/ca3d26ddd65faea66834880a93a9f609dbdd00d2
Cr-Commit-Position: refs/heads/master@{#419945}