Test for BrowserPlugin-based WebView Focus

Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b65ea7939693ae12a9134d9caaf33b2cc10a2f61
Cr-Commit-Position: refs/heads/master@{#404252}

[wjmaclean, 2016-06-30 20:39:41]

Description was changed from

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
==========

to

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-06-30 20:57:23]

Description was changed from

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-06-30 20:57:23]

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

[wjmaclean, 2016-06-30 20:58:21]

Test for https://codereview.chromium.org/2101663002/

I may want to iterate on this, but if you have any initial thoughts ...

[Charlie Reis, 2016-06-30 21:20:56]

Can you have someone more familiar with BrowserPlugin tests review this first? 
I'm happy to give an owners stamp afterward.

[wjmaclean, 2016-06-30 21:36:58]

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

[wjmaclean, 2016-06-30 21:37:11]

kenrb@ ?

[wjmaclean, 2016-07-01 15:17:55]

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

[commit-bot: I haz the power, 2016-07-01 15:18: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, 2016-07-01 16:28:37]

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

[commit-bot: I haz the power, 2016-07-01 16:28:38]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-07-05 14:30:57]

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:357:
frame_count_++;
Should this be reset to zero when a new surface is created? I think this could
use a comment in the header describing what it is, and it doesn't affect your
current tests in any way, but it feels like clearing it for each surface would
be more intuitive for potential future uses of this count.

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:221: unsigned
frame_count_;
I think this should be uint32_t, and the name might make it too easy to confuse
this with RenderWidgetHostViewBase::renderer_frame_number_, which is counting a
different thing. Counting the number of times the surface was drawn is different
from counting compositor frames.

https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1050: void
WaitForSurfaceReady(content::WebContents* web_contents) {
I know we talked about this earlier, but I don't remember if you looked into
whether SurfaceHitTestReadyNotifier can be moved to somewhere in
content/public/test, to make it available here and avoid having a parallel
mechanism for doing that. If it works correctly, your frame_count_ method is
simpler than how I was previously doing that, although it doesn't obviate the
need for a busy loop.

[wjmaclean, 2016-07-05 15:04:22]

See comments below, PTAL?

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:357:
frame_count_++;
On 2016/07/05 14:30:56, kenrb wrote:
> Should this be reset to zero when a new surface is created? I think this could
> use a comment in the header describing what it is, and it doesn't affect your
> current tests in any way, but it feels like clearing it for each surface would
> be more intuitive for potential future uses of this count.

Done.

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2116663002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:221: unsigned
frame_count_;
On 2016/07/05 14:30:56, kenrb wrote:
> I think this should be uint32_t, and the name might make it too easy to
confuse
> this with RenderWidgetHostViewBase::renderer_frame_number_, which is counting
a
> different thing. Counting the number of times the surface was drawn is
different
> from counting compositor frames.

Done.

https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1050: void
WaitForSurfaceReady(content::WebContents* web_contents) {
On 2016/07/05 14:30:57, kenrb wrote:
> I know we talked about this earlier, but I don't remember if you looked into
> whether SurfaceHitTestReadyNotifier can be moved to somewhere in
> content/public/test, to make it available here and avoid having a parallel
> mechanism for doing that. If it works correctly, your frame_count_ method is
> simpler than how I was previously doing that, although it doesn't obviate the
> need for a busy loop.

Moving the SurfaceHitTestReady relies on a cc DEP which is currently not
exported to all browser tests, and it looks either like (i) a lot of work to
change that, or (possibly) *impossible* to do in our current setup. So I felt
this was simpler.

We *could* add a RenderWidgetHostViewBaseObserver callback to avoid the
wait-loop, but that seemed to me like a lot of overhead on every frame draw
(though we could just notify on the first frame draw for a given surface?) ...
there's also the problem of being sure to get the observer registered before the
frame gets incremented for the first time, so it would likely lead to a racy
test. This seemed a bit simpler, if not as elegant.

[kenrb, 2016-07-05 15:32:02]

lgtm

https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1050: void
WaitForSurfaceReady(content::WebContents* web_contents) {
On 2016/07/05 15:04:22, wjmaclean wrote:
> On 2016/07/05 14:30:57, kenrb wrote:
> > I know we talked about this earlier, but I don't remember if you looked into
> > whether SurfaceHitTestReadyNotifier can be moved to somewhere in
> > content/public/test, to make it available here and avoid having a parallel
> > mechanism for doing that. If it works correctly, your frame_count_ method is
> > simpler than how I was previously doing that, although it doesn't obviate
the
> > need for a busy loop.
> 
> Moving the SurfaceHitTestReady relies on a cc DEP which is currently not
> exported to all browser tests, and it looks either like (i) a lot of work to
> change that, or (possibly) *impossible* to do in our current setup. So I felt
> this was simpler.
> 

If we change SurfaceHitTestReady to use surface_frame_count_ and work the same
as WaitForSurfaceReady, then could we get rid of the cc dependency and move it
to public? If that would work, it might be something for a later CL because we
would have to change all of the current sites where SurfaceHitTestReadyNotifier
is used.

> We *could* add a RenderWidgetHostViewBaseObserver callback to avoid the
> wait-loop, but that seemed to me like a lot of overhead on every frame draw
> (though we could just notify on the first frame draw for a given surface?) ...
> there's also the problem of being sure to get the observer registered before
the
> frame gets incremented for the first time, so it would likely lead to a racy
> test. This seemed a bit simpler, if not as elegant.

Yes I've thought about this before, and I haven't found anything very clean for
doing that.

[wjmaclean, 2016-07-05 15:39:35]

On 2016/07/05 15:32:02, kenrb wrote:
> lgtm
> 
>
https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
> File content/public/test/browser_test_utils.cc (right):
> 
>
https://codereview.chromium.org/2116663002/diff/40001/content/public/test/bro...
> content/public/test/browser_test_utils.cc:1050: void
> WaitForSurfaceReady(content::WebContents* web_contents) {
> On 2016/07/05 15:04:22, wjmaclean wrote:
> > On 2016/07/05 14:30:57, kenrb wrote:
> > > I know we talked about this earlier, but I don't remember if you looked
into
> > > whether SurfaceHitTestReadyNotifier can be moved to somewhere in
> > > content/public/test, to make it available here and avoid having a parallel
> > > mechanism for doing that. If it works correctly, your frame_count_ method
is
> > > simpler than how I was previously doing that, although it doesn't obviate
> the
> > > need for a busy loop.
> > 
> > Moving the SurfaceHitTestReady relies on a cc DEP which is currently not
> > exported to all browser tests, and it looks either like (i) a lot of work to
> > change that, or (possibly) *impossible* to do in our current setup. So I
felt
> > this was simpler.
> > 
> 
> If we change SurfaceHitTestReady to use surface_frame_count_ and work the same
> as WaitForSurfaceReady, then could we get rid of the cc dependency and move it
> to public? If that would work, it might be something for a later CL because we
> would have to change all of the current sites where
SurfaceHitTestReadyNotifier
> is used.

Possibly. WaitForSurfaceReady may not have the same guarantees as
SurfaceHitTestReady, since it doesn't do the ContainsSurfaceId() test. But if
this version works elsewhere, then we could switch to it.

[Charlie Reis, 2016-07-06 18:28:50]

Thanks.  Mainly some requests for documentation below.

https://codereview.chromium.org/2116663002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2116663002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:221: uint32_t
surface_frame_count_;
Seems mildly unfortunate to have to track this in all builds just to support
tests.  That said, I'd much rather have tests than not add this.  :)

Just thought I'd check if there's another way to do the waiting without adding
this counter.  If not, I'm ok with it.

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1059: if
(child_view->surface_frame_count() > 0)
Should this check come before the first delayed task, in case it's already true
when we call the method?

Also, it's worth having a comment somewhere saying how this condition equates to
the surface being ready.

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

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:313: // Find out if the BrowserPlugin
for a guest webcontents is focused. Returns
nit: WebContents

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:314: // false if the WebContents
doesn't have a BrowserPlugin.
"doesn't have a BrowserPlugin": I'm not sure how to read this phrase.  It could
mean an embedder WebContents that "has" one or more guest BrowserPlugins, or it
could mean that we're talking about a WebContents which is not itself a guest. 
I think it's the latter?

Maybe there's a clearer way to phrase?

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:319: // RenderWidgetHostViewAura.
Maybe mention why these two methods are useful?

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:329: void
WaitForSurfaceReady(content::WebContents* web_contents);
nit: Please comment all methods in the public interface.

[wjmaclean, 2016-07-06 19:49:31]

I've attempted to address your comments, let me know if this is better? PTAL

https://codereview.chromium.org/2116663002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2116663002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:221: uint32_t
surface_frame_count_;
On 2016/07/06 18:28:49, Charlie Reis wrote:
> Seems mildly unfortunate to have to track this in all builds just to support
> tests.  That said, I'd much rather have tests than not add this.  :)
> 
> Just thought I'd check if there's another way to do the waiting without adding
> this counter.  If not, I'm ok with it.

Agreed. kenrb@ and I haven't thought of another way, and even if it was, it
would likely be an observer or some such thing that would add at least as much
state/overhead as this.

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1059: if
(child_view->surface_frame_count() > 0)
On 2016/07/06 18:28:49, Charlie Reis wrote:
> Should this check come before the first delayed task, in case it's already
true
> when we call the method?
> 
> Also, it's worth having a comment somewhere saying how this condition equates
to
> the surface being ready.

Done.

The if-statement evloved as I worked on this, and should have been replaced by
testing in the while-loop condition when it was updated to this version.

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

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:313: // Find out if the BrowserPlugin
for a guest webcontents is focused. Returns
On 2016/07/06 18:28:50, Charlie Reis wrote:
> nit: WebContents

Done.

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:314: // false if the WebContents
doesn't have a BrowserPlugin.
On 2016/07/06 18:28:49, Charlie Reis wrote:
> "doesn't have a BrowserPlugin": I'm not sure how to read this phrase.  It
could
> mean an embedder WebContents that "has" one or more guest BrowserPlugins, or
it
> could mean that we're talking about a WebContents which is not itself a guest.

> I think it's the latter?
> 
> Maybe there's a clearer way to phrase?

Done.

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:319: // RenderWidgetHostViewAura.
On 2016/07/06 18:28:49, Charlie Reis wrote:
> Maybe mention why these two methods are useful?

Done.

https://codereview.chromium.org/2116663002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:329: void
WaitForSurfaceReady(content::WebContents* web_contents);
On 2016/07/06 18:28:49, Charlie Reis wrote:
> nit: Please comment all methods in the public interface.

Done.

Ooops, forgot to comment this one!

[Charlie Reis, 2016-07-06 20:17:13]

Ok, LGTM with one simplifying suggestion below.

https://codereview.chromium.org/2116663002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2116663002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:221: uint32_t
surface_frame_count_;
On 2016/07/06 19:49:31, wjmaclean wrote:
> On 2016/07/06 18:28:49, Charlie Reis wrote:
> > Seems mildly unfortunate to have to track this in all builds just to support
> > tests.  That said, I'd much rather have tests than not add this.  :)
> > 
> > Just thought I'd check if there's another way to do the waiting without
adding
> > this counter.  If not, I'm ok with it.
> 
> Agreed. kenrb@ and I haven't thought of another way, and even if it was, it
> would likely be an observer or some such thing that would add at least as much
> state/overhead as this.

Can we make it a bool instead of a counter?  The only use of it is to compare it
to zero, so something like has_received_surface_frame_ might be simpler.  (Or
are there plans to compare against other values soon?)

[wjmaclean, 2016-07-07 16:23:40]

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

[commit-bot: I haz the power, 2016-07-07 16:24:27]

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

[wjmaclean, 2016-07-07 16:24:44]

After removing the un-tested first time through the loop, I started seeing some
occasional failures, making me think the whole thing was racy, so I reworked the
surface ready-waiting.

PTAL

[Charlie Reis, 2016-07-07 17:22:09]

New approach LGTM.  Ken, does it look correct?

https://codereview.chromium.org/2116663002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2116663002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:355: 
nit: No need to change this file now.

https://codereview.chromium.org/2116663002/diff/100001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2116663002/diff/100001/content/public/test/br...
content/public/test/browser_test_utils.h:331: //
nit: This should just be a blank line, no comment.

[kenrb, 2016-07-07 17:24:32]

lgtm

[commit-bot: I haz the power, 2016-07-07 18:22:13]

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

[commit-bot: I haz the power, 2016-07-07 18:22:14]

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

[wjmaclean, 2016-07-07 18:23:58]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-07-07 18:24:47]

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-07-07 22:17:20]

Description was changed from

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-07 22:17:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-07 22:17:32]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 22:19:33]

Description was changed from

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Test for BrowserPlugin-based WebView Focus

This test accompanies https://codereview.chromium.org/2101663002/ but
was landed after to (1) allow the original CL to land sooner, and (2) to
facilitate merging the original CL to M52.

BUG=619906
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b65ea7939693ae12a9134d9caaf33b2cc10a2f61
Cr-Commit-Position: refs/heads/master@{#404252}
==========

[commit-bot: I haz the power, 2016-07-07 22:19:34]

Patchset 6 (id:??) landed as
https://crrev.com/b65ea7939693ae12a9134d9caaf33b2cc10a2f61
Cr-Commit-Position: refs/heads/master@{#404252}