Fix the position of context menu for BrowserPlugins inside OOPIF

Fix the position of context menu for BrowserPlugins inside OOPIF

Currently, we do not consider the displacement of OOPIF view on page when
update the geomtery of BrowserPluginGuest. This causes the context menu
offset (the displacement between WebContents's) to be wrong.

This CL will make the following changes to fix this issue:
1- The reported rect in OnUpdateGeometry() is transformed to owner view's
coordinate space.

2- We make sure BrowserPlugin does its own conversions using RenderWidget
of the local root containing the BrowserPlugin (as opposed to the RenderView
which seems wrong).

3- We make sure BrowserPlugin updates geometry when it is attached.

BUG=670433
Review-Url: https://codereview.chromium.org/2544223002
Cr-Commit-Position: refs/heads/master@{#446448}
Committed: https://chromium.googlesource.com/chromium/src/+/aeddbee1c18102e37b5dc083c99a337e12afa759

[EhsanK, 2016-12-02 05:35:54]

Description was changed from

==========
Fix the position of context menu for BrowserPlugins inside OOPIF

Currently, we do not consider the displacement of OOPIF view on page when
update the geomtery of BrowserPluginGuest. This causes the context menu
offset (the displacement between WebContents's) to be wrong.

This CL will make the following changes to fix this issue:
1- The reported rect in OnUpdateGeometry() is transformed to owner view's
coordinate space.

2- We make sure BrowserPlugin does its own conversions using RenderWidget
of the local root containing the BrowserPlugin (as opposed to the RenderView
which seems wrong).

3- We make sure BrowserPlugin updates geometry when it is attached.

BUG=670433
==========

to

==========
Fix the position of context menu for BrowserPlugins inside OOPIF

Currently, we do not consider the displacement of OOPIF view on page when
update the geomtery of BrowserPluginGuest. This causes the context menu
offset (the displacement between WebContents's) to be wrong.

This CL will make the following changes to fix this issue:
1- The reported rect in OnUpdateGeometry() is transformed to owner view's
coordinate space.

2- We make sure BrowserPlugin does its own conversions using RenderWidget
of the local root containing the BrowserPlugin (as opposed to the RenderView
which seems wrong).

3- We make sure BrowserPlugin updates geometry when it is attached.

BUG=670433
==========

[EhsanK, 2016-12-02 23:46:18]

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

[commit-bot: I haz the power, 2016-12-02 23:47:19]

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

[EhsanK, 2016-12-02 23:47:40]

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

[EhsanK, 2016-12-02 23:47:41]

Charlie, could you please review this CL? Thanks!

[Charlie Reis, 2016-12-03 00:08:40]

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

[Charlie Reis, 2016-12-03 00:08:41]

Thanks!  I can give an owner's stamp, but I don't know the coordinate code well
enough to review this.  Maybe lfg@ or kenrb@ can take a look first?

Is it possible to cover this with a test, or are context menus tricky?  I know
we've had bugs like this in the past.

[EhsanK, 2016-12-03 00:31:09]

Thanks!

lfg@, kenrb@: Could you please take a look?

Thanks!

[commit-bot: I haz the power, 2016-12-03 01:22:36]

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

[commit-bot: I haz the power, 2016-12-03 01:22:37]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-12-05 19:24:44]

On 2016/12/03 00:31:09, EhsanK wrote:
> Thanks!
> 
> lfg@, kenrb@: Could you please take a look?
> 
> Thanks!

creis@: I might be able to add a test for this as well. I just need to figure
out the best spot in chrome/* for the test.

[lfg, 2016-12-05 19:44:18]

I'm a bit confused and have some questions.

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

https://codereview.chromium.org/2544223002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:474: gfx::Vector2d
offset_from_embedder = guest_window_rect_.OffsetFromOrigin();
If we are transforming guest_window_rect to the root coordinates, then this
isn't an offset_from_embedder anymore.

Also, does this means that we were sending all events with wrong coordinates to
the guests?

Does this work with BrowserPlugins embedding other BrowserPlugins? For example,
<appview>?

[EhsanK, 2016-12-06 00:08:38]

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

[EhsanK, 2016-12-06 00:08:54]

Thanks Lucas! 

PTAL.

I added a browser test. The type of test added is more suited to be an
interactive test but I already had some code to reuse in
'chrome_site_per_process_browsertest.cc'. If the test location is OK I will add
alexmos@ for owner review of the test file.

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

https://codereview.chromium.org/2544223002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:474: gfx::Vector2d
offset_from_embedder = guest_window_rect_.OffsetFromOrigin();
On 2016/12/05 19:44:18, lfg wrote:
> If we are transforming guest_window_rect to the root coordinates, then this
> isn't an offset_from_embedder anymore.
> 
> Also, does this means that we were sending all events with wrong coordinates
to
> the guests?
> 
> Does this work with BrowserPlugins embedding other BrowserPlugins? For
example,
> <appview>?

> If we are transforming guest_window_rect to the root coordinates, then this
> isn't an offset_from_embedder anymore.
Correct.

> Also, does this means that we were sending all events with wrong coordinates
to
> the guests?
No since the conversion and transforms happen in BrowserPlugin on the renderer
side. But, but here the Resending coordinates would be incorrect. Thanks for
pointing out. In the patch I am not transforming the
guest window but rather change the way container bounds are calculated in
WebContentsViewGuest.

Also, both RWHVG and RWHV for tab return point when we do the transform. Since
no other guests can be in OOPIF this new code path will only affect
MimeHandlerView.

A non-OOPIF <appview> cannot be in any OOPIF. An OOPIF-<appview> might be in
OOPIF but we would not hit the modified code path.

[commit-bot: I haz the power, 2016-12-06 00:09:05]

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

[lfg, 2016-12-06 00:23:53]

Thanks, one more question.

https://codereview.chromium.org/2544223002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_guest.cc (left):

https://codereview.chromium.org/2544223002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_guest.cc:95: gfx::Point
guest_coordinates = guest_->GetScreenCoordinates(gfx::Point());
Will other uses of GetScreenCoordinates have the same problem? Should we change
GetScreenCoordinates instead?

[EhsanK, 2016-12-06 01:00:12]

Thanks! PTAL.

https://codereview.chromium.org/2544223002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_guest.cc (left):

https://codereview.chromium.org/2544223002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_guest.cc:95: gfx::Point
guest_coordinates = guest_->GetScreenCoordinates(gfx::Point());
On 2016/12/06 00:23:53, lfg wrote:
> Will other uses of GetScreenCoordinates have the same problem? Should we
change
> GetScreenCoordinates instead?

GetScreenCoordinates() and the new method added have two different
functionalities now. Screen coordinate is with respect to embedder, and the new
method with respect to embedder WebContents (root view I suppose). So it
shouldn't be like one would necessarily replace the other.

As for other applications of it, I guess they seem correct. I noticed the
following usages:
1- GetViewBounds() in RWHVG. This is correct as we convert the owner view bounds
along the |guest_window_rect_| offset.

2- DnD:  It seems incorrect to use GetScreenCoordinates() when the embedder is
an OOPIF, but I would not change it for two reasons:
  2-a There is a TODO(paulmeyer) about sending the ClientXY to
BrowserPluginEmbedder().
  2-b We don't actually support drag and drop for PDF right now (it looks
broken).

3- IME usages. It is correct since we deal with owner RWHV. But IME window
position is broken on PDF anyway (I guess selection bounds and range are not
transformed/reported properly).

So I guess for now we should keep the usages of GetScreenCoordinates() as is.

[commit-bot: I haz the power, 2016-12-06 01:05:16]

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

[commit-bot: I haz the power, 2016-12-06 01:05:17]

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_...)

[lfg, 2016-12-06 01:22:26]

Thanks, lgtm.

https://codereview.chromium.org/2544223002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_guest.cc (left):

https://codereview.chromium.org/2544223002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_guest.cc:95: gfx::Point
guest_coordinates = guest_->GetScreenCoordinates(gfx::Point());
On 2016/12/06 01:00:12, EhsanK wrote:
> On 2016/12/06 00:23:53, lfg wrote:
> > Will other uses of GetScreenCoordinates have the same problem? Should we
> change
> > GetScreenCoordinates instead?
> 
> GetScreenCoordinates() and the new method added have two different
> functionalities now. Screen coordinate is with respect to embedder, and the
new
> method with respect to embedder WebContents (root view I suppose). So it
> shouldn't be like one would necessarily replace the other.
> 
> As for other applications of it, I guess they seem correct. I noticed the
> following usages:
> 1- GetViewBounds() in RWHVG. This is correct as we convert the owner view
bounds
> along the |guest_window_rect_| offset.
> 
> 2- DnD:  It seems incorrect to use GetScreenCoordinates() when the embedder is
> an OOPIF, but I would not change it for two reasons:
>   2-a There is a TODO(paulmeyer) about sending the ClientXY to
> BrowserPluginEmbedder().
>   2-b We don't actually support drag and drop for PDF right now (it looks
> broken).
> 
> 3- IME usages. It is correct since we deal with owner RWHV. But IME window
> position is broken on PDF anyway (I guess selection bounds and range are not
> transformed/reported properly).
> 
> So I guess for now we should keep the usages of GetScreenCoordinates() as is.

Acknowledged.

[Charlie Reis, 2016-12-08 18:16:45]

Thanks, LGTM given lfg's review.  (I'm an owner of the test file as well, so you
should be set now.)

[EhsanK, 2016-12-08 18:28:16]

On 2016/12/08 18:16:45, Charlie Reis wrote:
> Thanks, LGTM given lfg's review.  (I'm an owner of the test file as well, so
you
> should be set now.)

Thanks Charlie! The test I added seems to fail on Windows (perhaps flaky). Maybe
it needs to be in interactive ui tests. I will investigate.

[EhsanK, 2016-12-08 19:06:00]

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

[commit-bot: I haz the power, 2016-12-08 19:06:48]

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

[EhsanK, 2016-12-08 20:17:18]

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

[EhsanK, 2016-12-08 20:17:19]

I moved the test to 'site_per_process_interactive_browsertest.cc' which made the
windows bot green.

adding alexmos@ review/owner approval.

PTAL.

[commit-bot: I haz the power, 2016-12-08 21:13:23]

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

[commit-bot: I haz the power, 2016-12-08 21:13:24]

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

[Charlie Reis, 2016-12-08 21:44:53]

I'm an owner of that file as well (see chrome/browser/OWNERS).

If it's just a move of the test, then this still LGTM.

[EhsanK, 2016-12-08 22:52:18]

Thanks Charlie.

Well...the test is probably racy. I need to make some changes to it and figure
out why it randomly fails on win chromium or win chromium x64. I think the race
shows itself on windows because if I remember correctly, context menu event
happens after mouse up (unlike other platforms which is mouse down). I wonder if
the race is due to the way test observes guest creation. I will have to work on
this and send a new patch.

Also I believe chromium butler extension cannot detect owners form external
owner files which is why I assumed you are not an owner of the test files.

[EhsanK, 2016-12-12 19:53:12]

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

[commit-bot: I haz the power, 2016-12-12 19:54:32]

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, 2016-12-12 20:12:52]

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

[commit-bot: I haz the power, 2016-12-12 20:12:53]

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

[EhsanK, 2016-12-13 00:13:43]

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

[commit-bot: I haz the power, 2016-12-13 00:15:09]

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

[EhsanK, 2016-12-13 01:36:27]

Charlie, PTAL.

I had to change the test to make it work on Windows. The issue was that
WaitForSingleGuestCreated() actually waits for creation of WebContents which is
before Attach. We still need the attach IPC in BrowserPluingGuest to update the
window rects so we have to wait until Navigation starts (which is after attach).

Also I noticed that waiting for navigation end is not a good idea as, I think,
it sometimes never ends in Windows.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:829: void
DidStartNavigation(content::NavigationHandle* handle) override {
DidEndNavigation will cause test failures on win_chromium_x64_rel_ng.(seems to
be timing out).

[commit-bot: I haz the power, 2016-12-13 02:21:48]

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

[commit-bot: I haz the power, 2016-12-13 02:21:49]

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

[EhsanK, 2016-12-13 17:42:55]

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

[commit-bot: I haz the power, 2016-12-13 17:43:17]

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

[Charlie Reis, 2016-12-13 19:20:49]

Sorry, this doesn't look good, assuming the context menu is supposed to run on
the extension page.

(If that's not required, maybe there's an easier way to test this without
involving extensions and waiting for a more appropriate attach event?)

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:811: // This class
observers a WebContents for a navigation to an extension scheme to
nit: observes

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:829: void
DidStartNavigation(content::NavigationHandle* handle) override {
On 2016/12/13 01:36:27, EhsanK wrote:
> DidEndNavigation will cause test failures on win_chromium_x64_rel_ng.(seems to
> be timing out).

I assume you mean DidFinishNavigation?

Anyway, that's concerning.  It's unusual to only wait for the navigation to
start, and it looks like the wrong thing to do below.  We should figure out why
the navigation never finished on Windows, or whatever is going wrong.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:867: // extension
starts. It will be used as an indicator that BrowserPluing
nit: BrowserPlugin

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:888: // complete.
This means the BrowserPlugin is attached and the
But it's not complete, given how the observer is written.  The extension page
hasn't committed yet, which means that the context menu actions below could
happen on the initial blank page in the BrowserPlugin, and thus be testing the
wrong thing.

[EhsanK, 2016-12-13 20:32:00]

Thanks Charlie for the reviews!

I have replied to you comments but the outstanding issue is figuring out why the
navigation fails (or more correctly, why the test fails when I wait for finish
navigation). I will test it in a draft CL, both for main and child frames to see
if there are any clues there. I will send an update when I can draw conclusions
about this issue.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:811: // This class
observers a WebContents for a navigation to an extension scheme to
On 2016/12/13 19:20:49, Charlie Reis wrote:
> nit: observes

Done.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:829: void
DidStartNavigation(content::NavigationHandle* handle) override {
On 2016/12/13 19:20:49, Charlie Reis wrote:
> On 2016/12/13 01:36:27, EhsanK wrote:
> > DidEndNavigation will cause test failures on win_chromium_x64_rel_ng.(seems
to
> > be timing out).
> 
> I assume you mean DidFinishNavigation?
Yes.

> Anyway, that's concerning.  It's unusual to only wait for the navigation to
> start, and it looks like the wrong thing to do below.
Hmm...apart from the issue of the potential failure in navigation, which I will
dig into more, I still think waiting for navigation start is more correct. Our
intention here is not to navigate to the extension but rather determine the
first point in time after the guest is attached. Attaching, is the first point
in time where we can rely on the position of context menu. Navigation start
happens right after attach.

>  We should figure out why
> the navigation never finished on Windows, or whatever is going wrong.
Yes. Agreed. Unfortunately it does not repro locally and only on one of the bots
so far. I will keep poking at it.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:867: // extension
starts. It will be used as an indicator that BrowserPluing
On 2016/12/13 19:20:49, Charlie Reis wrote:
> nit: BrowserPlugin

Done.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:888: // complete.
This means the BrowserPlugin is attached and the
On 2016/12/13 19:20:49, Charlie Reis wrote:
> But it's not complete, given how the observer is written.  The extension page
> hasn't committed yet, which means that the context menu actions below could
> happen on the initial blank page in the BrowserPlugin,
Sorry. 'complete' is incorrect. And we are probably sending the click to the
partially loaded extension or the empty page. 

> and thus be testing the wrong thing.
Hmm...I don't quite agree with the last point since we are only testing the
context menu position. As long as the BP is there and handles input and knows
its window rect we should be fine. WDYT?

[Charlie Reis, 2016-12-15 23:07:19]

I'm explaining my concerns a bit more below, but it sounds like maybe they're
aren't concrete problems in this case.  I could be ok proceeding with this if we
make the explanations clearer (though figuring out why the navigation doesn't
finish would be better).  :)

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:829: void
DidStartNavigation(content::NavigationHandle* handle) override {
On 2016/12/13 20:32:00, EhsanK wrote:
> On 2016/12/13 19:20:49, Charlie Reis wrote:
> > On 2016/12/13 01:36:27, EhsanK wrote:
> > > DidEndNavigation will cause test failures on
win_chromium_x64_rel_ng.(seems
> to
> > > be timing out).
> > 
> > I assume you mean DidFinishNavigation?
> Yes.
> 
> > Anyway, that's concerning.  It's unusual to only wait for the navigation to
> > start, and it looks like the wrong thing to do below.
> Hmm...apart from the issue of the potential failure in navigation, which I
will
> dig into more, I still think waiting for navigation start is more correct. Our
> intention here is not to navigate to the extension but rather determine the
> first point in time after the guest is attached. Attaching, is the first point
> in time where we can rely on the position of context menu. Navigation start
> happens right after attach.

I'm not thrilled with using navigation start as the way to listen for attach,
since those are two semantically different things.  Is there no way to directly
listen for attach?

If not, maybe we can do this, but this code should be much more explicit about
the fact that it's waiting for the guest to attach and not for the extension
page to be ready.  The current name doesn't reflect that, for example.

> 
> >  We should figure out why
> > the navigation never finished on Windows, or whatever is going wrong.
> Yes. Agreed. Unfortunately it does not repro locally and only on one of the
bots
> so far. I will keep poking at it.
> 

This would be the better way to go from my perspective, but I suppose I'm ok
with proceeding if the test really doesn't care about whether the extension page
has committed or not.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:888: // complete.
This means the BrowserPlugin is attached and the
On 2016/12/13 20:32:00, EhsanK wrote:
> On 2016/12/13 19:20:49, Charlie Reis wrote:
> > But it's not complete, given how the observer is written.  The extension
page
> > hasn't committed yet, which means that the context menu actions below could
> > happen on the initial blank page in the BrowserPlugin,
> Sorry. 'complete' is incorrect. And we are probably sending the click to the
> partially loaded extension or the empty page. 
> 
> > and thus be testing the wrong thing.
> Hmm...I don't quite agree with the last point since we are only testing the
> context menu position. As long as the BP is there and handles input and knows
> its window rect we should be fine. WDYT?

I would be worried about that leading to fragile or flaky tests, if sometimes
the context menu happens on the blank page and others on the extension page.  I
thought it might even fail if the extension commit dismissed the about:blank
context menu, but (surprisingly) that doesn't seem to happen.

Maybe it's ok for now, if we make it very clear that the test can't assume the
extension page has committed.  (I just don't want people to add things to the
test assuming it will happen on the extension page.)

[EhsanK, 2017-01-19 16:41:13]

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

[commit-bot: I haz the power, 2017-01-19 16:41:32]

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-01-19 17:40:58]

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

[commit-bot: I haz the power, 2017-01-19 17:40:59]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-01-25 18:48:50]

Thanks Charlie and sorry it took so long. Unfortunately, the test is not fixed
and we have missed the opportunity for merging to M-56 (I suppose).

I spent most of last week trying to repro this and finally could manage to make
the test fail on a specific windows 7 virtual machine. AFAIK this has nothing to
do with the context menus, nor waiting for either of Start and Finish
navigation. The way I am seeing the failure is timing out on navigating the
child frame.

I will dig into the issue and send another patch.

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:888: // complete.
This means the BrowserPlugin is attached and the
On 2016/12/15 23:07:19, Charlie Reis wrote:
> On 2016/12/13 20:32:00, EhsanK wrote:
> > On 2016/12/13 19:20:49, Charlie Reis wrote:
> > > But it's not complete, given how the observer is written.  The extension
> page
> > > hasn't committed yet, which means that the context menu actions below
could
> > > happen on the initial blank page in the BrowserPlugin,
> > Sorry. 'complete' is incorrect. And we are probably sending the click to the
> > partially loaded extension or the empty page. 
> > 
> > > and thus be testing the wrong thing.
> > Hmm...I don't quite agree with the last point since we are only testing the
> > context menu position. As long as the BP is there and handles input and
knows
> > its window rect we should be fine. WDYT?
> 
> I would be worried about that leading to fragile or flaky tests, if sometimes
> the context menu happens on the blank page and others on the extension page. 
I
> thought it might even fail if the extension commit dismissed the about:blank
> context menu, but (surprisingly) that doesn't seem to happen.
> 
> Maybe it's ok for now, if we make it very clear that the test can't assume the
> extension page has committed.  (I just don't want people to add things to the
> test assuming it will happen on the extension page.)

Coming back to this CL I am finding myself agreeing with your comment more (we
should be using an API which is meaningful).

That being said, this test has issues on Windows 7 and they are probably not
related to either of Finish and Start navigation events. It so happened that the
test was flaking for some other reason and I presumed it has to do with Finish
and Start (since it happened to flake on using finish only but that was by
chance).

I could finally repro this reliably on a VM and noticed that the test fails at
navigating the child frame. I will have to investigate this. Hopefully 1) that
is the only issue with the test, and 2) I can find the resolution soon.

[EhsanK, 2017-01-25 18:56:37]

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

[commit-bot: I haz the power, 2017-01-25 18:57:22]

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-01-25 20:23:44]

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

[commit-bot: I haz the power, 2017-01-25 20:23:45]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-01-25 20:37:19]

I made a small change in the HTML file for test and things seem OK now. I tried
the test locally and it is passing every time (gtest_repeat = 20).

The change which seems to have fixed it was to define the mime type for the
<embed>. Without it, on my VM, test times out in waiting for the guest view to
be created.

PTAL.

[Charlie Reis, 2017-01-25 21:27:02]

On 2017/01/25 20:37:19, EhsanK wrote:
> I made a small change in the HTML file for test and things seem OK now. I
tried
> the test locally and it is passing every time (gtest_repeat = 20).
> 
> The change which seems to have fixed it was to define the mime type for the
> <embed>. Without it, on my VM, test times out in waiting for the guest view to
> be created.
> 
> PTAL.

Thanks for tracking it down!  Strange that the MIME type would lead to platform
specific behavior, but I'm glad it's working now, and that we can wait for the
navigation to finish in the test.

LGTM with nits.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:812: // start.
s/start/finish/

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:830: if
(!handle->GetURL().SchemeIs(extensions::kExtensionScheme))
Let's also return if !handle->HasCommitted() or
navigation_handle->IsErrorPage(), just to make the test more robust.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:867: // extension
starts. It will be used as an indicator that BrowserPlugin
s/starts/commits/

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:876: // Send a right
click to the embedder frame and observe the context menu.
nit: Phrase this as declaring a lambda for it, since it sounds like we're
actually doing that here, but it doesn't happen until later.

Or even better, move this declaration down just below the uses, since there's no
need to declare it before doing the navigation_observer.Wait().  (It's a bit
confusing to have it up here.)

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:891:
navigation_observer.Wait();
Any reason this needs to be after the ExecuteScript on line 872?  I would expect
them to be somewhat unrelated, and that it might be clearer to put this first
(to have fewer things happening at the same time).

[EhsanK, 2017-01-26 15:46:36]

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

[commit-bot: I haz the power, 2017-01-26 15:46:46]

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

[EhsanK, 2017-01-26 15:47:03]

Thanks Charlie! PTAL.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:812: // start.
On 2017/01/25 21:27:02, Charlie Reis wrote:
> s/start/finish/

Thanks! Done.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:830: if
(!handle->GetURL().SchemeIs(extensions::kExtensionScheme))
On 2017/01/25 21:27:02, Charlie Reis wrote:
> Let's also return if !handle->HasCommitted() or
> navigation_handle->IsErrorPage(), just to make the test more robust.

Thanks! Done.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:867: // extension
starts. It will be used as an indicator that BrowserPlugin
On 2017/01/25 21:27:02, Charlie Reis wrote:
> s/starts/commits/

Thanks! Done.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:876: // Send a right
click to the embedder frame and observe the context menu.
Thanks!
> nit: Phrase this as declaring a lambda for it, since it sounds like we're
> actually doing that here, but it doesn't happen until later.
> 
Done.

> Or even better, move this declaration down just below the uses, since there's
no
> need to declare it before doing the navigation_observer.Wait().  (It's a bit
> confusing to have it up here.)

Done.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:891:
navigation_observer.Wait();
On 2017/01/25 21:27:02, Charlie Reis wrote:
> Any reason this needs to be after the ExecuteScript on line 872?  I would
expect
> them to be somewhat unrelated, and that it might be clearer to put this first
> (to have fewer things happening at the same time).
Good point and good catch actually. I tried moving navigation wait before the
script (which is changing the position of the frame). That made the test flake
since the frame was sometimes confused about its position (was assuming the
original position at (0, 0)). The way it was before was giving the frame the
chance to update its coordinates in time.

That being said, I think the right order of things to happen is to 
1) Navigate main page
2) change the position of the frame
3) navigate the frame
4) wait for extension navigation to end
5) test context menu.

This is the way the test is written now. WDYT?

[commit-bot: I haz the power, 2017-01-26 16:47:28]

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

[commit-bot: I haz the power, 2017-01-26 16:47:30]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-26 20:50:41]

Thanks!  LGTM.

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2544223002/diff/220001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:891:
navigation_observer.Wait();
On 2017/01/26 15:47:03, EhsanK wrote:
> On 2017/01/25 21:27:02, Charlie Reis wrote:
> > Any reason this needs to be after the ExecuteScript on line 872?  I would
> expect
> > them to be somewhat unrelated, and that it might be clearer to put this
first
> > (to have fewer things happening at the same time).
> Good point and good catch actually. I tried moving navigation wait before the
> script (which is changing the position of the frame). That made the test flake
> since the frame was sometimes confused about its position (was assuming the
> original position at (0, 0)). The way it was before was giving the frame the
> chance to update its coordinates in time.
> 
> That being said, I think the right order of things to happen is to 
> 1) Navigate main page
> 2) change the position of the frame
> 3) navigate the frame
> 4) wait for extension navigation to end
> 5) test context menu.
> 
> This is the way the test is written now. WDYT?

Yes, I like this new order-- thanks!  (Changing the position between navigating
the frame and waiting for the frame navigation to finish would likely have been
flaky, but doing the position change before the frame navigation sounds right.)

[EhsanK, 2017-01-26 21:10:44]

Thanks! Trying to CQ soon.

[EhsanK, 2017-01-26 21:10:51]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-01-26 21:10:51]

The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/2544223002/#ps240001
(title: "Addressing creis@'s comments + Reorganizing the test")

[commit-bot: I haz the power, 2017-01-26 21:11: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-01-26 21:17:30]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1485465051213560,
"parent_rev": "e46620f101019c220a303e6cc82c57c928a677bd", "commit_rev":
"aeddbee1c18102e37b5dc083c99a337e12afa759"}

[commit-bot: I haz the power, 2017-01-26 21:18:05]

Description was changed from

==========
Fix the position of context menu for BrowserPlugins inside OOPIF

Currently, we do not consider the displacement of OOPIF view on page when
update the geomtery of BrowserPluginGuest. This causes the context menu
offset (the displacement between WebContents's) to be wrong.

This CL will make the following changes to fix this issue:
1- The reported rect in OnUpdateGeometry() is transformed to owner view's
coordinate space.

2- We make sure BrowserPlugin does its own conversions using RenderWidget
of the local root containing the BrowserPlugin (as opposed to the RenderView
which seems wrong).

3- We make sure BrowserPlugin updates geometry when it is attached.

BUG=670433
==========

to

==========
Fix the position of context menu for BrowserPlugins inside OOPIF

Currently, we do not consider the displacement of OOPIF view on page when
update the geomtery of BrowserPluginGuest. This causes the context menu
offset (the displacement between WebContents's) to be wrong.

This CL will make the following changes to fix this issue:
1- The reported rect in OnUpdateGeometry() is transformed to owner view's
coordinate space.

2- We make sure BrowserPlugin does its own conversions using RenderWidget
of the local root containing the BrowserPlugin (as opposed to the RenderView
which seems wrong).

3- We make sure BrowserPlugin updates geometry when it is attached.

BUG=670433

Review-Url: https://codereview.chromium.org/2544223002
Cr-Commit-Position: refs/heads/master@{#446448}
Committed:
https://chromium.googlesource.com/chromium/src/+/aeddbee1c18102e37b5dc083c99a...
==========

[commit-bot: I haz the power, 2017-01-26 21:18:07]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/aeddbee1c18102e37b5dc083c99a...