Browser Plugin: Simplify NewWindow code

Browser Plugin: Simplify NewWindow code

Previously, there were two code paths in BrowserPluginGuest to request a new window (new <webview>) from the embedder: one for suppressed openers and one for unsuppressed openers. This CL unifies the two code paths.

BUG=none
Test=WebViewTest.Shim, WebViewTest.NewWindow


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

[Charlie Reis, 2013-04-04 21:48:46]

I admit that I don't understand what ViewMsg_ShowView or ViewMsg_Move do, so I'm
not able to follow what this change is doing.  Can you update the CL description
to say how the new approach works?

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:1095:
RequestNewWindowPermission(
How does this work now?

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

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:325: if (was_blocked)
Any reason, or just to be clean?

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.h (left):

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.h:199: void Terminate();
Wow, how did that stick around unimplemented?  Thanks for catching the dead
code.

https://codereview.chromium.org/13649007/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/13649007/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1358: // will be shown
immediately) and if it's not a guest (since we separately
This comment looks stale now.  For guests, we are now saving the window for
later?

https://codereview.chromium.org/13649007/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1512: if
(new_contents->GetRenderProcessHost()->IsGuest())
What if the process died and didn't have a connection?  We would have returned
NULL below, but now we return the new_contents.  Should this be below that
check?

[Fady Samuel, 2013-04-04 22:35:14]

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:1095:
RequestNewWindowPermission(
On 2013/04/04 21:48:46, creis wrote:
> How does this work now?

BrowserPluginGuest::AddNewContents handles both cases whether or not the opener
is suppressed.

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

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:325: if (was_blocked)
On 2013/04/04 21:48:46, creis wrote:
> Any reason, or just to be clean?

Yes, this gets called from WebContentsImpl::ShowCreatedWindow now. There,
was_blocked is NULL.

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.h (left):

https://codereview.chromium.org/13649007/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.h:199: void Terminate();
On 2013/04/04 21:48:46, creis wrote:
> Wow, how did that stick around unimplemented?  Thanks for catching the dead
> code.

It wasn't getting called.

https://codereview.chromium.org/13649007/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/13649007/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1358: // will be shown
immediately) and if it's not a guest (since we separately
On 2013/04/04 21:48:46, creis wrote:
> This comment looks stale now.  For guests, we are now saving the window for
> later?

We save the window until WebContentsImpl::ShowCreatedWindow, yes. Updated.

https://codereview.chromium.org/13649007/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1512: if
(new_contents->GetRenderProcessHost()->IsGuest())
On 2013/04/04 21:48:46, creis wrote:
> What if the process died and didn't have a connection?  We would have returned
> NULL below, but now we return the new_contents.  Should this be below that
> check?

If the process crashed then we'll do cleanup elsewhere: in BrowserPluginGuest.

The new contents won't have a RenderWidgetHostView yet for guests, we install
that on attachment to a BrowserPlugin.

[Charlie Reis, 2013-04-04 23:13:01]

Thanks.  LGTM.

[commit-bot: I haz the power, 2013-04-05 01:48:25]

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

[commit-bot: I haz the power, 2013-04-05 04:23:40]

Change committed as 192499