Create an interactive site-per-process browsertest class and move DocumentHasFocus there.

Create an interactive site-per-process browsertest class and move DocumentHasFocus there.

This test was disabled due to being flaky, most likely due to
interference to its focus manipulations.  Moving this test to
interactive_ui_tests should help with that flakiness.  A few other
focus tests will also be moved to the new
SitePerProcessInteractiveBrowserTest, and it can also eventually host
OOPIF fullscreen tests.

BUG=559273, 550497
Committed: https://crrev.com/af3efeb530b370f38f81627712c15a5483761f10
Cr-Commit-Position: refs/heads/master@{#386605}

[alexmos, 2016-04-01 23:19:43]

Description was changed from

==========
Create an interactive site-per-process browsertest class and move
DocumentHasFocus there.

This test was disabled due to being flaky, most likely due to
interference to its focus manipulations.  Moving this test to
interactive_ui_tests should help with that flakiness.  A few other
focus tests will also be moved to the new
SitePerProcessInteractiveBrowserTest, and it can also eventually host
OOPIF fullscreen tests.

BUG=559273
==========

to

==========
Create an interactive site-per-process browsertest class and move
DocumentHasFocus there.

This test was disabled due to being flaky, most likely due to
interference to its focus manipulations.  Moving this test to
interactive_ui_tests should help with that flakiness.  A few other
focus tests will also be moved to the new
SitePerProcessInteractiveBrowserTest, and it can also eventually host
OOPIF fullscreen tests.

BUG=559273, 550497
==========

[alexmos, 2016-04-05 16:16:55]

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

[alexmos, 2016-04-05 16:16:56]

Hey Nasko, can you please take a look?  This is the first CL to move a few focus
tests to interactive_ui_tests, which will hopefully help with flakiness issues. 
The other tests I plan on moving (in subsequent CLs) are the ones in issues
596508 and 599730.  This test class is also where I plan on placing OOPIF
fullscreen tests.

I exposed cross-site-iframe-factory.html to the test similarly to what
SiteDetailsBrowserTest did.  To make this work on swarming bots, I also had to
add a runtime dependency on content/test/data to interactive_ui_tests, mirroring
what browser_tests does today.

https://codereview.chromium.org/1852903002/diff/60001/chrome/browser/site_per...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/1852903002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:64:
EXPECT_NE(child1->GetSiteInstance(), grandchild->GetSiteInstance());
Note that the old test used DepictFrameTree, which isn't available in chrome/. 
I replaced that with these checks, which is all the test really cares about.

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

https://codereview.chromium.org/1852903002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:240: RenderFrameHost*
ChildFrameAt(RenderFrameHost* frame, size_t index);
I thought this might be useful more generally in place of FTN's child_at() for
chrome/ tests, but I could also make this a helper inside
site_per_process_interactive_browsertest.cc.

[nasko, 2016-04-08 18:29:20]

LGTM with a couple of non-blocking comments.

https://codereview.chromium.org/1852903002/diff/60001/chrome/browser/site_per...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/1852903002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:64:
EXPECT_NE(child1->GetSiteInstance(), grandchild->GetSiteInstance());
On 2016/04/05 16:16:56, alexmos (OOO until Apr 11) wrote:
> Note that the old test used DepictFrameTree, which isn't available in chrome/.

> I replaced that with these checks, which is all the test really cares about.

We could make DepictFrameTree content/ public test method, which takes
WebContents as input. Might be a good idea, I'd suggest pinging nick@ about it.

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

https://codereview.chromium.org/1852903002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:240: RenderFrameHost*
ChildFrameAt(RenderFrameHost* frame, size_t index);
On 2016/04/05 16:16:56, alexmos (OOO until Apr 11) wrote:
> I thought this might be useful more generally in place of FTN's child_at() for
> chrome/ tests, but I could also make this a helper inside
> site_per_process_interactive_browsertest.cc.

How would that work without access to the FrameTreeNodes though? If it can work,
let's put it in the interactive tests file for now. We can put it back here if
we have more usage in the future.

[alexmos, 2016-04-11 18:37:29]

https://codereview.chromium.org/1852903002/diff/60001/chrome/browser/site_per...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/1852903002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:64:
EXPECT_NE(child1->GetSiteInstance(), grandchild->GetSiteInstance());
On 2016/04/08 18:29:20, nasko (very slow Apr 8-18) wrote:
> On 2016/04/05 16:16:56, alexmos (OOO until Apr 11) wrote:
> > Note that the old test used DepictFrameTree, which isn't available in
chrome/.
> 
> > I replaced that with these checks, which is all the test really cares about.
> 
> We could make DepictFrameTree content/ public test method, which takes
> WebContents as input. Might be a good idea, I'd suggest pinging nick@ about
it.

Will do.   I'll go ahead without it here, and if we decide to do it, I'll update
this in a separate CL.

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

https://codereview.chromium.org/1852903002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:240: RenderFrameHost*
ChildFrameAt(RenderFrameHost* frame, size_t index);
On 2016/04/08 18:29:20, nasko (very slow Apr 8-18) wrote:
> On 2016/04/05 16:16:56, alexmos (OOO until Apr 11) wrote:
> > I thought this might be useful more generally in place of FTN's child_at()
for
> > chrome/ tests, but I could also make this a helper inside
> > site_per_process_interactive_browsertest.cc.
> 
> How would that work without access to the FrameTreeNodes though? If it can
work,
> let's put it in the interactive tests file for now. We can put it back here if
> we have more usage in the future.

Actually, sorry, I misspoke -- I can't put it in
site_per_process_interactive_browsertest.cc, since that doesn't have access to
FrameTreeNodes.  (If I could, there would be no need for this helper. :) )  On
the other hand, browser_test_utils.cc does have access to content internal
stuff.

This is just a shorthand for getting access to subframe RFH via FTNs as opposed
to having to search for subframes by name or URL using FrameMatchingPredicate in
chrome/ tests -- for example, see OriginReplicationAllowsAccessToStorage, which
I've also now converted to use this helper.  (So with this and the second CL, it
will be used by three tests.)  Are you ok keeping it in browser_test_utils.cc?

[alexmos, 2016-04-11 19:49:00]

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

[alexmos, 2016-04-11 19:49:00]

thestig@: can you please review chrome/ for OWNERS?

[Lei Zhang, 2016-04-11 21:26:50]

lgtm

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

https://codereview.chromium.org/1852903002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:67: auto
document_has_focus = [](content::RenderFrameHost* rfh) {
Why not just write a regular function in an anonymous namespace above?

Also, return bool instead of auto? It's easier to read and no harder to write.

https://codereview.chromium.org/1852903002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:68: bool hasFocus =
false;
nit: has_focus;

[nasko, 2016-04-11 21:56:45]

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

https://codereview.chromium.org/1852903002/diff/60001/content/public/test/bro...
content/public/test/browser_test_utils.h:240: RenderFrameHost*
ChildFrameAt(RenderFrameHost* frame, size_t index);
On 2016/04/11 18:37:29, alexmos wrote:
> On 2016/04/08 18:29:20, nasko (very slow Apr 8-18) wrote:
> > On 2016/04/05 16:16:56, alexmos (OOO until Apr 11) wrote:
> > > I thought this might be useful more generally in place of FTN's child_at()
> for
> > > chrome/ tests, but I could also make this a helper inside
> > > site_per_process_interactive_browsertest.cc.
> > 
> > How would that work without access to the FrameTreeNodes though? If it can
> work,
> > let's put it in the interactive tests file for now. We can put it back here
if
> > we have more usage in the future.
> 
> Actually, sorry, I misspoke -- I can't put it in
> site_per_process_interactive_browsertest.cc, since that doesn't have access to
> FrameTreeNodes.  (If I could, there would be no need for this helper. :) )  On
> the other hand, browser_test_utils.cc does have access to content internal
> stuff.
> 
> This is just a shorthand for getting access to subframe RFH via FTNs as
opposed
> to having to search for subframes by name or URL using FrameMatchingPredicate
in
> chrome/ tests -- for example, see OriginReplicationAllowsAccessToStorage,
which
> I've also now converted to use this helper.  (So with this and the second CL,
it
> will be used by three tests.)  Are you ok keeping it in browser_test_utils.cc?

I find the matching by name is a good way to avoid exposing more public APIs
outside of content/, but I think it is fine for this case, since it is test only
code.

[alexmos, 2016-04-12 00:47:36]

Thanks for the quick review!

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

https://codereview.chromium.org/1852903002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:67: auto
document_has_focus = [](content::RenderFrameHost* rfh) {
On 2016/04/11 21:26:50, Lei Zhang wrote:
> Why not just write a regular function in an anonymous namespace above?

This was carried over from the old test -- the helper isn't going to be used in
any other tests, even in this file, so it seemed to me that this way makes the
test easier to follow.  But I'm fine either way.
 
> Also, return bool instead of auto? It's easier to read and no harder to write.

Done - I've added the explicit bool return type. (I'm assuming this was what you
meant, since the "auto" was for the lambda type?)

https://codereview.chromium.org/1852903002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:68: bool hasFocus =
false;
On 2016/04/11 21:26:49, Lei Zhang wrote:
> nit: has_focus;

Done.

[Lei Zhang, 2016-04-12 01:15:57]

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

https://codereview.chromium.org/1852903002/diff/100001/chrome/browser/site_pe...
chrome/browser/site_per_process_interactive_browsertest.cc:67: auto
document_has_focus = [](content::RenderFrameHost* rfh) {
On 2016/04/12 00:47:36, alexmos wrote:
> On 2016/04/11 21:26:50, Lei Zhang wrote:
> > Why not just write a regular function in an anonymous namespace above?
> 
> This was carried over from the old test -- the helper isn't going to be used
in
> any other tests, even in this file, so it seemed to me that this way makes the
> test easier to follow.  But I'm fine either way.

Ok, especially if you know it's not going to be used in any other tests.

> > Also, return bool instead of auto? It's easier to read and no harder to
write.
> 
> Done - I've added the explicit bool return type. (I'm assuming this was what
you
> meant, since the "auto" was for the lambda type?)

Oh, I'm just having a brain fart. Ignore.

[alexmos, 2016-04-12 01:19:50]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-04-12 01:19:51]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1852903002/#ps120001
(title: "thestig's comments")

[commit-bot: I haz the power, 2016-04-12 01:20:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1852903002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1852903002/120001

[commit-bot: I haz the power, 2016-04-12 03:15:00]

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

[commit-bot: I haz the power, 2016-04-12 03:15:01]

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

[alexmos, 2016-04-12 06:55:53]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-04-12 06:56:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1852903002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1852903002/120001

[commit-bot: I haz the power, 2016-04-12 07:17:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-12 07:19:19]

Description was changed from

==========
Create an interactive site-per-process browsertest class and move
DocumentHasFocus there.

This test was disabled due to being flaky, most likely due to
interference to its focus manipulations.  Moving this test to
interactive_ui_tests should help with that flakiness.  A few other
focus tests will also be moved to the new
SitePerProcessInteractiveBrowserTest, and it can also eventually host
OOPIF fullscreen tests.

BUG=559273, 550497
==========

to

==========
Create an interactive site-per-process browsertest class and move
DocumentHasFocus there.

This test was disabled due to being flaky, most likely due to
interference to its focus manipulations.  Moving this test to
interactive_ui_tests should help with that flakiness.  A few other
focus tests will also be moved to the new
SitePerProcessInteractiveBrowserTest, and it can also eventually host
OOPIF fullscreen tests.

BUG=559273, 550497

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

[commit-bot: I haz the power, 2016-04-12 07:19:20]

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