[HBD] Only use Plugin Content Settings for Flash.

[HBD] Only use Plugin Content Settings for Flash.

All other plugins (PDF, Widevine, Nacl), as well as custom plugins,
should be treated as JavaScript.

This is because starting in 55, all the Plugin content settings UI
surfaces have been renamed the Flash content settings.

This is related to the chrome://plugins deprecation.

BUG=615738, 647284
Committed: https://crrev.com/5bd02f157d55ecf5c4c97b1161dd4fb5208d0be0
Cr-Commit-Position: refs/heads/master@{#421348}

[tommycli, 2016-09-20 22:50:08]

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

[commit-bot: I haz the power, 2016-09-20 22:50: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-20 23:03:33]

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

[commit-bot: I haz the power, 2016-09-20 23:03:35]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-09-20 23:56:11]

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

[commit-bot: I haz the power, 2016-09-20 23:56:47]

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-20 23:56:52]

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

[tommycli, 2016-09-20 23:56:53]

raymes: PTAL, does this accurately reflect our discussion in the linked bug?
thanks.

[commit-bot: I haz the power, 2016-09-21 00:33:19]

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

[commit-bot: I haz the power, 2016-09-21 00:33:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-09-21 00:38:23]

On 2016/09/20 23:56:53, tommycli wrote:
> raymes: PTAL, does this accurately reflect our discussion in the linked bug?
> thanks.

PPS tests failing because they use a custom Pepper plugin. =]

Will update those tests tomorrow. Thanks

[raymes, 2016-09-21 05:45:35]

On that note - do you know if there is a way to get flash to run in
browser_tests? I need to do that for the permissions test too.

On Wed, 21 Sep 2016 at 10:38 <tommycli@chromium.org> wrote:

> On 2016/09/20 23:56:53, tommycli wrote:
> > raymes: PTAL, does this accurately reflect our discussion in the linked
> bug?
> > thanks.
>
> PPS tests failing because they use a custom Pepper plugin. =]
>
> Will update those tests tomorrow. Thanks
>
> https://codereview.chromium.org/2356053002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2016-09-21 06:54:10]

Cool, I think this is a good approach.

[raymes, 2016-09-21 06:54:42]

lgtm

[tommycli, 2016-09-21 16:50:05]

On 2016/09/21 05:45:35, raymes wrote:
> On that note - do you know if there is a way to get flash to run in
> browser_tests? I need to do that for the permissions test too.
> 
> On Wed, 21 Sep 2016 at 10:38 <mailto:tommycli@chromium.org> wrote:
> 
> > On 2016/09/20 23:56:53, tommycli wrote:
> > > raymes: PTAL, does this accurately reflect our discussion in the linked
> > bug?
> > > thanks.
> >
> > PPS tests failing because they use a custom Pepper plugin. =]
> >
> > Will update those tests tomorrow. Thanks
> >
> > https://codereview.chromium.org/2356053002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I think you won't get Flash to run on the bots, since they build pure Chromium.

The custom Pepper plugin within the PPS Tests I think will be our salvation. We
just need to modify our code in a few places to trigger for both Flash and also
the fake test plugin if a custom command line flag is on:

Maybe just re-use the kEnablePepperTesting flag? Or make a new one?

    command_line->AppendSwitch(switches::kEnablePepperTesting);

[tommycli, 2016-09-21 18:12:29]

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

[commit-bot: I haz the power, 2016-09-21 18:12:53]

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-21 18:28:06]

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

[commit-bot: I haz the power, 2016-09-21 18:28:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-09-21 18:34:06]

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

[commit-bot: I haz the power, 2016-09-21 18:34:44]

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-21 19:26:09]

raymes: PTAL again.

The consequence of this change is that a bunch of tests (that use custom or fake
plugins) will need the kTreatCustomPluginsAsFlash flag.

I'm having second thoughts - maybe we should continue to use the Plugins content
setting for custom plugins (even though the UI only refers to Flash).

That would remove the need for this flag.

Let me know what you think Raymes.

Thank you!

Tommy

[commit-bot: I haz the power, 2016-09-21 19:41:14]

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

[commit-bot: I haz the power, 2016-09-21 19:41:15]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-09-22 00:44:30]

Would another option be to change plugin_filter_utils.cc to use the plugins
setting for Flash+Test plugins?

[tommycli, 2016-09-22 22:53:25]

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

[commit-bot: I haz the power, 2016-09-22 22:53:52]

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-22 22:55:43]

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

[commit-bot: I haz the power, 2016-09-22 22:55:44]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-09-22 22:57:31]

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

[commit-bot: I haz the power, 2016-09-22 22:58:05]

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

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

[commit-bot: I haz the power, 2016-09-22 23:23:28]

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-22 23:59:17]

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

[commit-bot: I haz the power, 2016-09-22 23:59:18]

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_...)

[tommycli, 2016-09-23 20:57:20]

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

[commit-bot: I haz the power, 2016-09-23 20:57:49]

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-23 21:15:55]

On 2016/09/22 00:44:30, raymes wrote:
> Would another option be to change plugin_filter_utils.cc to use the plugins
> setting for Flash+Test plugins?

Done! I changed the Power Saver test plugin to pretend to be Flash (for the
purposes of testing).

[tommycli, 2016-09-23 21:16:11]

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

[tommycli, 2016-09-23 21:16:12]

jochen: PTAL, thanks!

[commit-bot: I haz the power, 2016-09-23 21:30:29]

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[tommycli, 2016-09-23 22:26:15]

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

[commit-bot: I haz the power, 2016-09-23 22:27:12]

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-23 22:31:13]

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

[commit-bot: I haz the power, 2016-09-23 22:31:14]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-09-23 22:33:31]

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

[commit-bot: I haz the power, 2016-09-23 22:34:24]

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-23 23:29:20]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommycli, 2016-09-23 23:44:10]

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

[commit-bot: I haz the power, 2016-09-23 23:44:33]

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-24 00:21:49]

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

[commit-bot: I haz the power, 2016-09-24 00:22:24]

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-24 00:29:04]

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

[commit-bot: I haz the power, 2016-09-24 00:29:31]

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-24 00:58:31]

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

[commit-bot: I haz the power, 2016-09-24 00:58:32]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[raymes, 2016-09-26 08:18:30]

lg - just a few nits/questions

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_browsertest.cc (left):

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:50: #if
defined(ENABLE_PEPPER_CDMS)
I'm not sure if this was specifically testing some CDM related thing. It doesn't
seem to be specific to CDM but we should make sure we're not losing coverage.
Perhaps ask xhwang?

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:608:
ASSERT_TRUE(ppapi::RegisterPowerSaverTestPlugin(command_line));
Hmm why is this needed?

https://codereview.chromium.org/2356053002/diff/240001/chrome/common/plugin_u...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:17: if (plugin.name ==
base::ASCIIToUTF16(content::kFlashPluginName))
I think we should check that it's an out of process pepper plugin too (it
shouldn't matter, but I think it's slightly safer)

https://codereview.chromium.org/2356053002/diff/240001/chrome/test/data/load_...
File chrome/test/data/load_pepper_plugin.html (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/test/data/load_...
chrome/test/data/load_pepper_plugin.html:25: child.innerHTML = '<object type="'
+ mimeType + '" id="plugin" data="foo">' +
is data="foo" needed?

https://codereview.chromium.org/2356053002/diff/240001/content/public/test/pp...
File content/public/test/ppapi_test_utils.h (right):

https://codereview.chromium.org/2356053002/diff/240001/content/public/test/pp...
content/public/test/ppapi_test_utils.h:32: // with the Flash name, so the
Plugins Content Settings applies to it.
I might just modify this function to register this plugin for both the test mime
type and flash type. Does that seem ok to you?

[jochen (gone - plz use gerrit), 2016-09-26 15:07:56]

jochen@chromium.org changed reviewers:
+ battre@chromium.org

[jochen (gone - plz use gerrit), 2016-09-26 15:07:57]

+battre.

So we'll force users to block cookies if they don't like nacl plugins?

[battre, 2016-09-26 15:17:20]

On 2016/09/26 15:07:57, jochen (slow) wrote:
> +battre.
> 
> So we'll force users to block cookies if they don't like nacl plugins?

Do you have something that shows the bigger plan? I think the proposal is not to
group with cookies but JavaScript but then, this would be a mislabelling in 
chrome://settings/content. I am not sure I understand the proposal.

Best regards,
Dominic

[tommycli, 2016-09-26 16:29:43]

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

[tommycli, 2016-09-26 16:29:44]

xhwang: PTAL below, thanks!

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_browsertest.cc (left):

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:50: #if
defined(ENABLE_PEPPER_CDMS)
On 2016/09/26 08:18:29, raymes wrote:
> I'm not sure if this was specifically testing some CDM related thing. It
doesn't
> seem to be specific to CDM but we should make sure we're not losing coverage.
> Perhaps ask xhwang?

xhwang, can you verify that changing this to test the Power Saver plugin doesn't
decrease any coverage of the CDM plugin?

(It looked like it just used the CDM plugin because it was convenient)

[tommycli, 2016-09-26 16:38:28]

On 2016/09/26 15:17:20, battre wrote:
> On 2016/09/26 15:07:57, jochen (slow) wrote:
> > +battre.
> > 
> > So we'll force users to block cookies if they don't like nacl plugins?
> 
> Do you have something that shows the bigger plan? I think the proposal is not
to
> group with cookies but JavaScript but then, this would be a mislabelling in 
> chrome://settings/content. I am not sure I understand the proposal.
> 
> Best regards,
> Dominic

The bigger plan is shown in
https://bugs.chromium.org/p/chromium/issues/detail?id=615738. There's no design
doc as far as I'm aware.

The gist of the bigger plan: There aren't any Pepper plugins other than Flash,
PDF, Widevine, and Nacl. Because of that, we are removing chrome://plugins.

NaCl and Widevine are already exempted from the Plugin content setting. PDF is
being exempted in 55.

Therefore, we renamed all the Plugin content settings UI surfaces to "Flash".
Since this would be confusing if it applied to third party Pepper plugins
(loaded via command line --register-pepper-plugin only), this CL makes it not
apply to third party custom plugins.

We aren't aware of any third party custom plugins actually, there may not be
any.

[tommycli, 2016-09-26 16:38:41]

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

[commit-bot: I haz the power, 2016-09-26 16:39:10]

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-26 16:40:02]

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

[commit-bot: I haz the power, 2016-09-26 16:40:27]

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-26 17:23:30]

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

[commit-bot: I haz the power, 2016-09-26 17:23:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommycli, 2016-09-26 18:21:34]

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

[commit-bot: I haz the power, 2016-09-26 18:22:37]

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-26 18:53:33]

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

[commit-bot: I haz the power, 2016-09-26 18:53:34]

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_...)

[tommycli, 2016-09-26 21:54:20]

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

[commit-bot: I haz the power, 2016-09-26 21:55:02]

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-26 22:29:31]

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

[commit-bot: I haz the power, 2016-09-26 22:29:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-09-26 22:42:03]

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

[commit-bot: I haz the power, 2016-09-26 22:42:41]

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-26 23:28:07]

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

[commit-bot: I haz the power, 2016-09-26 23:28:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2016-09-26 23:44:51]

I didn't review everything. I have a few comment related to the CDM and general
readability.

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:359:
default_command_line, switches::kDisableComponentUpdate, command_line);
The test related to the CDM is special that the CDM is bundled and registered
through the component updater. This is why we need to remove the
switches::kDisableComponentUpdate to make it work. See
https://bugs.chromium.org/p/chromium/issues/detail?id=613581 for some context.

If we don't test the CDM (since it's not special in terms of content settings),
maybe we don't need to remove this flag anymore.

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:524: #endif  //
defined(WIDEVINE_CDM_AVAILABLE)
This should still be guarded by defined(ENABLE_PEPPER_CDMS), on some platforms
(e.g. Android and ChromeCast), we have WIDEVINE_CDM_AVAILABLE but it's not using
a plugin at all.

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1357: // TODO(tommycli):
Remove once we implement PPS on ALLOW. crbug.com/649814
What is PPS?

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:19:
content::WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS) {
how about in-process pepper, or it doesn't exist? if so, can we have a comment
just in case people may ask again?

actually, why do care about whether it's in-process or out-of-process flash?

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:22: 
Can we just return true here and remove all the code below?

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:39: // treat all other plugins (loaded only via
command-line), as JavaScript.
Do you mean "all other plugins that are loaded via command-line"? How about
other plugins registered internally? It'd be great to make this comment
clearer...

https://codereview.chromium.org/2356053002/diff/340001/content/public/test/pp...
File content/public/test/ppapi_test_utils.h (right):

https://codereview.chromium.org/2356053002/diff/340001/content/public/test/pp...
content/public/test/ppapi_test_utils.h:34: WARN_UNUSED_RESULT;
When we call this in various tests, readers will have no clue that the "power
saver" plugin is registered to "application/x-ppapi-tests" mime type, with a
"Flash" name. I feel this is too tricky and hard to read, until people search
for RegisterPowerSaverTestPlugin() and find the comments here.

Can we somehow rename something (e.g. the function name here) to make things
easier to understand?

[raymes, 2016-09-27 01:31:18]

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:608:
ASSERT_TRUE(ppapi::RegisterPowerSaverTestPlugin(command_line));
On 2016/09/26 08:18:29, raymes wrote:
> Hmm why is this needed?

Just in case you missed this - it wasn't clear why this was needed here?

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:19:
content::WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS) {
On 2016/09/26 23:44:51, xhwang (slow) wrote:
> how about in-process pepper, or it doesn't exist? if so, can we have a comment
> just in case people may ask again?
> 
> actually, why do care about whether it's in-process or out-of-process flash?

There is no in-process flash. We want to make sure that it's actually the Flash
pepper plugin (as opposed to some other random plugin loaded with the same name
as flash).

[battre, 2016-09-27 07:45:49]

On 2016/09/26 16:38:28, tommycli wrote:
> On 2016/09/26 15:17:20, battre wrote:
> > On 2016/09/26 15:07:57, jochen (slow) wrote:
> > > +battre.
> > > 
> > > So we'll force users to block cookies if they don't like nacl plugins?
> > 
> > Do you have something that shows the bigger plan? I think the proposal is
not
> to
> > group with cookies but JavaScript but then, this would be a mislabelling in 
> > chrome://settings/content. I am not sure I understand the proposal.
> > 
> > Best regards,
> > Dominic
> 
> The bigger plan is shown in
> https://bugs.chromium.org/p/chromium/issues/detail?id=615738. There's no
design
> doc as far as I'm aware.
> 
> The gist of the bigger plan: There aren't any Pepper plugins other than Flash,
> PDF, Widevine, and Nacl. Because of that, we are removing chrome://plugins.
> 
> NaCl and Widevine are already exempted from the Plugin content setting. PDF is
> being exempted in 55.
> 
> Therefore, we renamed all the Plugin content settings UI surfaces to "Flash".
> Since this would be confusing if it applied to third party Pepper plugins
> (loaded via command line --register-pepper-plugin only), this CL makes it not
> apply to third party custom plugins.
> 
> We aren't aware of any third party custom plugins actually, there may not be
> any.

That sounds reasonable to me.

Best regards,
Dominic

[tommycli, 2016-09-27 16:18:28]

Patchset #19 (id:360001) has been deleted

[tommycli, 2016-09-27 18:08:37]

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

[commit-bot: I haz the power, 2016-09-27 18:09:10]

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-27 18:16:33]

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

[commit-bot: I haz the power, 2016-09-27 18:16:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-09-27 18:19:56]

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

[commit-bot: I haz the power, 2016-09-27 18:20:24]

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-27 18:46:57]

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

[commit-bot: I haz the power, 2016-09-27 18:47:29]

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-27 18:51:07]

Thanks all! This is ready for another look!

I changed all the x-ppapi-tests to x-shockwave-flash per xhwang's suggestion.
(Idea stolen from raymes' patch)

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:608:
ASSERT_TRUE(ppapi::RegisterPowerSaverTestPlugin(command_line));
On 2016/09/26 08:18:29, raymes wrote:
> Hmm why is this needed?

Done. The line in PrerenderTestUtils already covers it

https://codereview.chromium.org/2356053002/diff/240001/chrome/common/plugin_u...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:17: if (plugin.name ==
base::ASCIIToUTF16(content::kFlashPluginName))
On 2016/09/26 08:18:29, raymes wrote:
> I think we should check that it's an out of process pepper plugin too (it
> shouldn't matter, but I think it's slightly safer)

Done. Thanks for the suggestion.

https://codereview.chromium.org/2356053002/diff/240001/chrome/test/data/load_...
File chrome/test/data/load_pepper_plugin.html (right):

https://codereview.chromium.org/2356053002/diff/240001/chrome/test/data/load_...
chrome/test/data/load_pepper_plugin.html:25: child.innerHTML = '<object type="'
+ mimeType + '" id="plugin" data="foo">' +
On 2016/09/26 08:18:29, raymes wrote:
> is data="foo" needed?

Yes, and I added a comment.

https://codereview.chromium.org/2356053002/diff/240001/content/public/test/pp...
File content/public/test/ppapi_test_utils.h (right):

https://codereview.chromium.org/2356053002/diff/240001/content/public/test/pp...
content/public/test/ppapi_test_utils.h:32: // with the Flash name, so the
Plugins Content Settings applies to it.
On 2016/09/26 08:18:30, raymes wrote:
> I might just modify this function to register this plugin for both the test
mime
> type and flash type. Does that seem ok to you?

No problem with me! I stole this idea from your CL :D

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:359:
default_command_line, switches::kDisableComponentUpdate, command_line);
On 2016/09/26 23:44:50, xhwang (slow) wrote:
> The test related to the CDM is special that the CDM is bundled and registered
> through the component updater. This is why we need to remove the
> switches::kDisableComponentUpdate to make it work. See
> https://bugs.chromium.org/p/chromium/issues/detail?id=613581 for some context.
> 
> If we don't test the CDM (since it's not special in terms of content
settings),
> maybe we don't need to remove this flag anymore.

We should probably still test the CDM, since users will experience breakages if
it doesn't work.

Do we still need this function though? I figured since we were testing widevine
(the production plugin), there is no need to test the Clearkey (test only
plugin).

Also - Clearkey was odd in that it was treated as a Plugin instead of JavaScript
(like widevine). So I just removed the Clearkey tests and substituted fake-Flash
instead.

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1357: // TODO(tommycli):
Remove once we implement PPS on ALLOW. crbug.com/649814
On 2016/09/26 23:44:51, xhwang (slow) wrote:
> What is PPS?

Done.

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:19:
content::WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS) {
On 2016/09/27 01:31:18, raymes wrote:
> On 2016/09/26 23:44:51, xhwang (slow) wrote:
> > how about in-process pepper, or it doesn't exist? if so, can we have a
comment
> > just in case people may ask again?
> > 
> > actually, why do care about whether it's in-process or out-of-process flash?
> 
> There is no in-process flash. We want to make sure that it's actually the
Flash
> pepper plugin (as opposed to some other random plugin loaded with the same
name
> as flash).

Actually since it seems that PLUGINS is a stricter content setting than
JAVASCRIPT, we would want to treat any plugin that MIGHT BE Flash, as Flash.
Moreover, every other place in the code that tests for Flash uses only the name,
rather than the name and type.

Therefore I removed the OUT_OF_PROCESS stipulation. Thoughts?

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:22: 
On 2016/09/26 23:44:51, xhwang (slow) wrote:
> Can we just return true here and remove all the code below?

Done.

https://codereview.chromium.org/2356053002/diff/340001/chrome/common/plugin_u...
chrome/common/plugin_utils.cc:39: // treat all other plugins (loaded only via
command-line), as JavaScript.
On 2016/09/26 23:44:51, xhwang (slow) wrote:
> Do you mean "all other plugins that are loaded via command-line"? How about
> other plugins registered internally? It'd be great to make this comment
> clearer...

Done.

https://codereview.chromium.org/2356053002/diff/340001/content/public/test/pp...
File content/public/test/ppapi_test_utils.h (right):

https://codereview.chromium.org/2356053002/diff/340001/content/public/test/pp...
content/public/test/ppapi_test_utils.h:34: WARN_UNUSED_RESULT;
On 2016/09/26 23:44:51, xhwang (slow) wrote:
> When we call this in various tests, readers will have no clue that the "power
> saver" plugin is registered to "application/x-ppapi-tests" mime type, with a
> "Flash" name. I feel this is too tricky and hard to read, until people search
> for RegisterPowerSaverTestPlugin() and find the comments here.
> 
> Can we somehow rename something (e.g. the function name here) to make things
> easier to understand?

Done.

[jochen (gone - plz use gerrit), 2016-09-27 19:36:04]

lgtm

[commit-bot: I haz the power, 2016-09-27 19:50:47]

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

[commit-bot: I haz the power, 2016-09-27 19:50:48]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-09-27 20:06:12]

On 2016/09/27 19:36:04, jochen (slow) wrote:
> lgtm

jochen: thanks!

xhwang: Did the latest patchset address your concerns? Thanks!

[tommycli, 2016-09-27 20:34:19]

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

[commit-bot: I haz the power, 2016-09-27 20:34:38]

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

[xhwang, 2016-09-27 20:35:26]

Thanks. LGTM % some more nits/comments.

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:524: #endif  //
defined(WIDEVINE_CDM_AVAILABLE)
On 2016/09/26 23:44:50, xhwang (slow) wrote:
> This should still be guarded by defined(ENABLE_PEPPER_CDMS), on some platforms
> (e.g. Android and ChromeCast), we have WIDEVINE_CDM_AVAILABLE but it's not
using
> a plugin at all.

This is not addressed.

https://codereview.chromium.org/2356053002/diff/420001/content/public/test/pp...
File content/public/test/ppapi_test_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/420001/content/public/test/pp...
content/public/test/ppapi_test_utils.cc:93: bool
RegisterFlashTestPlugin(base::CommandLine* command_line) {
Please add some more comments. It's very confusing as for why we are using
PowerSaver plugin to test flash.

[commit-bot: I haz the power, 2016-09-27 20:39:53]

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

[commit-bot: I haz the power, 2016-09-27 20:39:54]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-09-27 20:45:38]

xhwang: thank you!

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_browsertest.cc (right):

https://codereview.chromium.org/2356053002/diff/340001/chrome/browser/content...
chrome/browser/content_settings/content_settings_browsertest.cc:524: #endif  //
defined(WIDEVINE_CDM_AVAILABLE)
On 2016/09/27 20:35:26, xhwang (slow) wrote:
> On 2016/09/26 23:44:50, xhwang (slow) wrote:
> > This should still be guarded by defined(ENABLE_PEPPER_CDMS), on some
platforms
> > (e.g. Android and ChromeCast), we have WIDEVINE_CDM_AVAILABLE but it's not
> using
> > a plugin at all.
> 
> This is not addressed.

Done. Thanks! I missed that one.

https://codereview.chromium.org/2356053002/diff/420001/content/public/test/pp...
File content/public/test/ppapi_test_utils.cc (right):

https://codereview.chromium.org/2356053002/diff/420001/content/public/test/pp...
content/public/test/ppapi_test_utils.cc:93: bool
RegisterFlashTestPlugin(base::CommandLine* command_line) {
On 2016/09/27 20:35:26, xhwang (slow) wrote:
> Please add some more comments. It's very confusing as for why we are using
> PowerSaver plugin to test flash.

Done.

[tommycli, 2016-09-27 20:45:45]

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

[commit-bot: I haz the power, 2016-09-27 20:46:07]

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-27 21:40:40]

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

[commit-bot: I haz the power, 2016-09-27 21:40:42]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-09-27 21:47:07]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-09-27 21:47:08]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
xhwang@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2356053002/#ps440001
(title: "address xhwang comments")

[commit-bot: I haz the power, 2016-09-27 21:47:30]

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-27 21:53:29]

Committed patchset #22 (id:440001)

[commit-bot: I haz the power, 2016-09-27 21:56:47]

Description was changed from

==========
[HBD] Only use Plugin Content Settings for Flash.

All other plugins (PDF, Widevine, Nacl), as well as custom plugins,
should be treated as JavaScript.

This is because starting in 55, all the Plugin content settings UI
surfaces have been renamed the Flash content settings.

This is related to the chrome://plugins deprecation.

BUG=615738
==========

to

==========
[HBD] Only use Plugin Content Settings for Flash.

All other plugins (PDF, Widevine, Nacl), as well as custom plugins,
should be treated as JavaScript.

This is because starting in 55, all the Plugin content settings UI
surfaces have been renamed the Flash content settings.

This is related to the chrome://plugins deprecation.

BUG=615738

Committed: https://crrev.com/5bd02f157d55ecf5c4c97b1161dd4fb5208d0be0
Cr-Commit-Position: refs/heads/master@{#421348}
==========

[commit-bot: I haz the power, 2016-09-27 21:56:49]

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

[tommycli, 2016-09-27 22:07:30]

Description was changed from

==========
[HBD] Only use Plugin Content Settings for Flash.

All other plugins (PDF, Widevine, Nacl), as well as custom plugins,
should be treated as JavaScript.

This is because starting in 55, all the Plugin content settings UI
surfaces have been renamed the Flash content settings.

This is related to the chrome://plugins deprecation.

BUG=615738

Committed: https://crrev.com/5bd02f157d55ecf5c4c97b1161dd4fb5208d0be0
Cr-Commit-Position: refs/heads/master@{#421348}
==========

to

==========
[HBD] Only use Plugin Content Settings for Flash.

All other plugins (PDF, Widevine, Nacl), as well as custom plugins,
should be treated as JavaScript.

This is because starting in 55, all the Plugin content settings UI
surfaces have been renamed the Flash content settings.

This is related to the chrome://plugins deprecation.

BUG=615738, 647284

Committed: https://crrev.com/5bd02f157d55ecf5c4c97b1161dd4fb5208d0be0
Cr-Commit-Position: refs/heads/master@{#421348}
==========