Fix message routing for BrowserPlugin in iframe

Fix message routing for BrowserPlugin in iframe

Message routing in the renderer to a particular BrowserPlugin happens through BrowserPluginManager.

Previously, there was a BrowserPluginManager per RenderView that was created lazily. This CL moves BrowserPluginManager to be created once for the RenderThread. This allows correct routing to BrowserPlugins regardless of whether they live on the main page or inside an iframe.

BUG=442033
TBR=kenrb@chromium.org for trivial change to extension_messages.h

Committed: https://crrev.com/6c1dfeb5606544bb37a3ffe9ba49aa1c6a973fed
Cr-Commit-Position: refs/heads/master@{#309053}

[Fady Samuel, 2014-12-16 19:57:59]

fsamuel@chromium.org changed reviewers:
+ creis@chromium.org

[Fady Samuel, 2014-12-16 19:58:00]

Charlie, could you please take a look at this change? It's fairly mechanical.

[Fady Samuel, 2014-12-16 20:44:00]

fsamuel@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Fady Samuel, 2014-12-16 20:44:01]

+lazyboy@

[lazyboy, 2014-12-16 23:57:30]

Overall, seems reasonable.
I have a question in RenderViewImpl::DidCommitCompositorFrame() but I'm not
familiar /w the compositor thing enough to judge if that's an issue...

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:398:
msg->set_routing_id(owner_web_contents_->GetRoutingID());
The comment about this routing_id in browser_plugin_guest.h has gone stale,
please update.

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:226: virtual bool
ShouldForwardToGuestContainer(const IPC::Message& msg);
Need comment.

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.h (left):

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:28: class MockBrowserPlugin;
This was lingering around it seems, great!
nit: Can you also sort this block?

https://codereview.chromium.org/801173002/diff/60001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/801173002/diff/60001/content/renderer/render_...
content/renderer/render_view_impl.cc:4074:
BrowserPluginManager::Get()->DidCommitCompositorFrame();
I'm not sure about this one, if you have two <webview>s, in two "render view"s,
then
would it not incorrectly call DidCommitCompositorFrame to one of them?

Can we use RenderFrameObserver::DidCommitCompositorFrame()? Is that wired up?

https://codereview.chromium.org/801173002/diff/60001/extensions/common/api/gu...
File extensions/common/api/guest_view_internal.json (right):

https://codereview.chromium.org/801173002/diff/60001/extensions/common/api/gu...
extensions/common/api/guest_view_internal.json:92: "description": "The instance
ID of the guest renderer. This not exposed to developers through the API."
I'd do this in a separate CL.

[Charlie Reis, 2014-12-17 00:06:44]

Seems generally ok, though I'm not sure I understand what the problem was.  Why
was storing it on the RenderView not working in the <webview>-in-iframe case?

Also, shouldn't there be a test for this?

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:398:
msg->set_routing_id(owner_web_contents_->GetRoutingID());
What is it set to instead?

(Is this still for routed messages, or is it for control messages now?)

https://codereview.chromium.org/801173002/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/801173002/diff/60001/content/content_renderer...
content/content_renderer.gypi:50: 'public/renderer/browser_plugin_delegate.cc',
nit: .cc before .h

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:226: virtual bool
ShouldForwardToGuestContainer(const IPC::Message& msg);
This is a public interface, so this requires a comment.

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_manager.h:23: //
BrowserPluginManager manages the routing of messages to the appropriate
Since this is no longer specific to a RenderView, perhaps we should mention that
it's a singleton?

[Fady Samuel, 2014-12-17 19:30:35]

PTAL

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:398:
msg->set_routing_id(owner_web_contents_->GetRoutingID());
On 2014/12/16 23:57:30, lazyboy wrote:
> The comment about this routing_id in browser_plugin_guest.h has gone stale,
> please update.

Done.

https://codereview.chromium.org/801173002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:398:
msg->set_routing_id(owner_web_contents_->GetRoutingID());
On 2014/12/17 00:06:44, Charlie Reis wrote:
> What is it set to instead?
> 
> (Is this still for routed messages, or is it for control messages now?)

These messages are control messages now. All BrowserPluginMsgs have instance_id
as their first parameter which is used for routing to the appropriate
BrowserPlugin.

https://codereview.chromium.org/801173002/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/801173002/diff/60001/content/content_renderer...
content/content_renderer.gypi:50: 'public/renderer/browser_plugin_delegate.cc',
On 2014/12/17 00:06:44, Charlie Reis wrote:
> nit: .cc before .h

Done.

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:226: virtual bool
ShouldForwardToGuestContainer(const IPC::Message& msg);
On 2014/12/16 23:57:30, lazyboy wrote:
> Need comment.

Done.

https://codereview.chromium.org/801173002/diff/60001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:226: virtual bool
ShouldForwardToGuestContainer(const IPC::Message& msg);
On 2014/12/17 00:06:44, Charlie Reis wrote:
> This is a public interface, so this requires a comment.

Done.

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.h (left):

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:28: class MockBrowserPlugin;
On 2014/12/16 23:57:30, lazyboy wrote:
> This was lingering around it seems, great!
> nit: Can you also sort this block?

Done.

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://codereview.chromium.org/801173002/diff/60001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_manager.h:23: //
BrowserPluginManager manages the routing of messages to the appropriate
On 2014/12/17 00:06:44, Charlie Reis wrote:
> Since this is no longer specific to a RenderView, perhaps we should mention
that
> it's a singleton?

Done.

https://codereview.chromium.org/801173002/diff/60001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/801173002/diff/60001/content/renderer/render_...
content/renderer/render_view_impl.cc:4074:
BrowserPluginManager::Get()->DidCommitCompositorFrame();
On 2014/12/16 23:57:30, lazyboy wrote:
> I'm not sure about this one, if you have two <webview>s, in two "render
view"s,
> then
> would it not incorrectly call DidCommitCompositorFrame to one of them?
> 
> Can we use RenderFrameObserver::DidCommitCompositorFrame()? Is that wired up?

Passing in the routing ID.

https://codereview.chromium.org/801173002/diff/60001/extensions/common/api/gu...
File extensions/common/api/guest_view_internal.json (right):

https://codereview.chromium.org/801173002/diff/60001/extensions/common/api/gu...
extensions/common/api/guest_view_internal.json:92: "description": "The instance
ID of the guest renderer. This not exposed to developers through the API."
On 2014/12/16 23:57:30, lazyboy wrote:
> I'd do this in a separate CL.

Done.

[Charlie Reis, 2014-12-17 20:06:23]

Thanks.

On 2014/12/17 00:06:44, Charlie Reis wrote:
> Seems generally ok, though I'm not sure I understand what the problem was. 
Why
> was storing it on the RenderView not working in the <webview>-in-iframe case?
> 
> Also, shouldn't there be a test for this?

Did you miss this part of my feedback?

I'll take a look at the rest in the meantime.

[Fady Samuel, 2014-12-17 20:25:03]

On 2014/12/17 20:06:23, Charlie Reis wrote:
> Thanks.
> 
> On 2014/12/17 00:06:44, Charlie Reis wrote:
> > Seems generally ok, though I'm not sure I understand what the problem was. 
> Why
> > was storing it on the RenderView not working in the <webview>-in-iframe
case?
> > 
> > Also, shouldn't there be a test for this?
> 
> Did you miss this part of my feedback?
> 
> I'll take a look at the rest in the meantime.

Long story short: If we have a <webview> inside an iframe, contentWindow will
probably end up being null. I'll add a test.

[Fady Samuel, 2014-12-18 00:05:01]

PTAL, I added a test.

[Charlie Reis, 2014-12-18 00:27:40]

Thanks.  Comments on the test below.

On 2014/12/17 20:25:03, Fady Samuel wrote:
> On 2014/12/17 20:06:23, Charlie Reis wrote:
> > Thanks.
> > 
> > On 2014/12/17 00:06:44, Charlie Reis wrote:
> > > Seems generally ok, though I'm not sure I understand what the problem was.

> > Why
> > > was storing it on the RenderView not working in the <webview>-in-iframe
> case?
> > > 
> > > Also, shouldn't there be a test for this?
> > 
> > Did you miss this part of my feedback?
> > 
> > I'll take a look at the rest in the meantime.
> 
> Long story short: If we have a <webview> inside an iframe, contentWindow will
> probably end up being null. I'll add a test.

Sounds like the piece I was missing was that messages were being sent to the
RenderView, which delivered them to its main RenderFrame (and then on to
RenderFrameObservers).  In this case, they needed to go to subframes instead. 
This CL removes the need for the routing ID instead.

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2427: // This test verify that the
set of rules registries of a webview will be
nit: verifies

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2429: // http://crbug.com/438327
This is a different bug number.  Is it a related bug, or just a typo?

Also the comment should probably mention that this is for the iframe case, since
it's the main emphasis.

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2431:
LoadAppWithGuest("web_view/inside_iframe");
Does this file need to be added?

[Fady Samuel, 2014-12-18 00:35:51]

Fixed.

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2427: // This test verify that the
set of rules registries of a webview will be
On 2014/12/18 00:27:40, Charlie Reis wrote:
> nit: verifies

Done.

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2429: // http://crbug.com/438327
On 2014/12/18 00:27:40, Charlie Reis wrote:
> This is a different bug number.  Is it a related bug, or just a typo?
> 
> Also the comment should probably mention that this is for the iframe case,
since
> it's the main emphasis.

Doh. Blind copy and paste. Fixed.

https://codereview.chromium.org/801173002/diff/120001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2431:
LoadAppWithGuest("web_view/inside_iframe");
On 2014/12/18 00:27:39, Charlie Reis wrote:
> Does this file need to be added?

Yes.

[Charlie Reis, 2014-12-18 00:39:23]

Thanks, LGTM

[lazyboy, 2014-12-18 00:49:07]

https://chromiumcodereview.appspot.com/801173002/diff/140001/chrome/test/data...
File chrome/test/data/extensions/platform_apps/web_view/inside_iframe/main.js
(right):

https://chromiumcodereview.appspot.com/801173002/diff/140001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/inside_iframe/main.js:6: var
iframeWindow = document.querySelector('iframe').contentWindow;
There's a chance of race if iframe hasn't loaded here.
Better way would be, let inside_iframe/main.html's <iframe>
not have a src and here you'd set
document.querySelector('iframe').onload = ... run the test body ...
and then
set the src of the iframe here.

Otherwise if loading everything is slow, then this will fail.

[Fady Samuel, 2014-12-18 16:44:55]

PTAL Istiaque

https://codereview.chromium.org/801173002/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/inside_iframe/main.js
(right):

https://codereview.chromium.org/801173002/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/inside_iframe/main.js:6: var
iframeWindow = document.querySelector('iframe').contentWindow;
On 2014/12/18 00:49:07, lazyboy wrote:
> There's a chance of race if iframe hasn't loaded here.
> Better way would be, let inside_iframe/main.html's <iframe>
> not have a src and here you'd set
> document.querySelector('iframe').onload = ... run the test body ...
> and then
> set the src of the iframe here.
> 
> Otherwise if loading everything is slow, then this will fail.

Done.

[lazyboy, 2014-12-18 16:50:27]

lgtm

https://chromiumcodereview.appspot.com/801173002/diff/180001/content/renderer...
File content/renderer/browser_plugin/browser_plugin_manager.cc (right):

https://chromiumcodereview.appspot.com/801173002/diff/180001/content/renderer...
content/renderer/browser_plugin/browser_plugin_manager.cc:88: iter.Advance();
Looking at this now, it could simply be
while(..) {
  if (iter->rvid == rvid) {
    iter->DidCommit..()
  }
  iter.Advance()
}

[Fady Samuel, 2014-12-18 16:59:18]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-12-18 16:59:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801173002/200001

[commit-bot: I haz the power, 2014-12-18 17:14:50]

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

[commit-bot: I haz the power, 2014-12-18 17:14:51]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[Fady Samuel, 2014-12-18 17:22:42]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-12-18 17:22:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801173002/220001

[Fady Samuel, 2014-12-18 18:12:14]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-12-18 18:12:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801173002/240001

[commit-bot: I haz the power, 2014-12-18 19:21:52]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-12-18 19:22:26]

Patchset 13 (id:??) landed as
https://crrev.com/6c1dfeb5606544bb37a3ffe9ba49aa1c6a973fed
Cr-Commit-Position: refs/heads/master@{#309053}