Browser Plugin: Move input to RWHVGuest to support interstitial pages

Browser Plugin: Move input to RWHVGuest to support interstitial pages

BUG=273089
Committed: https://crrev.com/90e100b3b8d6149cce60045aea28ff7eb41f3483
Cr-Commit-Position: refs/heads/master@{#294170}

[Fady Samuel, 2014-09-08 20:08:55]

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

[lazyboy, 2014-09-08 20:30:49]

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:28: #include
"content/browser/web_contents/web_contents_impl.h"
Why is this required here?

https://codereview.chromium.org/554733003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_guest.h:169: const gfx::Rect&
guest_window_rect,
nit: indent.

[Fady Samuel, 2014-09-08 20:46:23]

PTAL

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:28: #include
"content/browser/web_contents/web_contents_impl.h"
On 2014/09/08 20:30:49, lazyboy wrote:
> Why is this required here?

To convert "embedder_web_contents_" to a WebContents from a WebContentsImpl.

we are not allowed to access a WebContentsImpl from RWHVGuest.

https://codereview.chromium.org/554733003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_guest.h:169: const gfx::Rect&
guest_window_rect,
On 2014/09/08 20:30:49, lazyboy wrote:
> nit: indent.

Done.

[lazyboy, 2014-09-08 20:53:19]

lgtm

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:28: #include
"content/browser/web_contents/web_contents_impl.h"
On 2014/09/08 20:46:23, Fady Samuel wrote:
> On 2014/09/08 20:30:49, lazyboy wrote:
> > Why is this required here?
> 
> To convert "embedder_web_contents_" to a WebContents from a WebContentsImpl.
> 
> we are not allowed to access a WebContentsImpl from RWHVGuest.

I'd move embedder_web_contents() to browser_plugin_guest.cc then.
It's not so obvious there's a cast involved and embedder_web_contents() is not a
simple getter.

[Fady Samuel, 2014-09-09 14:58:17]

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:28: #include
"content/browser/web_contents/web_contents_impl.h"
On 2014/09/08 20:53:19, lazyboy wrote:
> On 2014/09/08 20:46:23, Fady Samuel wrote:
> > On 2014/09/08 20:30:49, lazyboy wrote:
> > > Why is this required here?
> > 
> > To convert "embedder_web_contents_" to a WebContents from a WebContentsImpl.
> > 
> > we are not allowed to access a WebContentsImpl from RWHVGuest.
> 
> I'd move embedder_web_contents() to browser_plugin_guest.cc then.
> It's not so obvious there's a cast involved and embedder_web_contents() is not
a
> simple getter.


Technically I should also be renaming this to GetEmbedderWebContents to indicate
this is a non-trivial accessor. However, that requires my updating multiple
callsites with many OWNERS. Could I do that in a separate cleanup CL?

[lazyboy, 2014-09-09 15:24:04]

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:28: #include
"content/browser/web_contents/web_contents_impl.h"
On 2014/09/09 14:58:17, Fady Samuel wrote:
> On 2014/09/08 20:53:19, lazyboy wrote:
> > On 2014/09/08 20:46:23, Fady Samuel wrote:
> > > On 2014/09/08 20:30:49, lazyboy wrote:
> > > > Why is this required here?
> > > 
> > > To convert "embedder_web_contents_" to a WebContents from a
WebContentsImpl.
> > > 
> > > we are not allowed to access a WebContentsImpl from RWHVGuest.
> > 
> > I'd move embedder_web_contents() to browser_plugin_guest.cc then.
> > It's not so obvious there's a cast involved and embedder_web_contents() is
not
> a
> > simple getter.
> 
> 
> Technically I should also be renaming this to GetEmbedderWebContents to
indicate
> this is a non-trivial accessor. However, that requires my updating multiple
> callsites with many OWNERS. Could I do that in a separate cleanup CL?

Sure, I'd put a todo here then.

[lazyboy, 2014-09-09 15:24:05]

lgtm

[Fady Samuel, 2014-09-09 16:28:05]

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

[Fady Samuel, 2014-09-09 16:28:05]

+creis@: Charlie, could you please review:

render_widget_host_view_guest
web_contents_view_guest.

[Charlie Reis, 2014-09-09 21:22:14]

Mostly looks good, but I don't understand the change to the return type of
embedder_web_contents().  I didn't see why it was needed, and it causes us to
need extra casts.

https://chromiumcodereview.appspot.com/554733003/diff/100001/content/browser/...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/554733003/diff/100001/content/browser/...
content/browser/browser_plugin/browser_plugin_guest.h:131: // TODO(fsamuel):
This is a non-trivial accessor because it converts a
I guess I'm missing the relevant detail-- how does the conversion make it
non-trivial?  Isn't that just an upcast?

If it is just an upcast, I also don't see why the change was needed.

[Fady Samuel, 2014-09-09 21:23:45]

On 2014/09/09 21:22:14, Charlie Reis wrote:
> Mostly looks good, but I don't understand the change to the return type of
> embedder_web_contents().  I didn't see why it was needed, and it causes us to
> need extra casts.
> 
>
https://chromiumcodereview.appspot.com/554733003/diff/100001/content/browser/...
> File content/browser/browser_plugin/browser_plugin_guest.h (right):
> 
>
https://chromiumcodereview.appspot.com/554733003/diff/100001/content/browser/...
> content/browser/browser_plugin/browser_plugin_guest.h:131: // TODO(fsamuel):
> This is a non-trivial accessor because it converts a
> I guess I'm missing the relevant detail-- how does the conversion make it
> non-trivial?  Isn't that just an upcast?
> 
> If it is just an upcast, I also don't see why the change was needed.


According to the presubmit checks render_widget_host_view_guest.cc isn't allowed
to know WebContentsImpl but it needs access to the embedder WebContents.

[Charlie Reis, 2014-09-10 00:28:21]

Ah, I see.  One alternative suggestion below, which might be cleaner.

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:133: if (rwhv &&
rwhv->OnMessageReceivedFromEmbedder(message))
Can we pass embedder_web_contents_ here instead of changing the accessor?

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:131: // TODO(fsamuel):
This is a non-trivial accessor because it converts a
On 2014/09/09 21:22:14, Charlie Reis wrote:
> I guess I'm missing the relevant detail-- how does the conversion make it
> non-trivial?  Isn't that just an upcast?

One part of my question was about the TODO, which I don't think is necessary. 
It's still trivial and can remain inlined.


> If it is just an upcast, I also don't see why the change was needed.

Thanks for the explanation, but let's avoid the change of return type if we can.
 One suggestion below.

https://codereview.chromium.org/554733003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/554733003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:71:
OnHandleInputEvent)
To pass the embedder_web_contents to the handler, you can use
IPC_BEGIN_MESSAGE_MAP_WITH_PARAM.

(Nasko pointed me to an example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge...)

[Fady Samuel, 2014-09-10 02:51:37]

PTAL Charlie!

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:133: if (rwhv &&
rwhv->OnMessageReceivedFromEmbedder(message))
On 2014/09/10 00:28:21, Charlie Reis wrote:
> Can we pass embedder_web_contents_ here instead of changing the accessor?

Done.

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:131: // TODO(fsamuel):
This is a non-trivial accessor because it converts a
On 2014/09/10 00:28:21, Charlie Reis wrote:
> On 2014/09/09 21:22:14, Charlie Reis wrote:
> > I guess I'm missing the relevant detail-- how does the conversion make it
> > non-trivial?  Isn't that just an upcast?
> 
> One part of my question was about the TODO, which I don't think is necessary. 
> It's still trivial and can remain inlined.
> 
> 
> > If it is just an upcast, I also don't see why the change was needed.
> 
> Thanks for the explanation, but let's avoid the change of return type if we
can.
>  One suggestion below.

Done.

https://codereview.chromium.org/554733003/diff/100001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:131: // TODO(fsamuel):
This is a non-trivial accessor because it converts a
On 2014/09/09 21:22:14, Charlie Reis wrote:
> I guess I'm missing the relevant detail-- how does the conversion make it
> non-trivial?  Isn't that just an upcast?
> 
> If it is just an upcast, I also don't see why the change was needed.

Done.

https://codereview.chromium.org/554733003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/554733003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:71:
OnHandleInputEvent)
On 2014/09/10 00:28:21, Charlie Reis wrote:
> To pass the embedder_web_contents to the handler, you can use
> IPC_BEGIN_MESSAGE_MAP_WITH_PARAM.
> 
> (Nasko pointed me to an example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge...)

Done.

[Charlie Reis, 2014-09-10 04:42:05]

Thanks, that's much better!  LGTM.

https://codereview.chromium.org/554733003/diff/140001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/554733003/diff/140001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:272: return
embedder_web_contents()->GetRenderWidgetHostView();
These changes aren't really necessary anymore.

[Fady Samuel, 2014-09-10 11:43:45]

CQ'ing.

https://codereview.chromium.org/554733003/diff/140001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/554733003/diff/140001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:272: return
embedder_web_contents()->GetRenderWidgetHostView();
On 2014/09/10 04:42:04, Charlie Reis wrote:
> These changes aren't really necessary anymore.

Done.

[Fady Samuel, 2014-09-10 11:43:50]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-09-10 11:44:16]

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

[commit-bot: I haz the power, 2014-09-10 12:00:11]

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

[commit-bot: I haz the power, 2014-09-10 12:00:12]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Fady Samuel, 2014-09-10 13:25:10]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:25:55]

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

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

Committed patchset #10 (id:180001) as d96390684c1e6748ab8436492084468678f25ac4

[commit-bot: I haz the power, 2014-09-10 14:32:23]

Patchset 10 (id:??) landed as
https://crrev.com/90e100b3b8d6149cce60045aea28ff7eb41f3483
Cr-Commit-Position: refs/heads/master@{#294170}