Only permit plug-in loads in the browser if the plug-in isn't blocked or the

Only permit plug-in loads in the browser if the plug-in isn't blocked or the
user has authorized it with a browser-mediated interaction.

BUG=172573
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180103

[Chris Evans, 2013-01-31 04:00:18]

Here's the change we chatted about.
I've rewritten it twice in an effort to avoid layering violations, minimize the
impact on content/ and keep it simple.

Hopefully it's on the right path :)

[jam, 2013-01-31 18:11:25]

lgtm

is it possible to add a regression test for this?

https://codereview.chromium.org/12086077/diff/3005/chrome/browser/plugins/chr...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/12086077/diff/3005/chrome/browser/plugins/chr...
chrome/browser/plugins/chrome_plugin_service_filter.cc:129: // We always grant
the browser permission.
nit: it would be helpful to have this comment in the header, as I originally
found it confusing why callers were passing in 0 until I saw this

https://codereview.chromium.org/12086077/diff/3005/chrome/browser/plugins/chr...
chrome/browser/plugins/chrome_plugin_service_filter.cc:216: : render_view_id(-1)
{
nit: use MSG_ROUTING_NONE

[Chris Evans, 2013-02-01 02:06:35]

On 2013/01/31 18:11:25, jam wrote:
> lgtm
> 
> is it possible to add a regression test for this?
> 
>
https://codereview.chromium.org/12086077/diff/3005/chrome/browser/plugins/chr...
> File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):
> 
>
https://codereview.chromium.org/12086077/diff/3005/chrome/browser/plugins/chr...
> chrome/browser/plugins/chrome_plugin_service_filter.cc:129: // We always grant
> the browser permission.
> nit: it would be helpful to have this comment in the header, as I originally
> found it confusing why callers were passing in 0 until I saw this
> 
>
https://codereview.chromium.org/12086077/diff/3005/chrome/browser/plugins/chr...
> chrome/browser/plugins/chrome_plugin_service_filter.cc:216: :
render_view_id(-1)
> {
> nit: use MSG_ROUTING_NONE

Two nits fixed; one test added; a further test tweaked; should also now compile
on Android.

Kicking off another full try run before landing.

[Bernhard Bauer, 2013-02-04 13:58:53]

https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/chr...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/chr...
chrome/browser/plugins/chrome_plugin_service_filter.cc:93: break;
Wait, this might not be quite right. If |use| is true, the plug-in should be
used regardless of the other checks below. This allows us to use the PDF plug-in
for Print Preview regardless of whether it's enabled or not. Did you check that?

https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_info_message_filter.h (right):

https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_info_message_filter.h:66: void GrantAccess(const
ChromeViewHostMsg_GetPluginInfo_Status& status,
This doesn't actually grant access in all cases, right? Might be worth renaming
it to make that clearer?

https://codereview.chromium.org/12086077/diff/9007/content/public/browser/plu...
File content/public/browser/plugin_service_filter.h (right):

https://codereview.chromium.org/12086077/diff/9007/content/public/browser/plu...
content/public/browser/plugin_service_filter.h:25: virtual bool
IsPluginEnabled(int render_process_id,
Naming bikeshedding comment: I think this confuses the terminology a bit. A
plug-in being "enabled" or "disabled" so far has referred to the user
consciously tweaking things in chrome://plugins, whereas the content interface
is a bit more general (for one, content doesn't know about user preferences that
enable/disable things, and furthermore, there might be other reasons why a
plug-in is not available, like extension-private plug-ins.) Would it be horrible
to change this to "available" or something?

[Chris Evans, 2013-02-04 15:59:52]

On 2013/02/04 13:58:53, Bernhard Bauer wrote:
>
https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/chr...
> File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):
> 
>
https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/chr...
> chrome/browser/plugins/chrome_plugin_service_filter.cc:93: break;
> Wait, this might not be quite right. If |use| is true, the plug-in should be
> used regardless of the other checks below. This allows us to use the PDF
plug-in
> for Print Preview regardless of whether it's enabled or not. Did you check
that?
> 
>
https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/plu...
> File chrome/browser/plugins/plugin_info_message_filter.h (right):
> 
>
https://codereview.chromium.org/12086077/diff/9007/chrome/browser/plugins/plu...
> chrome/browser/plugins/plugin_info_message_filter.h:66: void GrantAccess(const
> ChromeViewHostMsg_GetPluginInfo_Status& status,
> This doesn't actually grant access in all cases, right? Might be worth
renaming
> it to make that clearer?
> 
>
https://codereview.chromium.org/12086077/diff/9007/content/public/browser/plu...
> File content/public/browser/plugin_service_filter.h (right):
> 
>
https://codereview.chromium.org/12086077/diff/9007/content/public/browser/plu...
> content/public/browser/plugin_service_filter.h:25: virtual bool
> IsPluginEnabled(int render_process_id,
> Naming bikeshedding comment: I think this confuses the terminology a bit. A
> plug-in being "enabled" or "disabled" so far has referred to the user
> consciously tweaking things in chrome://plugins, whereas the content interface
> is a bit more general (for one, content doesn't know about user preferences
that
> enable/disable things, and furthermore, there might be other reasons why a
> plug-in is not available, like extension-private plug-ins.) Would it be
horrible
> to change this to "available" or something?

Thanks Bernhard! Follow-up tracked at https://codereview.chromium.org/12177018