Browser Plugin: Only install BrowserPluginMessageFilter if render process supports BrowserPlugin.

Browser Plugin: Only install BrowserPluginMessageFilter if render process supports BrowserPlugin.

BUG=222403
Test=Manually test BrowserPlugin/<webview> on chrome and content_shell


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

[Charlie Reis, 2013-03-20 17:59:53]

https://codereview.chromium.org/12518033/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12518033/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:1967: content::BrowserContext*
browser_context, const GURL& url) {
url -> site_url

https://codereview.chromium.org/12518033/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/12518033/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:381:
supports_browser_plugin_(supports_browser_plugin) {
Can you add a CHECK(!is_guest || supports_browser_plugin)?

https://codereview.chromium.org/12518033/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:724: return is_guest_;
Maybe this should be supports_browser_plugin_ && is_guest_?  Best if we notice
if these ever differ.

https://codereview.chromium.org/12518033/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/12518033/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.h:65: bool
supports_browser_plugin);
Let's swap is_guest and supports_browser_plugin, since the latter is a
prerequisite for the former.

https://codereview.chromium.org/12518033/diff/1/content/public/browser/conten...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/12518033/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:481: // Returns true if the
given RenderProcessHost should listen for BrowserPlugin
This comment is stale, since there's no process provided.  We probably don't
want to be specific about that use case, since it could be used elsewhere as
well.

https://codereview.chromium.org/12518033/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:484: const GURL& url);
Let's call this site_url rather than url, since that makes it clearer that we're
talking about something like a SiteInstance's site rather than an arbitrary page
load.

https://codereview.chromium.org/12518033/diff/1/content/shell/shell_content_b...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/12518033/diff/1/content/shell/shell_content_b...
content/shell/shell_content_browser_client.cc:166: return
(CommandLine::ForCurrentProcess()->HasSwitch(
nit: no parens around CommandLine, and then fix the indent.

[Fady Samuel, 2013-03-20 21:42:38]

Addressed comments. Doh, I accidentally deleted the first patch. :-( Could you
please re-review :-(

[Charlie Reis, 2013-03-21 01:09:39]

Like like just two comments got lost in the mixup.  Other than that it seems
fine.

https://codereview.chromium.org/12518033/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/12518033/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:380:
supports_browser_plugin_(supports_browser_plugin),
Can you add a CHECK(!is_guest || supports_browser_plugin)?

https://codereview.chromium.org/12518033/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:723: return is_guest_;
Maybe this should be supports_browser_plugin_ && is_guest_?  Best if we notice
if these ever differ.

[Charlie Reis, 2013-03-21 01:10:02]

On 2013/03/21 01:09:39, creis wrote:
> Like like just two comments got lost in the mixup.

Err, looks like.  :)

[Fady Samuel, 2013-03-21 02:51:13]

https://codereview.chromium.org/12518033/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/12518033/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:380:
supports_browser_plugin_(supports_browser_plugin),
On 2013/03/21 01:09:39, creis wrote:
> Can you add a CHECK(!is_guest || supports_browser_plugin)?

No, unfortunately.

is_guest does not imply supports_browser_plugin. The latter means that the
process is capable of hosting BrowserPlugins. The former means it's a guest
process. Guests cannot host other BrowserPlugins, at least for webviews.

https://codereview.chromium.org/12518033/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:723: return is_guest_;
On 2013/03/21 01:09:39, creis wrote:
> Maybe this should be supports_browser_plugin_ && is_guest_?  Best if we notice
> if these ever differ.

No, guests don't support browser plugin.

[Charlie Reis, 2013-03-21 15:53:36]

My mistake.

John, this LGTM.  It adds a callout to ContentBrowserClient in public to see if
a given site is allowed to load BrowserPlugins.  Can you confirm the change to
content/public?

[jam, 2013-03-21 16:41:44]

lgtm

[commit-bot: I haz the power, 2013-03-21 17:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/12518033/11001

[commit-bot: I haz the power, 2013-03-22 18:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/12518033/11001

[commit-bot: I haz the power, 2013-03-23 14:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/12518033/11001

[commit-bot: I haz the power, 2013-03-23 18:39:32]

Change committed as 190057