BrowserPlugin: Plumb ViewHostMsg_CreateWindow to BrowserPluginGuest

BrowserPlugin: Plumb ViewHostMsg_CreateWindow to BrowserPluginGuest

As a prerequisite to supporting window.open, ViewHostMsg_CreateWindow messages need to be handled by BrowserPluginGuest as all guest-related state is accessed and handled there.

This patch handles and forwards ViewHostMsg_CreateWindow messages intercepted on the I/O thread from the guest process prior to reaching RenderMessageFilter and forwards them to BrowserPluginGuest to reject, instead of rejecting them immediately in BrowserPluginMessageFilter.

Future patches will replace the simple reject logic in BrowserPluginGuest with additional logic that will enable creating new windows.

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

[Fady Samuel, 2013-01-04 17:43:34]

Fairly trivial patch, just a refactor: Plumb the handling of
ViewHostMsg_CreateWindow from BrowserPluginMessageFilter to BrowserPluginGuest.

This is towards supporting window.open

[Fady Samuel, 2013-01-07 13:00:09]

Friendly *ping*

[sadrul, 2013-01-07 14:14:17]

Perhaps the CL description could be a bit more verbose (see the chromium-dev@
thread).

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_message_filter.cc (right):

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_message_filter.cc:36: void
BrowserPluginMessageFilter::OnCreateWindow(const IPC::Message& message) {
Override BrowserMessageFilter::OverrideThreadForMessage so that the message is
dispatched on the UI thread. Keep the DCHECK in the callback that it is indeed
getting dispatched in the UI thread.

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_message_filter.cc:55:
params.opener_id);
Check for non-null rvh here. (same for web_contents and guest below)

[Fady Samuel, 2013-01-07 14:58:50]

Hi Cris,

While this code is still simple, could you please verify that the message
routing I'm doing in BrowserPluginMessageFilter is not anything crazy/dangerous?

Thanks,

Fady

[Fady Samuel, 2013-01-08 17:45:18]

PTAL.

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_message_filter.cc (right):

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_message_filter.cc:36: void
BrowserPluginMessageFilter::OnCreateWindow(const IPC::Message& message) {
On 2013/01/07 14:14:17, sadrul wrote:
> Override BrowserMessageFilter::OverrideThreadForMessage so that the message is
> dispatched on the UI thread. Keep the DCHECK in the callback that it is indeed
> getting dispatched in the UI thread.

Done.

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_message_filter.cc:55:
params.opener_id);
On 2013/01/07 14:14:17, sadrul wrote:
> Check for non-null rvh here. (same for web_contents and guest below)

Is this necessary? If null web_contents will be null, and "guest" should return
a segfault?

[Cris Neckar, 2013-01-08 18:49:40]

On 2013/01/08 17:45:18, Fady Samuel wrote:
> PTAL.
> 
>
https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
> File content/browser/browser_plugin/browser_plugin_message_filter.cc (right):
> 
>
https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
> content/browser/browser_plugin/browser_plugin_message_filter.cc:36: void
> BrowserPluginMessageFilter::OnCreateWindow(const IPC::Message& message) {
> On 2013/01/07 14:14:17, sadrul wrote:
> > Override BrowserMessageFilter::OverrideThreadForMessage so that the message
is
> > dispatched on the UI thread. Keep the DCHECK in the callback that it is
indeed
> > getting dispatched in the UI thread.
> 
> Done.
> 
>
https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
> content/browser/browser_plugin/browser_plugin_message_filter.cc:55:
> params.opener_id);
> On 2013/01/07 14:14:17, sadrul wrote:
> > Check for non-null rvh here. (same for web_contents and guest below)
> 
> Is this necessary? If null web_contents will be null, and "guest" should
return
> a segfault?

IPC security review LGTM

[sadrul, 2013-01-08 18:51:16]

LGTM

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_message_filter.cc (right):

https://codereview.chromium.org/11753030/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_message_filter.cc:55:
params.opener_id);
On 2013/01/08 17:45:18, Fady Samuel wrote:
> On 2013/01/07 14:14:17, sadrul wrote:
> > Check for non-null rvh here. (same for web_contents and guest below)
> 
> Is this necessary? If null web_contents will be null, and "guest" should
return
> a segfault?

Ah, is rvh always guaranteed to be non-null? If rvh being NULL is unexpected,
then this is fine.

[commit-bot: I haz the power, 2013-01-08 19:19:09]

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

[commit-bot: I haz the power, 2013-01-08 19:20:26]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 20:57:55]

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