Bring RFH/RVH unit tests closer to reality of how RF/RV are initialized

Bring RFH/RVH unit tests closer to reality of how RF/RV are initialized

BUG=357747
TEST=all unit tests continue passing
TBR=sky@chromium.org

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

[nasko, 2015-05-22 23:18:07]

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

[nasko, 2015-05-22 23:18:08]

Hey Nick,
What do you think of this as an approach to make unit tests be closer to
reality?
Thanks in advance!
Nasko

[ncarter (slow), 2015-05-26 21:44:15]

https://codereview.chromium.org/1151973005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:357: "RenderFrameDeleted(1) ->
1*: []\n"
Hooray!

https://codereview.chromium.org/1151973005/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1897: 
What's the story with the lines that were removed here? Were they there as a
hack?

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_frame_host.cc:134: //SetRenderFrameCreated(true);
Seems like debug cruft?

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_frame_host.cc:150: //SetRenderFrameCreated(true);
Same here.

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_frame_host.cc:247: InitializeRenderFrameIfNeeded();
This is very reasonable.

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_view_host.cc:250: if (main_frame)
When is this null? In RenderViewHostImpl's CreateRenderView it seems like this
call is done without checking for null.

https://codereview.chromium.org/1151973005/diff/20001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:25: static bool
g_collect_stack_traces = true;
If you've got to edit the code to turn this off, it's just as easy for the user
to comment out that part of the log message. So I'd argue we probably don't need
this global, unless we need to turn it off programmatically for other reasons
e.g. it doesn't work on all platforms.

I've envisioned that there may be a test in the future that isn't compatible
with some part of this sanity checker, and if that ever happens, we'll provide a
way to suppress the sanity checker as a whole rather than suppressing just one
feature of it like this.

https://codereview.chromium.org/1151973005/diff/20001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:53: <<
Format(render_frame_host) << trace;
This is pretty sweet!

[nasko, 2015-05-26 22:25:49]

Some fixes and responses. Now that I know this approach makes sense to you as
well, I'll rebase, look at the failing tests, and send out an update.

https://codereview.chromium.org/1151973005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:357: "RenderFrameDeleted(1) ->
1*: []\n"
On 2015/05/26 21:44:14, ncarter wrote:
> Hooray!

Yay!

https://codereview.chromium.org/1151973005/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1897: 
On 2015/05/26 21:44:14, ncarter wrote:
> What's the story with the lines that were removed here? Were they there as a
> hack?

This might belong to a separate CL. It is incorrect assumption on the part of
the unit test. When we do a GoBack() while an interstitial page is showing, we
dismiss the interstitial, but we drop the navigation. Therefore the
PrepareForCommit and SendNavigate are not needed and test the wrong thing. Now
that I've modified the unit tests to be closer to reality, I've found this as a
failure and after chatting with Charlie, this is the right fix for it.

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_frame_host.cc:134: //SetRenderFrameCreated(true);
On 2015/05/26 21:44:14, ncarter wrote:
> Seems like debug cruft?

Yes, indeed.

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_frame_host.cc:150: //SetRenderFrameCreated(true);
On 2015/05/26 21:44:14, ncarter wrote:
> Same here.

Done.

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_frame_host.cc:247: InitializeRenderFrameIfNeeded();
On 2015/05/26 21:44:14, ncarter wrote:
> This is very reasonable.

Awesome!

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/test/test_rende...
content/test/test_render_view_host.cc:250: if (main_frame)
On 2015/05/26 21:44:14, ncarter wrote:
> When is this null? In RenderViewHostImpl's CreateRenderView it seems like this
> call is done without checking for null.

This is in preparation for removing swapped out. When we want to create "swapped
out" object, in the next iteration we will be creating just a
RenderFrameProxyHost and the RenderViewHost will not have an associated main
frame.

https://codereview.chromium.org/1151973005/diff/20001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1151973005/diff/20001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:25: static bool
g_collect_stack_traces = true;
On 2015/05/26 21:44:14, ncarter wrote:
> If you've got to edit the code to turn this off, it's just as easy for the
user
> to comment out that part of the log message. So I'd argue we probably don't
need
> this global, unless we need to turn it off programmatically for other reasons
> e.g. it doesn't work on all platforms.
> 
> I've envisioned that there may be a test in the future that isn't compatible
> with some part of this sanity checker, and if that ever happens, we'll provide
a
> way to suppress the sanity checker as a whole rather than suppressing just one
> feature of it like this.

As we chatted, I'll land this in a separate CL and make it based on a command
line flag.

https://codereview.chromium.org/1151973005/diff/20001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:53: <<
Format(render_frame_host) << trace;
On 2015/05/26 21:44:14, ncarter wrote:
> This is pretty sweet!

It indeed is very helpful in tracking down issues. I will land the full support
(including deletions tracking) in a separate CL.

[ncarter (slow), 2015-05-27 20:57:22]

lgtm

Took another pass, no additional comments.

[nasko, 2015-05-28 21:44:04]

nasko@chromium.org changed reviewers:
+ bengr@chromium.org, sky@chromium.org

[nasko, 2015-05-28 21:44:04]

Adding OWNERS reviewers:

sky@ - chrome/browser/safe_browsing/
bengr@ - components/data_reduction_proxy/content/browser/

[bengr, 2015-05-28 21:58:22]

data_reduction_proxy lgtm

[ncarter (slow), 2015-05-29 00:01:19]

https://codereview.chromium.org/1151973005/diff/130001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1151973005/diff/130001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:654: is_live =
render_view_host_->IsRenderViewLive();
If we're preserving the behavior of this function only for <webview>, does this
need to consider !GetParent() too?

[nasko, 2015-05-29 00:03:20]

https://codereview.chromium.org/1151973005/diff/130001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1151973005/diff/130001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:654: is_live =
render_view_host_->IsRenderViewLive();
On 2015/05/29 00:01:18, ncarter wrote:
> If we're preserving the behavior of this function only for <webview>, does
this
> need to consider !GetParent() too?

Not really. We don't support any cross-process navigations in guests, so the
RenderViewHost is one for all frames. Any frame in such a process should key off
of the RenderViewHost.

[nasko, 2015-05-29 00:32:14]

TBRing sky@ because of simple/mostly mechanical change.

[nasko, 2015-05-29 00:32:25]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-05-29 00:32:25]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1151973005/#ps130001
(title: "Add workaround to <webview> bug (492830)")

[commit-bot: I haz the power, 2015-05-29 00:34:21]

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

[commit-bot: I haz the power, 2015-05-29 00:38:31]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-05-29 00:39:14]

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

[Nico, 2015-05-30 07:03:09]

A revert of this CL (patchset #8 id:130001) has been created in
https://codereview.chromium.org/1146403009/ by thakis@chromium.org.

The reason for reverting is: Speculative, might have caused
http://crbug.com/493584 (RlzLibTests failing in unit_tests in official builds).