[ Don't commit ] Add FromGWS variants to the AbortTiming metrics

Add FromGWS variants to the AbortTiming metrics

BUG=605259

[Charlie Harrison, 2016-04-21 20:04:28]

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

[Charlie Harrison, 2016-04-21 20:04:30]

PTAL. I ended up doing a couple tweaks after a lot of testing.

I'm also having trouble replicating the Close behavior on a local Android
device, but we may have to ship with that bug.

[Charlie Harrison, 2016-04-21 20:05:06]

That is, manual testing. I can add unit tests to this tomorrow.

[Bryan McQuade, 2016-04-21 20:50:41]

thanks! I haven't made it all the way through but i left some initial comments.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:381:
if (web_contents->GetOpener()) {
one concern i have is if the current tab doesn't have a committed url we will
fall back to the opener even if other navigations have occurred in this tab,
which doesn't seem like what we want.

an alternative would be to treat the last committed url of the opener as the
previously committed url for the first PageLoadTracker instantiated in each
MetricsWebContentsObserver (the first navigation in the tab), in cases where
there is an opener. so we'd basically be moving this logic into MWCO. this seems
conceptually consistent with the intent of providing a previously committed url
and reduces the complexity of PLMObservers. we might need to update the comments
for previously_committed_url slightly to explain this change in logic. the only
drawback here is that observers can no longer distinguish between these cases,
but i'm not sure i expect that they'll have a need to. what do you think?

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:406:
internal::NavigationFromGWSType navigation_type =
if navigation_type ends up returning NavigationNotFromGWS can we just return
early and avoid doing any of the work below?

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:414:
// background is often slightly ahead of the signal for close.
interesting - is this due to recording some timings in the render process and
some in the browser process, or is there just an inherent race here?

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:417:
bool foregrounded =
can we factor this bit of logic into a static helper method, something like

bool WasAbortedInForeground(...) {
if (time_to_abort.is_zero())
  return false;
if (abort_type == ABORT_NONE)
  return false;
if (WasStartedInForegroundEventInForeground(time_to_abort, extra_info)
  return true;
DCHECK(time_to_abort > extra_info.first_background_time);
// Consider this a foregrounded abort if it occurred within 100ms of...
TimeDelta bg_abort_delta = time_to_abort - extra_info.first_background_time;
if (bg_abort_delta.InMillisecondsF() <= 100)
  return true;
return false;
}

something like that anyway.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:461:
return internal::NavigationNotFromGWS;
this ends up implementing the policy we want but it doesn't feel like this logic
belongs in this method. can we instead check IsUrlFromGWS() at the call site to
GetNavigationFromGWSType and if that method returns true there, don't even
continue past that point?

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:31:
NavigationNotFromGWS,
let's put this first to make it the default / none / 0 case of the enum

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:68:
DCHECK(!timing.navigation_start.is_null());
yikes, is this a bug fix? good catch.

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:32: // Send
an initial IPC relatively early to help track aborts.
do we anticipate it's likely that we'll batch up other sends in the first 50ms?
if it seems unlikely i think just keeping this simple and calling SendNow
directly seems reasonable.

[Charlie Harrison, 2016-04-21 21:09:24]

Thanks for the prompt review. Mostly ACKed your comments.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:381:
if (web_contents->GetOpener()) {
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> one concern i have is if the current tab doesn't have a committed url we will
> fall back to the opener even if other navigations have occurred in this tab,
> which doesn't seem like what we want.
> 
> an alternative would be to treat the last committed url of the opener as the
> previously committed url for the first PageLoadTracker instantiated in each
> MetricsWebContentsObserver (the first navigation in the tab), in cases where
> there is an opener. so we'd basically be moving this logic into MWCO. this
seems
> conceptually consistent with the intent of providing a previously committed
url
> and reduces the complexity of PLMObservers. we might need to update the
comments
> for previously_committed_url slightly to explain this change in logic. the
only
> drawback here is that observers can no longer distinguish between these cases,
> but i'm not sure i expect that they'll have a need to. what do you think?

Agreed that's a better approach.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:406:
internal::NavigationFromGWSType navigation_type =
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> if navigation_type ends up returning NavigationNotFromGWS can we just return
> early and avoid doing any of the work below?

Yep.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:414:
// background is often slightly ahead of the signal for close.
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> interesting - is this due to recording some timings in the render process and
> some in the browser process, or is there just an inherent race here?

I think there's an inherent race with the WebContents destructor. This may have
something to do with the problems with Close on Android, but I couldn't figure
that one out today.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:417:
bool foregrounded =
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> can we factor this bit of logic into a static helper method, something like
> 
> bool WasAbortedInForeground(...) {
> if (time_to_abort.is_zero())
>   return false;
> if (abort_type == ABORT_NONE)
>   return false;
> if (WasStartedInForegroundEventInForeground(time_to_abort, extra_info)
>   return true;
> DCHECK(time_to_abort > extra_info.first_background_time);
> // Consider this a foregrounded abort if it occurred within 100ms of...
> TimeDelta bg_abort_delta = time_to_abort - extra_info.first_background_time;
> if (bg_abort_delta.InMillisecondsF() <= 100)
>   return true;
> return false;
> }
> 
> something like that anyway.

Acknowledged.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:461:
return internal::NavigationNotFromGWS;
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> this ends up implementing the policy we want but it doesn't feel like this
logic
> belongs in this method. can we instead check IsUrlFromGWS() at the call site
to
> GetNavigationFromGWSType and if that method returns true there, don't even
> continue past that point?

SGTM.

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/1901303004/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:31:
NavigationNotFromGWS,
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> let's put this first to make it the default / none / 0 case of the enum

Acknowledged.

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:68:
DCHECK(!timing.navigation_start.is_null());
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> yikes, is this a bug fix? good catch.

Not quite a bugfix, this bit of code is necessary so that we can bootstrap the
timer to send a provisional IPC to establish that the page load is being tracked
by the renderer. I was able to close a committed page that hadn't sent any IPCs,
so it didn't log any AfterCommit metrics.

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1901303004/diff/20001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:32: // Send
an initial IPC relatively early to help track aborts.
On 2016/04/21 20:50:41, Bryan McQuade wrote:
> do we anticipate it's likely that we'll batch up other sends in the first
50ms?
> if it seems unlikely i think just keeping this simple and calling SendNow
> directly seems reasonable.

So, calling SendNow() would be better, but it would cause the IPC to be sent
before the DidCommitProvisionalLoad IPC. Therefore this IPC would be thrown out.
I did a 50ms timer because it might batch up some other sends, will lose the
race for DidCommitProvisionalLoad, and is in sub-perceptual levels.

[Charlie Harrison, 2016-04-21 21:26:55]

FYI I am hugely supportive of not having observers know about the opener. That
means we have half as many histograms :)

FYI I found a bunch of scary abort related things due to Javascript redirects.

If a user aborts before the redirect has committed, we won't log it. Arguably
that's expected behavior. Testing locally, I found myself aborting *just* after
the redirector started actually redirecting, so my timings were very small.

I would just say to be wary of our BeforeCommit metrics. I trust our AfterCommit
metrics more for this case.

[Bryan McQuade, 2016-04-21 21:46:52]

On 2016/04/21 at 21:26:55, csharrison wrote:
> FYI I am hugely supportive of not having observers know about the opener. That
means we have half as many histograms :)
> 
> FYI I found a bunch of scary abort related things due to Javascript redirects.
> 
> If a user aborts before the redirect has committed, we won't log it. Arguably
that's expected behavior. Testing locally, I found myself aborting *just* after
the redirector started actually redirecting, so my timings were very small.
> 
> I would just say to be wary of our BeforeCommit metrics. I trust our
AfterCommit metrics more for this case.

Great, thanks! For the new tab/opener case, do we get the same signals in terms
of the navigation coming from a link and having a user gesture? Just want to
make sure it looks the same as a regular in-tab navigation for our purposes.

Are there any drawbacks to folding the opener URL into he
previously_committed_url? I don't see any but I haven't read through the thread
you cc'd me to yet so I might've missed something.

I was worried about folding the referer-based logic into the main histograms but
the opener case seems less risky since IIUC we do get the full url in that case.

[Charlie Harrison, 2016-04-21 21:50:28]

On 2016/04/21 21:46:52, Bryan McQuade wrote:
> On 2016/04/21 at 21:26:55, csharrison wrote:
> > FYI I am hugely supportive of not having observers know about the opener.
That
> means we have half as many histograms :)
> > 
> > FYI I found a bunch of scary abort related things due to Javascript
redirects.
> > 
> > If a user aborts before the redirect has committed, we won't log it.
Arguably
> that's expected behavior. Testing locally, I found myself aborting *just*
after
> the redirector started actually redirecting, so my timings were very small.
> > 
> > I would just say to be wary of our BeforeCommit metrics. I trust our
> AfterCommit metrics more for this case.
> 
> Great, thanks! For the new tab/opener case, do we get the same signals in
terms
> of the navigation coming from a link and having a user gesture? Just want to
> make sure it looks the same as a regular in-tab navigation for our purposes.
I believe so, but it's tough to check all the cases because most testing I'm
doing is using the js redirector, which loses that data. Any advice for testing
the other cases?

> 
> Are there any drawbacks to folding the opener URL into he
> previously_committed_url? I don't see any but I haven't read through the
thread
> you cc'd me to yet so I might've missed something.
I think worst case we will just miss navigations, but afaict we'd miss way more
with referrer checking due to using origin referrers.

> 
> I was worried about folding the referer-based logic into the main histograms
but
> the opener case seems less risky since IIUC we do get the full url in that
case.

[Charlie Harrison, 2016-04-22 16:32:48]

Added some tests. PTAL.

[Bryan McQuade, 2016-04-25 22:38:44]

Thanks! A few comments.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:377:
switch (navigation_type) {
there are few enough types in this enum now that I think it's reasonable to just
inline the logic that computes the enum in this function and get rid of the
enum.

i know that complicates the tests a little bit but i think that's ok. we can
audit that certain histograms get logged rather than inspecting the navigation
type in the tests, which is probably arguably better anyway since it's testing
what really matters.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:58:
static bool IsUrlFromGWS(const GURL& url);
if this one is also only public for testing, let's move it below the comment
below.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:83:
internal::NavigationFromGWSType GetNavigationFromGWSType(
it feels funny to be returning a type in the internal namespace in the public
API of the class. see my comment below about removing this, but if you decide we
can't remove it let's move the enum declaration out of the internal namespace.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:13:
const char kExampleFromGWS[] = "https://www.google.com/webhp?q=d";
nit: let's call this kGwsUrl or kGoogleSearchResultsUrl - the 'example' part of
kExampleUrl was to indicate it was for example.com.

[Bryan McQuade, 2016-04-26 14:52:59]

Can we split this into 2 or 3 changes?
1. abort metrics
2. opener
3. fix to send timing IPC on commit

I'd be open to merging 2 and 3.

I know this creates a bit more merge pain but this change feels a bit big.

[Charlie Harrison, 2016-04-26 16:53:35]

Ok here's the latest fix that I'll split up. I'll review the ping logic / opener
in that cl.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:377:
switch (navigation_type) {
On 2016/04/25 22:38:44, Bryan McQuade wrote:
> there are few enough types in this enum now that I think it's reasonable to
just
> inline the logic that computes the enum in this function and get rid of the
> enum.
> 
> i know that complicates the tests a little bit but i think that's ok. we can
> audit that certain histograms get logged rather than inspecting the navigation
> type in the tests, which is probably arguably better anyway since it's testing
> what really matters.

Fair enough. Done.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:58:
static bool IsUrlFromGWS(const GURL& url);
On 2016/04/25 22:38:44, Bryan McQuade wrote:
> if this one is also only public for testing, let's move it below the comment
> below.

Moved to private.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:83:
internal::NavigationFromGWSType GetNavigationFromGWSType(
On 2016/04/25 22:38:44, Bryan McQuade wrote:
> it feels funny to be returning a type in the internal namespace in the public
> API of the class. see my comment below about removing this, but if you decide
we
> can't remove it let's move the enum declaration out of the internal namespace.

It's true, but it is only used for tests, which is also the reasoning behind the
internal namespace. I removed it per your other comment, though.

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1901303004/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:13:
const char kExampleFromGWS[] = "https://www.google.com/webhp?q=d";
On 2016/04/25 22:38:44, Bryan McQuade wrote:
> nit: let's call this kGwsUrl or kGoogleSearchResultsUrl - the 'example' part
of
> kExampleUrl was to indicate it was for http://example.com.

Haha. Done. That's autopilot for you :)

[Charlie Harrison, 2016-04-26 17:41:42]

Description was changed from

==========
Add FromGWS variants to the AbortTiming metrics

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

to

==========
Add FromGWS variants to the AbortTiming metrics

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