[page_load_metrics] use data from WebContents::GetOpener

This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

This patch was split from the previous CL here:
https://codereview.chromium.org/1901303004/

BUG=605259
Committed: https://crrev.com/ff3dd0217b152227db56a7d0731b431873ce88eb
Cr-Commit-Position: refs/heads/master@{#389893}

[Charlie Harrison, 2016-04-26 18:27:42]

Description was changed from

==========
Opener change

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

to

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

BUG=605259
==========

[Charlie Harrison, 2016-04-26 18:27:42]

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

[Charlie Harrison, 2016-04-26 18:28:23]

Description was changed from

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

BUG=605259
==========

to

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

This patch was split from the previous CL here:
https://codereview.chromium.org/1901303004/

BUG=605259
==========

[Charlie Harrison, 2016-04-26 18:33:57]

Here's the second patch. I confirmed that the opener is non null for the open in
new tab case.

I used dev tools to simulate a Nexus 5X and confirmed that the a tags used the
correct href with a ping. I then manually inserted "target='_blank'" and
confirmed an abort logged the FromGWS2 variant abort metric.

[Bryan McQuade, 2016-04-26 18:36:39]

Basically looks good, thanks! I'm inclined to track whether any navs have been
initiated rather than whether a nav has committed (see comments for details).
Let me know if it's important to track commit instead though. Can you also
verify that this does what we expect for the search results case where results
are opened in a new window?

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:528: const
GURL currently_committed_url =
do you want this to be const GURL or const GURL& ?

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.h:298: bool
has_commit_;
any reason not to instead track whether at least one navigation has been
initiated instead (in DidStartNavigation rather than DidFinishNavigation)? if
the first nav fails to commit and then the user issues a second nav (e.g. types
a URL in the URL bar) we would not want to track that second nav as being
associated with the opener iiuc.

[Charlie Harrison, 2016-04-26 19:02:41]

I verified behavior in as described in the previous message.

I've also updated to ignore about:blank, though I can't seem to reproduce
about:blanks coming through WebContentsObserver navigation api as is.

In any case, I think it's helpful because iiuc, about:blank is navigated
internally before the real navigation is made.

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:528: const
GURL currently_committed_url =
On 2016/04/26 18:36:39, Bryan McQuade wrote:
> do you want this to be const GURL or const GURL& ?

const GURL&. Good catch.

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1924543002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.h:298: bool
has_commit_;
On 2016/04/26 18:36:39, Bryan McQuade wrote:
> any reason not to instead track whether at least one navigation has been
> initiated instead (in DidStartNavigation rather than DidFinishNavigation)? if
> the first nav fails to commit and then the user issues a second nav (e.g.
types
> a URL in the URL bar) we would not want to track that second nav as being
> associated with the opener iiuc.

Hm yeah I think you're right. We only want to use the opener for the last
committed load once (in OnStart).

[Bryan McQuade, 2016-04-26 19:43:31]

LGTM. Thanks for this!

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:501: if
(navigation_handle->GetURL().spec().compare(url::kAboutBlankURL) == 0)
seems like a good addition, thanks!

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:519: //
Pass in the last committed url to the PageLoadTracker. This is used for
let's leave out the 'this is used for FromGWS metrics' since other observers may
start using it at which point the comment becomes a bit stale. we're making this
a first class part of the observer API so I think we should assume that any
observer may start using this data.

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:530: const
GURL currently_committed_url =
oh, looks like this might still need to be updated to const GURL&

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:297: // Has
the MWCO observed at least one navigation commit?
'navigation commit' -> 'navigation'

[Charlie Harrison, 2016-04-26 20:37:30]

Thanks!

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:519: //
Pass in the last committed url to the PageLoadTracker. This is used for
On 2016/04/26 19:43:31, Bryan McQuade wrote:
> let's leave out the 'this is used for FromGWS metrics' since other observers
may
> start using it at which point the comment becomes a bit stale. we're making
this
> a first class part of the observer API so I think we should assume that any
> observer may start using this data.

Done.

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:530: const
GURL currently_committed_url =
On 2016/04/26 19:43:31, Bryan McQuade wrote:
> oh, looks like this might still need to be updated to const GURL&

Done. Sorry.

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1924543002/diff/40001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:297: // Has
the MWCO observed at least one navigation commit?
On 2016/04/26 19:43:31, Bryan McQuade wrote:
> 'navigation commit' -> 'navigation'

Done.

[Charlie Harrison, 2016-04-26 20:37:35]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-04-26 20:37:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/1924543002/#ps60001
(title: "final review")

[commit-bot: I haz the power, 2016-04-26 20:37:49]

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

[commit-bot: I haz the power, 2016-04-26 21:27:31]

Description was changed from

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

This patch was split from the previous CL here:
https://codereview.chromium.org/1901303004/

BUG=605259
==========

to

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

This patch was split from the previous CL here:
https://codereview.chromium.org/1901303004/

BUG=605259
==========

[commit-bot: I haz the power, 2016-04-26 21:27:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-26 21:29:30]

Description was changed from

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

This patch was split from the previous CL here:
https://codereview.chromium.org/1901303004/

BUG=605259
==========

to

==========
This patch uses the WebContents' opener to extract the previous committed url
for the very first navigation within the MetricsWebContentsObserver.

This is used for the FromGWS abort metrics.

This patch was split from the previous CL here:
https://codereview.chromium.org/1901303004/

BUG=605259

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

[commit-bot: I haz the power, 2016-04-26 21:29:31]

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