Enable WebViewTest.NestedGuestContainerBounds for --site-per-process

Enable WebViewTest.NestedGuestContainerBounds for --site-per-process

The current implementation of BrowserPluginGuest::GetCoordinatesInEmbedderWebContents transforms
guest_view_rect_.origin() to root view coordinates, which is incorrect when the guest is inside
another guest. This is specifically true when PDF is embedded inside a <webview>.

This CL will add the logic to properly modify GetCoordinatsInEmbedderWebContents such that
the returned value is correctly within the coordinate of embedder.

BUG=581898
TBR=wjmaclean@chromium.org

Review-Url: https://codereview.chromium.org/2886013002
Cr-Commit-Position: refs/heads/master@{#472512}
Committed: https://chromium.googlesource.com/chromium/src/+/0f41f67e8aad5ed714486a40555eae4cbb88a759

[EhsanK, 2017-05-16 22:14:49]

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

[commit-bot: I haz the power, 2017-05-16 22:16:50]

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-05-17 01:48:30]

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

[commit-bot: I haz the power, 2017-05-17 01:48:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[EhsanK, 2017-05-17 15:16:12]

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

[EhsanK, 2017-05-17 15:16:13]

Hello Lucas! PTAL.

+[site-isolation-reviews] because the original patch related to PDFs inside
OOPIFs.

[lfg, 2017-05-17 18:03:25]

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

https://codereview.chromium.org/2886013002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:494: .OffsetFromOrigin();
Is this plus the transformation above the same as transforming the point to the
parent's coord space? There are functions to do that in
CrossProcessFrameConnector, but they aren't exposed in RenderWidgetHostView.

Is that something we should consider? It may not be worth it, since this is only
used for browser plugin.

WDYT?

[EhsanK, 2017-05-17 18:22:50]

Thanks Lucas. Responded to the comment. PTAL.

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

https://codereview.chromium.org/2886013002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:494: .OffsetFromOrigin();
On 2017/05/17 18:03:24, lfg wrote:
> Is this plus the transformation above the same as transforming the point to
the
> parent's coord space? 
Yes exactly.
> There are functions to do that in
> CrossProcessFrameConnector, but they aren't exposed in RenderWidgetHostView.
> 
Well we are inside content/ and they are exposed in RWHVBase so that should not
be a problem. The one I need is TransformPointToCoordSpaceForView.

> Is that something we should consider? It may not be worth it, since this is
only
> used for browser plugin.
> 
> WDYT?
The problem is the dependency on the implementation of
TransformPointToLocalCoordSpace or TransformPointToCoordSpaceForView for
RenderWidgetHostViewGuest. The RWHVG does not implement either so IMO it might
not be worth the effort (given that it is hopefully going away soon).

I could try implementing those if you think it is worth it.

[lfg, 2017-05-17 18:28:53]

lgtm


On 2017/05/17 18:22:50, EhsanK wrote:
> Thanks Lucas. Responded to the comment. PTAL.
> 
>
https://codereview.chromium.org/2886013002/diff/1/content/browser/browser_plu...
> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> 
>
https://codereview.chromium.org/2886013002/diff/1/content/browser/browser_plu...
> content/browser/browser_plugin/browser_plugin_guest.cc:494:
.OffsetFromOrigin();
> On 2017/05/17 18:03:24, lfg wrote:
> > Is this plus the transformation above the same as transforming the point to
> the
> > parent's coord space? 
> Yes exactly.
> > There are functions to do that in
> > CrossProcessFrameConnector, but they aren't exposed in RenderWidgetHostView.
> > 
> Well we are inside content/ and they are exposed in RWHVBase so that should
not
> be a problem. The one I need is TransformPointToCoordSpaceForView.
> 
> > Is that something we should consider? It may not be worth it, since this is
> only
> > used for browser plugin.
> > 
> > WDYT?
> The problem is the dependency on the implementation of
> TransformPointToLocalCoordSpace or TransformPointToCoordSpaceForView for
> RenderWidgetHostViewGuest. The RWHVG does not implement either so IMO it might
> not be worth the effort (given that it is hopefully going away soon).
> 
> I could try implementing those if you think it is worth it.

That doesn't seem worth doing.

[EhsanK, 2017-05-17 18:29:42]

Description was changed from

==========
Enable WebViewTest.NestedGuestContainerBounds for --site-per-process

The current implementation of
BrowserPluginGuest::GetCoordinatesInEmbedderWebContents transforms
guest_view_rect_.origin() to root view coordinates, which is incorrect when the
guest is inside
another guest. This is specifically true when PDF is embedded inside a
<webview>.

This CL will add the logic to properly modify GetCoordinatsInEmbedderWebContents
such that
the returned value is correctly within the coordinate of embedder.

BUG=581898
==========

to

==========
Enable WebViewTest.NestedGuestContainerBounds for --site-per-process

The current implementation of
BrowserPluginGuest::GetCoordinatesInEmbedderWebContents transforms
guest_view_rect_.origin() to root view coordinates, which is incorrect when the
guest is inside
another guest. This is specifically true when PDF is embedded inside a
<webview>.

This CL will add the logic to properly modify GetCoordinatsInEmbedderWebContents
such that
the returned value is correctly within the coordinate of embedder.

BUG=581898
TBR=wjmaclean@chromium.org
==========

[EhsanK, 2017-05-17 18:30:40]

Thanks!

[EhsanK, 2017-05-17 18:30:44]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2017-05-17 18:33: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-05-17 18:44:59]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1495045844620290, "parent_rev":
"cdf4eebcaf0e0f12a26b9292ffb13adc57c3bdec", "commit_rev":
"0f41f67e8aad5ed714486a40555eae4cbb88a759"}

[commit-bot: I haz the power, 2017-05-17 18:45:11]

Description was changed from

==========
Enable WebViewTest.NestedGuestContainerBounds for --site-per-process

The current implementation of
BrowserPluginGuest::GetCoordinatesInEmbedderWebContents transforms
guest_view_rect_.origin() to root view coordinates, which is incorrect when the
guest is inside
another guest. This is specifically true when PDF is embedded inside a
<webview>.

This CL will add the logic to properly modify GetCoordinatsInEmbedderWebContents
such that
the returned value is correctly within the coordinate of embedder.

BUG=581898
TBR=wjmaclean@chromium.org
==========

to

==========
Enable WebViewTest.NestedGuestContainerBounds for --site-per-process

The current implementation of
BrowserPluginGuest::GetCoordinatesInEmbedderWebContents transforms
guest_view_rect_.origin() to root view coordinates, which is incorrect when the
guest is inside
another guest. This is specifically true when PDF is embedded inside a
<webview>.

This CL will add the logic to properly modify GetCoordinatsInEmbedderWebContents
such that
the returned value is correctly within the coordinate of embedder.

BUG=581898
TBR=wjmaclean@chromium.org

Review-Url: https://codereview.chromium.org/2886013002
Cr-Commit-Position: refs/heads/master@{#472512}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f41f67e8aad5ed714486a40555e...
==========

[commit-bot: I haz the power, 2017-05-17 18:45:12]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0f41f67e8aad5ed714486a40555e...