Fix flakyness in NavigateRemoteFrame browsertest.

Fix flakyness in NavigateRemoteFrame browsertest.

Fix a race where RenderFrameProxy::OnCompositorFrameSwapped message can arrive after the RemoteFrame was swapped with a LocalFrame.

Enabled NavigateRemoteFrame tests, merged NavigateRemoteToDataURL and NavigateRemoteToBlankURL into NavigateRemoteFrameToBlankAndDataURLs.

BUG=446575
Committed: https://crrev.com/f52ea14b61ebdf8ad18814da5166a2b30b7a3f66
Cr-Commit-Position: refs/heads/master@{#319567}

[lfg, 2015-02-10 23:40:08]

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

[lfg, 2015-02-10 23:40:09]

Please, take a look.

Thanks!

[nasko, 2015-02-11 14:44:06]

Couple of minor comments.

https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:339: // Load cross-site page
into iframe.
Why do we need this here? These tests belongs logically in a separate test
cases, as you had them before. In addition, splitting it decreases the
probability the test case will be flaky, as it does less things, therefore on
slow bots it will complete faster (less likely to timeout).

https://codereview.chromium.org/881413005/diff/20001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/881413005/diff/20001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:199: // can happen when swapping a
RemoteFrame with a LocalFrame, where this
nit: WebRemoteFrame and WebLocalFrame

[lfg, 2015-02-11 18:23:53]

https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:339: // Load cross-site page
into iframe.
On 2015/02/11 14:44:06, nasko wrote:
> Why do we need this here? These tests belongs logically in a separate test
> cases, as you had them before. In addition, splitting it decreases the
> probability the test case will be flaky, as it does less things, therefore on
> slow bots it will complete faster (less likely to timeout).

The reason I did it is because they basically test similar things (navigating a
remote frame) and having a single test is overall faster than having 3 separate
tests.

I can split it again if you think this is the best approach.

[nasko, 2015-02-11 18:27:46]

On 2015/02/11 18:23:53, lfg wrote:
>
https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
> content/browser/site_per_process_browsertest.cc:339: // Load cross-site page
> into iframe.
> On 2015/02/11 14:44:06, nasko wrote:
> > Why do we need this here? These tests belongs logically in a separate test
> > cases, as you had them before. In addition, splitting it decreases the
> > probability the test case will be flaky, as it does less things, therefore
on
> > slow bots it will complete faster (less likely to timeout).
> 
> The reason I did it is because they basically test similar things (navigating
a
> remote frame) and having a single test is overall faster than having 3
separate
> tests.
> 
> I can split it again if you think this is the best approach.

Yes, they do test similar things, but they take longer to execute, which allow
slow bots to timeout on them. That results in flaky tests and gets them
disabled. 
Single test is not necessarily faster than 3 tests and this type of assumption
shouldn't be used when writing tests. Let's do the right thing from engineering
perspective and let the infrastructure scaling take care of the rest.

[lfg, 2015-02-11 20:20:23]

On 2015/02/11 18:27:46, nasko wrote:
> On 2015/02/11 18:23:53, lfg wrote:
> >
>
https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
> > File content/browser/site_per_process_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/881413005/diff/20001/content/browser/site_per...
> > content/browser/site_per_process_browsertest.cc:339: // Load cross-site page
> > into iframe.
> > On 2015/02/11 14:44:06, nasko wrote:
> > > Why do we need this here? These tests belongs logically in a separate test
> > > cases, as you had them before. In addition, splitting it decreases the
> > > probability the test case will be flaky, as it does less things, therefore
> on
> > > slow bots it will complete faster (less likely to timeout).
> > 
> > The reason I did it is because they basically test similar things
(navigating
> a
> > remote frame) and having a single test is overall faster than having 3
> separate
> > tests.
> > 
> > I can split it again if you think this is the best approach.
> 
> Yes, they do test similar things, but they take longer to execute, which allow
> slow bots to timeout on them. That results in flaky tests and gets them
> disabled. 
> Single test is not necessarily faster than 3 tests and this type of assumption
> shouldn't be used when writing tests. Let's do the right thing from
engineering
> perspective and let the infrastructure scaling take care of the rest.

Done, please take a look!

[nasko, 2015-02-11 21:19:49]

LGTM, provided the remote frame test starts passing.

[lfg, 2015-03-06 20:45:35]

Hi nasko, I think we are ready to land this. There are no more issues that
prevents the tests to pass, and the asan bots are also green.

Since it's been a while, please take another look before I add it to the CQ.

[nasko, 2015-03-07 00:24:50]

Still LGTM

[lfg, 2015-03-07 18:35:11]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-03-07 18:36:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/881413005/60001

[commit-bot: I haz the power, 2015-03-07 23:03:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-07 23:04:25]

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