[HBD] Update Plugins type in Website Settings Permissions UI

[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of
M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove
the Detect option and make it look the same as any other
ASK-default content type.

BUG=642564
Committed: https://crrev.com/691e0c7d46341465de7a605b5f2aee941ccfedc0
Cr-Commit-Position: refs/heads/master@{#418894}

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

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:05]

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:25:20]

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

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

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-09-01 00:55:34]

Description was changed from

==========
[HBD] Update Site Settings Permissions Dropdown from Omnibox

BUG=642564
==========

to

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove the Detect
option and make it look the same as any other ASK-default content type.

BUG=642564
==========

[tommycli, 2016-09-01 00:55:35]

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

[tommycli, 2016-09-01 00:56:04]

Description was changed from

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove the Detect
option and make it look the same as any other ASK-default content type.

BUG=642564
==========

to

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of 
M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove 
the Detect option and make it look the same as any other 
ASK-default content type.

BUG=642564
==========

[tommycli, 2016-09-01 00:56:24]

benwells: PTAL, screenshot in last comment of bug. Thank you!

[tommycli, 2016-09-01 00:56:58]

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

[commit-bot: I haz the power, 2016-09-01 00:57:38]

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

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

[commit-bot: I haz the power, 2016-09-01 01:37:29]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-09-02 00:36:15]

What does 'the last real plugin' mean? Are there other plugins or not?

[tommycli, 2016-09-02 00:38:32]

On 2016/09/02 00:36:15, benwells wrote:
> What does 'the last real plugin' mean? Are there other plugins or not?

There are three other "Plugins":

Widevine (ships with Chrome, used by Netfilx, treated as JavaScript in content
settings, just happens to use Pepper)
NaCl (also a chrome internal component, I think also treated as JS)
PDF (also a chrome internal component, no longer affected by Plugins content
settings)

So these three also appear under chrome://plugins and use Pepper, but are not
really plugins, since they have separate content settings, and are basically
Chrome internal components that happen to use the Pepper API.

[benwells, 2016-09-05 05:40:02]

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

[benwells, 2016-09-05 05:40:03]

+raymes as a plugins and permissions expert.

[raymes, 2016-09-05 06:56:31]

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:237: effective_setting
= CONTENT_SETTING_ASK;
I'm confused! In PluginsFieldTrial::EffectiveContentSetting there is a comment
which says "For Plugins, ASK is obsolete." and it returns
CONTENT_SETTING_DETECT_IMPORTANT_CONTENT if that feature is enabled, and then
here in that case we change it back to ask?

[tommycli, 2016-09-13 16:29:28]

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:237: effective_setting
= CONTENT_SETTING_ASK;
On 2016/09/05 06:56:31, raymes wrote:
> I'm confused! In PluginsFieldTrial::EffectiveContentSetting there is a comment
> which says "For Plugins, ASK is obsolete." and it returns
> CONTENT_SETTING_DETECT_IMPORTANT_CONTENT if that feature is enabled, and then
> here in that case we change it back to ask?
> 

It's unfortunate, but that's exactly what we want. Anyone with ASK in their
settings should get the DETECT behavior, since we don't support ASK anymore.
(Since Plugin Power Saver last year)

But for the HTML5 by Default feature, the behavior resembles ASK, and
eventually, once it's no longer a feature flag, we will migrate back to ASK. We
aren't actually migrating the settings yet because it's still shipping behind a
flag instead of permanently on.

It's unfortunate that we're flip flopping on the settings value, but we didn't
foresee the HTML5 by Default functionality when we worked on PPS.

And we can't just change EffectiveContentSetting, as that's used elsewhere in
the code also.

[raymes, 2016-09-14 01:29:13]

On 2016/09/13 16:29:28, tommycli wrote:
>
https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings_ui.cc (right):
> 
>
https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings_ui.cc:237:
effective_setting
> = CONTENT_SETTING_ASK;
> On 2016/09/05 06:56:31, raymes wrote:
> > I'm confused! In PluginsFieldTrial::EffectiveContentSetting there is a
comment
> > which says "For Plugins, ASK is obsolete." and it returns
> > CONTENT_SETTING_DETECT_IMPORTANT_CONTENT if that feature is enabled, and
then
> > here in that case we change it back to ask?
> > 
> 
> It's unfortunate, but that's exactly what we want. Anyone with ASK in their
> settings should get the DETECT behavior, since we don't support ASK anymore.
> (Since Plugin Power Saver last year)
> 
> But for the HTML5 by Default feature, the behavior resembles ASK, and
> eventually, once it's no longer a feature flag, we will migrate back to ASK.
We
> aren't actually migrating the settings yet because it's still shipping behind
a
> flag instead of permanently on.
> 
> It's unfortunate that we're flip flopping on the settings value, but we didn't
> foresee the HTML5 by Default functionality when we worked on PPS.
> 
> And we can't just change EffectiveContentSetting, as that's used elsewhere in
> the code also.

So behaviorally, we still want to keep the DETECT behavior? We just want the
strings to change to be the ASK strings in the case of kPreferHtmlOverPlugins?

[tommycli, 2016-09-14 01:32:17]

On 2016/09/14 01:29:13, raymes wrote:
> On 2016/09/13 16:29:28, tommycli wrote:
> >
>
https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
> > File chrome/browser/ui/website_settings/website_settings_ui.cc (right):
> > 
> >
>
https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
> > chrome/browser/ui/website_settings/website_settings_ui.cc:237:
> effective_setting
> > = CONTENT_SETTING_ASK;
> > On 2016/09/05 06:56:31, raymes wrote:
> > > I'm confused! In PluginsFieldTrial::EffectiveContentSetting there is a
> comment
> > > which says "For Plugins, ASK is obsolete." and it returns
> > > CONTENT_SETTING_DETECT_IMPORTANT_CONTENT if that feature is enabled, and
> then
> > > here in that case we change it back to ask?
> > > 
> > 
> > It's unfortunate, but that's exactly what we want. Anyone with ASK in their
> > settings should get the DETECT behavior, since we don't support ASK anymore.
> > (Since Plugin Power Saver last year)
> > 
> > But for the HTML5 by Default feature, the behavior resembles ASK, and
> > eventually, once it's no longer a feature flag, we will migrate back to ASK.
> We
> > aren't actually migrating the settings yet because it's still shipping
behind
> a
> > flag instead of permanently on.
> > 
> > It's unfortunate that we're flip flopping on the settings value, but we
didn't
> > foresee the HTML5 by Default functionality when we worked on PPS.
> > 
> > And we can't just change EffectiveContentSetting, as that's used elsewhere
in
> > the code also.
> 
> So behaviorally, we still want to keep the DETECT behavior? We just want the
> strings to change to be the ASK strings in the case of kPreferHtmlOverPlugins?

Exactly, it's super lame, but the alternative of modifying the WebUI to support
two different content settings depending on the feature flag would be also lame
and unprecedented.

If it makes you feel any better, this Options code is going to be deleted soon
once MD Settings launches.

[tommycli, 2016-09-14 20:57:13]

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

[tommycli, 2016-09-14 20:57:13]

bauerb: PTAL, this is the Website Settings equivalent to the other patch. It
also rewrites the strings based on a feature flag.

Tommy

[raymes, 2016-09-15 02:54:53]

raymes@chromium.org changed reviewers:
- bauerb@chromium.org

[raymes, 2016-09-15 02:54:54]

-bauerb who I think knows less about this code.

lgtm

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_menu_model.cc:45:
base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)
nit: Please add a comment explaining why we display the ask label for detect
important content.

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_menu_model.cc:82:
!base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)) {
nit: Please add a comment here explaining why we don't add the detect important
content option in this case

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:92:
{CONTENT_SETTINGS_TYPE_PLUGINS, IDS_WEBSITE_SETTINGS_TYPE_FLASH,
As we discussed - we need to clarify/ensure this will only apply to flash and
not other custom plugins.

[raymes, 2016-09-15 03:21:34]

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:92:
{CONTENT_SETTINGS_TYPE_PLUGINS, IDS_WEBSITE_SETTINGS_TYPE_FLASH,
On 2016/09/15 02:54:53, raymes wrote:
> As we discussed - we need to clarify/ensure this will only apply to flash and
> not other custom plugins.

Now that I think about this more, I don't think this is really a problem for
Chrome. The only way a different plugin could get loaded is via command line
flags, which aren't really a supported use case. This may be a different story
for people with custom Chromium builds though, so we may want to consider
ensuring this only applies to flash.

[tommycli, 2016-09-15 16:40:45]

raymes: thanks!

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_menu_model.cc:45:
base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)
On 2016/09/15 02:54:53, raymes wrote:
> nit: Please add a comment explaining why we display the ask label for detect
> important content.

Done.

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_menu_model.cc:82:
!base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)) {
On 2016/09/15 02:54:53, raymes wrote:
> nit: Please add a comment here explaining why we don't add the detect
important
> content option in this case

Done.

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2291323003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:92:
{CONTENT_SETTINGS_TYPE_PLUGINS, IDS_WEBSITE_SETTINGS_TYPE_FLASH,
On 2016/09/15 03:21:34, raymes wrote:
> On 2016/09/15 02:54:53, raymes wrote:
> > As we discussed - we need to clarify/ensure this will only apply to flash
and
> > not other custom plugins.
> 
> Now that I think about this more, I don't think this is really a problem for
> Chrome. The only way a different plugin could get loaded is via command line
> flags, which aren't really a supported use case. This may be a different story
> for people with custom Chromium builds though, so we may want to consider
> ensuring this only applies to flash.

I filed this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=647284

[tommycli, 2016-09-15 16:40:48]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-09-15 16:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2291323003/#ps80001
(title: "add comments")

[commit-bot: I haz the power, 2016-09-15 16:41:10]

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-15 17:21:32]

Description was changed from

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of 
M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove 
the Detect option and make it look the same as any other 
ASK-default content type.

BUG=642564
==========

to

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of 
M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove 
the Detect option and make it look the same as any other 
ASK-default content type.

BUG=642564
==========

[commit-bot: I haz the power, 2016-09-15 17:21:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 17:23:04]

Description was changed from

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of 
M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove 
the Detect option and make it look the same as any other 
ASK-default content type.

BUG=642564
==========

to

==========
[HBD] Update Plugins type in Website Settings Permissions UI

Renames Plugins => Flash since Flash is the last real plugin as of
M55.

Also, when the kPreferHtmlOverPlugins feature flag is on, remove
the Detect option and make it look the same as any other
ASK-default content type.

BUG=642564

Committed: https://crrev.com/691e0c7d46341465de7a605b5f2aee941ccfedc0
Cr-Commit-Position: refs/heads/master@{#418894}
==========

[commit-bot: I haz the power, 2016-09-15 17:23:05]

Patchset 5 (id:??) landed as
https://crrev.com/691e0c7d46341465de7a605b5f2aee941ccfedc0
Cr-Commit-Position: refs/heads/master@{#418894}