Fix interstitials on OOPIF-based guests.

Fix interstitials on OOPIF-based guests.

This CL attaches the interstitial RenderWidgetHostViewChildFrame to a
CrossProcessFrameConnector in a RenderFrameProxyHost in an outer
WebContents when an interstitial page gets attached. It also routes
input events in the interstitial through the delegate's WebContents
RenderWidgetHostInputEventRouter.

BUG=660373
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2797473005
Cr-Commit-Position: refs/heads/master@{#463473}
Committed: https://chromium.googlesource.com/chromium/src/+/1453e416f1a511576ff3ee6641e6a77a651b2ef4

[lfg, 2017-04-04 19:00:49]

Description was changed from

==========
Fix interstitials on OOPIF-based guests.

This CL attaches the interstitial RenderWidgetHostViewChildFrame to a
CrossProcessFrameConnector in a RenderFrameProxyHost in an outer
WebContents when an interstitial page gets attached. It also routes
input events in the interstitial through the delegate's WebContents
RenderWidgetHostInputEventRouter.

BUG=660373
==========

to

==========
Fix interstitials on OOPIF-based guests.

This CL attaches the interstitial RenderWidgetHostViewChildFrame to a
CrossProcessFrameConnector in a RenderFrameProxyHost in an outer
WebContents when an interstitial page gets attached. It also routes
input events in the interstitial through the delegate's WebContents
RenderWidgetHostInputEventRouter.

BUG=660373
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[lfg, 2017-04-04 19:00:54]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 19:01:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2017-04-04 19:53:10]

lfg@chromium.org changed reviewers:
+ kenrb@chromium.org

[lfg, 2017-04-04 19:53:11]

Ken, can you do a first pass?

[commit-bot: I haz the power, 2017-04-04 20:27:56]

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

[commit-bot: I haz the power, 2017-04-04 20:27:57]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-04-05 20:00:00]

This all looks fine, although I don't know enough about the focus code in
WebContentsImpl to comment on that.

Should you also add tests to ensure that a focused interstitial is correctly
returned, and maybe that hit testing a mouse event works correctly?

[lfg, 2017-04-05 21:19:11]

On 2017/04/05 20:00:00, kenrb wrote:
> This all looks fine, although I don't know enough about the focus code in
> WebContentsImpl to comment on that.
> 
> Should you also add tests to ensure that a focused interstitial is correctly
> returned, and maybe that hit testing a mouse event works correctly?

I've added a test that ensures that the interstitial is registered within the
RenderWidgetHostInputEventRouter, which at least guarantees that it got a
surface and can be hittested against. To do the full hittesting of mouse
events I would have to expose the entirety of RenderWidgetHostInputEventRouter
on the content/public/ API, which didn't seem necessary, as we already have
other tests that test the hittesting on OOPIFs.

As for the focused interstitial, it's also not exposed outside of content/,
but it's a lot simpler to just add a browser_test_utils that exposes it. I'll
work on that.

[kenrb, 2017-04-06 15:03:40]

On 2017/04/05 21:19:11, lfg wrote:
> I've added a test that ensures that the interstitial is registered within the
> RenderWidgetHostInputEventRouter, which at least guarantees that it got a
> surface and can be hittested against. To do the full hittesting of mouse
> events I would have to expose the entirety of RenderWidgetHostInputEventRouter
> on the content/public/ API, which didn't seem necessary, as we already have
> other tests that test the hittesting on OOPIFs.
> 

Can you just route an event to the top-level RenderWidgetHost, and ensure that
it goes to the interstitial? The purpose would be to ensure that logic in the
RenderWidgetHostInputEventRouter doesn't mess things up, because we have had
regressions for special case widget targets as we keep adding complexity there.

[lfg, 2017-04-06 18:23:33]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 18:24:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 18:38:26]

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

[commit-bot: I haz the power, 2017-04-06 18:38:27]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[lfg, 2017-04-06 18:39:32]

Patchset #2 (id:20001) has been deleted

[lfg, 2017-04-06 18:40:14]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 18:41:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 19:49:02]

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

[commit-bot: I haz the power, 2017-04-06 19:49:03]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-04-06 22:37:10]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 22:38:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 23:20:07]

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

[commit-bot: I haz the power, 2017-04-06 23:20:08]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[lfg, 2017-04-07 07:44:20]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 07:44:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 08:45:54]

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

[commit-bot: I haz the power, 2017-04-07 08:45:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lfg, 2017-04-07 16:42:19]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 16:42:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2017-04-07 16:44:57]

Ken, please take another look.

I've added a test that asserts that the interstitial is unfocused, then sends a
click event that gets hittested and routed through the router and then asserts
that the interstitial got focused.

I also fixed an issue where the interstitial has to set the focused WebContents,
otherwise it only gets focused if the guest was previously focused.

[kenrb, 2017-04-07 17:06:31]

lgtm with a nit.

https://codereview.chromium.org/2797473005/diff/80001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2797473005/diff/80001/content/public/test/bro...
content/public/test/browser_test_utils.h:370: // Rout the |event| through the
RenderWidgetHostInputEventRouter.
You might want to elaborate this comment a bit. The important part is that this
function allows correct targeting of mouse events when there are OOPIFs in the
page.

[commit-bot: I haz the power, 2017-04-07 17:39:55]

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

[commit-bot: I haz the power, 2017-04-07 17:39:56]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-04-07 18:00:46]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[lfg, 2017-04-07 18:01:00]

lfg@chromium.org changed reviewers:
+ avi@chromium.org

[lfg, 2017-04-07 18:01:00]

Avi, please take a look.

[commit-bot: I haz the power, 2017-04-07 18:01:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 19:05:45]

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

[commit-bot: I haz the power, 2017-04-07 19:05:46]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-04-08 00:30:13]

This looks reasonable. I'm not an interstitial expert but from what I know this
lgtm.

More of WebContents dealing with the guts of interstitial pages but ugh. We need
to revamp all of that.

https://codereview.chromium.org/2797473005/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2797473005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1975: return
GetFocusedWebContents()->GetMainFrame()->GetRenderWidgetHost();
might as well use focused_web_contents here

[lfg, 2017-04-10 17:33:05]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 17:33:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2017-04-10 17:33:41]

lfg@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[lfg, 2017-04-10 17:33:42]

+wjmaclean for web_view_browsertest.cc.

https://codereview.chromium.org/2797473005/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2797473005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1975: return
GetFocusedWebContents()->GetMainFrame()->GetRenderWidgetHost();
On 2017/04/08 00:30:12, Avi (OOO 10-12 April) wrote:
> might as well use focused_web_contents here

Done.

[commit-bot: I haz the power, 2017-04-10 17:50:17]

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

[commit-bot: I haz the power, 2017-04-10 17:50:18]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[wjmaclean, 2017-04-10 17:56:41]

On 2017/04/10 17:33:42, lfg wrote:
> +wjmaclean for web_view_browsertest.cc.
> 
>
https://codereview.chromium.org/2797473005/diff/100001/content/browser/web_co...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/2797473005/diff/100001/content/browser/web_co...
> content/browser/web_contents/web_contents_impl.cc:1975: return
> GetFocusedWebContents()->GetMainFrame()->GetRenderWidgetHost();
> On 2017/04/08 00:30:12, Avi (OOO 10-12 April) wrote:
> > might as well use focused_web_contents here
> 
> Done.

lgtm

[lfg, 2017-04-10 18:53:39]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 18:54:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2017-04-10 19:51:01]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2017-04-10 19:51:02]

The CQ bit was checked by lfg@chromium.org

[lfg, 2017-04-10 19:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
avi@chromium.org, wjmaclean@chromium.org
Link to the patchset: https://codereview.chromium.org/2797473005/#ps140001
(title: "rebase, fix blink renaming issues")

[commit-bot: I haz the power, 2017-04-10 19:51:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 21:21:39]

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

[commit-bot: I haz the power, 2017-04-10 21:21:41]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[lfg, 2017-04-10 21:23:15]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2017-04-10 21:24:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 21:40:33]

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

[commit-bot: I haz the power, 2017-04-10 21:40:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lfg, 2017-04-10 21:41:24]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2017-04-10 21:42:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 21:54:29]

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

[commit-bot: I haz the power, 2017-04-10 21:54:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lfg, 2017-04-10 22:14:57]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 22:15:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2017-04-11 00:11:00]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2017-04-11 00:11:00]

The CQ bit was checked by lfg@chromium.org

[lfg, 2017-04-11 00:11:01]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
avi@chromium.org, wjmaclean@chromium.org
Link to the patchset: https://codereview.chromium.org/2797473005/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-11 00:11:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 00:48:55]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491869460932160,
"parent_rev": "6f41f62772e6617844594d3a40e4394f1d6aa923", "commit_rev":
"1453e416f1a511576ff3ee6641e6a77a651b2ef4"}

[commit-bot: I haz the power, 2017-04-11 00:49:42]

Description was changed from

==========
Fix interstitials on OOPIF-based guests.

This CL attaches the interstitial RenderWidgetHostViewChildFrame to a
CrossProcessFrameConnector in a RenderFrameProxyHost in an outer
WebContents when an interstitial page gets attached. It also routes
input events in the interstitial through the delegate's WebContents
RenderWidgetHostInputEventRouter.

BUG=660373
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix interstitials on OOPIF-based guests.

This CL attaches the interstitial RenderWidgetHostViewChildFrame to a
CrossProcessFrameConnector in a RenderFrameProxyHost in an outer
WebContents when an interstitial page gets attached. It also routes
input events in the interstitial through the delegate's WebContents
RenderWidgetHostInputEventRouter.

BUG=660373
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2797473005
Cr-Commit-Position: refs/heads/master@{#463473}
Committed:
https://chromium.googlesource.com/chromium/src/+/1453e416f1a511576ff3ee6641e6...
==========

[commit-bot: I haz the power, 2017-04-11 00:49:44]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1453e416f1a511576ff3ee6641e6...