Simplify MetricsWebContentsObserver::OnTimingUpdated

Simplify MetricsWebContentsObserver::OnTimingUpdated

The check for whether a frame has navigated away is a bit more complex
than needed and uses RenderViewHost, which has been deprecated for a
while. This CL moves the child frame check first, which allows the code
to perform a simpler check using the WebContents::GetMainFrame().

BUG=

Review-Url: https://codereview.chromium.org/2795283003
Cr-Commit-Position: refs/heads/master@{#461875}
Committed: https://chromium.googlesource.com/chromium/src/+/0735c4618fe9c05775cf55be073f7fcb4c2e75ac

[nasko, 2017-04-04 19:54:21]

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

[nasko, 2017-04-04 19:54:21]

Hey Bryan,
You asked me in the past about this and I noticed it while reading the page
metrics code to understand it better (to drill into PlzNavigate perf). I hope
this cleanup helps a bit.
Thanks in advance!
Nasko

[nasko, 2017-04-04 19:54:31]

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

[commit-bot: I haz the power, 2017-04-04 19:55:10]

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

[Charlie Harrison, 2017-04-04 20:06:09]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2017-04-04 20:06:09]

drive-by LGTM

[Bryan McQuade, 2017-04-04 20:59:32]

LGTM, thanks!

[commit-bot: I haz the power, 2017-04-04 21:13:10]

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

[commit-bot: I haz the power, 2017-04-04 21:13:11]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-04-04 22:18:14]

https://codereview.chromium.org/2795283003/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2795283003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:528: if
(render_frame_host->GetParent() != nullptr) {
Dumping just a bit of context from chat with csharrison@. The check as it is can
receive data from a case that is a bit strange and I'm not sure how or whether
it has impact on these metrics. The case is:

* Main frame starts navigation and commits.
* Subframe is navigated and commits in RFH_1, but does not complete the load.
* Same subframe navigates again cross-process (possible with out-of-process
iframes) and commits in RFH_2, causing the old RFH_1 to be cleaned up.
* As part of its cleanup, RFH_1 sends timing data from the renderer.

In this scenario, the code before would've ignored that IPC, since the for the
subframe the rfh->GetRenderViewHost()->GetMainFrame() will return nullptr. That
is obviously different than the WebContents::GetMainFrame(), so it would have
returned early.

With the new code, the IPC sent during RFH_1 cleanup (if such an IPC is
possibile) will actually be processed here.

csharrison@ indicated in chat that this is fine and the CL can be committed as
is, but I'm documenting it for historical record.

[nasko, 2017-04-04 22:19:31]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-04-04 22:20: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, 2017-04-04 22:42:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491344371834850, "parent_rev":
"13a18597e1078a96d032601556e299fa7c35d691", "commit_rev":
"0735c4618fe9c05775cf55be073f7fcb4c2e75ac"}

[commit-bot: I haz the power, 2017-04-04 22:43:32]

Description was changed from

==========
Simplify MetricsWebContentsObserver::OnTimingUpdated

The check for whether a frame has navigated away is a bit more complex
than needed and uses RenderViewHost, which has been deprecated for a
while. This CL moves the child frame check first, which allows the code
to perform a simpler check using the WebContents::GetMainFrame().

BUG=
==========

to

==========
Simplify MetricsWebContentsObserver::OnTimingUpdated

The check for whether a frame has navigated away is a bit more complex
than needed and uses RenderViewHost, which has been deprecated for a
while. This CL moves the child frame check first, which allows the code
to perform a simpler check using the WebContents::GetMainFrame().

BUG=

Review-Url: https://codereview.chromium.org/2795283003
Cr-Commit-Position: refs/heads/master@{#461875}
Committed:
https://chromium.googlesource.com/chromium/src/+/0735c4618fe9c05775cf55be073f...
==========

[commit-bot: I haz the power, 2017-04-04 22:43:33]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0735c4618fe9c05775cf55be073f...

[Bryan McQuade, 2017-04-05 13:02:25]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2802693002/ by bmcquade@chromium.org.

The reason for reverting is: Address bug 708468.

[Bryan McQuade, 2017-04-06 18:46:34]

https://codereview.chromium.org/2795283003/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2795283003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:528: if
(render_frame_host->GetParent() != nullptr) {
On 2017/04/04 at 22:18:14, nasko (out until Apr 17th) wrote:
> Dumping just a bit of context from chat with csharrison@. The check as it is
can receive data from a case that is a bit strange and I'm not sure how or
whether it has impact on these metrics. The case is:
> 
> * Main frame starts navigation and commits.
> * Subframe is navigated and commits in RFH_1, but does not complete the load.
> * Same subframe navigates again cross-process (possible with out-of-process
iframes) and commits in RFH_2, causing the old RFH_1 to be cleaned up.
> * As part of its cleanup, RFH_1 sends timing data from the renderer.
> 
> In this scenario, the code before would've ignored that IPC, since the for the
subframe the rfh->GetRenderViewHost()->GetMainFrame() will return nullptr. That
is obviously different than the WebContents::GetMainFrame(), so it would have
returned early.
> 
> With the new code, the IPC sent during RFH_1 cleanup (if such an IPC is
possibile) will actually be processed here.
> 
> csharrison@ indicated in chat that this is fine and the CL can be committed as
is, but I'm documenting it for historical record.

Thanks for this. It's helpful to know.

I'm assuming that, in addition to RFH_1 having
rfh->GetRenderViewHost()->GetMainFrame() returning nullptr, traversing the
parent chain of RFH_1 to find the topmost frame would similarly not find the
main frame's RFH because RFH_1 has been removed from the frame hierarchy. Is
that right?