Add common page filtering logic for page load metrics.

Add common page filtering logic for page load metrics.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/80cf9541db8094f3b233d2b7b2590aaa25c5af1b
Cr-Commit-Position: refs/heads/master@{#419467}

[Bryan McQuade, 2016-09-12 20:05:55]

Description was changed from

==========
Add common page filtering logic for page load metrics.

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

to

==========
Add common page filtering logic for page load metrics.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-09-12 20:05:59]

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

[commit-bot: I haz the power, 2016-09-12 20:06:25]

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

[Bryan McQuade, 2016-09-12 20:07:16]

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

[commit-bot: I haz the power, 2016-09-12 20:07:43]

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

[Bryan McQuade, 2016-09-12 20:30:35]

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

[commit-bot: I haz the power, 2016-09-12 20:30:55]

Dry run: 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, 2016-09-12 21:12:09]

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

[commit-bot: I haz the power, 2016-09-12 21:12:10]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Bryan McQuade, 2016-09-13 15:21:06]

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

[commit-bot: I haz the power, 2016-09-13 15:21:33]

Dry run: 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, 2016-09-13 16:43:44]

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

[commit-bot: I haz the power, 2016-09-13 16:43:45]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-09-13 17:39:21]

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_tracker_predicate.h (right):

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_tracker_predicate.h:51: virtual bool
IsNetworkErrorPage() = 0;
perhaps this should be called 'IsHTTPErrorPage'? I hesitated there only because
readers might get confused as to whether it's HTTP specific (not HTTPS) - do you
think that's a concern? Which name do you prefer?

[Charlie Harrison, 2016-09-13 17:51:03]

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

[Charlie Harrison, 2016-09-13 17:51:04]

This is a cool change. Thank you for doing it!

1. I wonder if implementing this as a stack allocated class makes sense, or if
you could just use static functions? The pattern of doing:

Decider d(args...)
return d.decide()

Seems less performant (and less elegant) than:

return decide(args...)



2. Can you improve documentation on the tracker class? I was reading it first
and it wasn't immediately clear that this is about synchronizing the renderer
and browser. I thought maybe observers could hook their own tracker impls.

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_tracker_predicate.h (right):

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_tracker_predicate.h:17: class
PageTrackerPredicate {
Calling a class a predicate doesn't really sit well with me. Isn't a predicate
usually a function?

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_tracker_predicate.h:51: virtual bool
IsNetworkErrorPage() = 0;
On 2016/09/13 17:39:21, Bryan McQuade wrote:
> perhaps this should be called 'IsHTTPErrorPage'? I hesitated there only
because
> readers might get confused as to whether it's HTTP specific (not HTTPS) - do
you
> think that's a concern? Which name do you prefer?

Go with HTTP. You could even be more verbose: "IsHTTP2XXOr3XXResponse".

[Bryan McQuade, 2016-09-13 17:59:10]

Thanks! RE: stack allocated object, I'm not sure how to simplify further. I
could do something like this:

BrowserPageTrackerPolicyInfo page_tracker_info(
    embedder_interface_.get(), web_contents(), navigation_handle);
return ShouldTrackPage(page_tracker_info);

I believe I still need to instantiate a per-navigation concrete instance that
implements the virtual interface exposing the information needed to make the
policy decision. The difference here is that ShouldTrackPage is not an instance
method of the PageTrackerPolicyInfo base interface, which I think is slightly
better, but I'm not sure. ShouldTrackPage still needs to live in common, and
would pobably be declared in the base virtual class's header (possibly as a
static function). WDYT?

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_tracker_predicate.h (right):

https://codereview.chromium.org/2331053003/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_tracker_predicate.h:17: class
PageTrackerPredicate {
On 2016/09/13 at 17:51:04, Charlie Harrison wrote:
> Calling a class a predicate doesn't really sit well with me. Isn't a predicate
usually a function?

Yeah, I agree - it's not my favorite name either, but I couldn't think of a
better name here. What else might we call it. I do think a predicate is usually
a function, or 'function object' which exposes a virtual boolean method to be
invoked. Here's one example:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/declarativ...

[Charlie Harrison, 2016-09-13 18:09:04]

Ok your explanations make sense. I was hoping for something using static methods
but we do need virtual methods...

Maybe we could inline, like:
Decider(args...).decide()?

[Bryan McQuade, 2016-09-13 18:59:40]

On 2016/09/13 at 18:09:04, csharrison wrote:
> Ok your explanations make sense. I was hoping for something using static
methods but we do need virtual methods...
> 
> Maybe we could inline, like:
> Decider(args...).decide()?

Thanks! Decider sounds like a better suffix than predicate. WDYT about:

return BrowserPageTrackDecider(args...).ShouldTrack();

[Charlie Harrison, 2016-09-13 19:02:30]

On 2016/09/13 18:59:40, Bryan McQuade wrote:
> On 2016/09/13 at 18:09:04, csharrison wrote:
> > Ok your explanations make sense. I was hoping for something using static
> methods but we do need virtual methods...
> > 
> > Maybe we could inline, like:
> > Decider(args...).decide()?
> 
> Thanks! Decider sounds like a better suffix than predicate. WDYT about:
> 
> return BrowserPageTrackDecider(args...).ShouldTrack();

That looks good.

[Bryan McQuade, 2016-09-13 19:13:58]

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

[commit-bot: I haz the power, 2016-09-13 19:14:33]

Dry run: 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, 2016-09-13 20:29:49]

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

[commit-bot: I haz the power, 2016-09-13 20:29:50]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-09-13 20:42:09]

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

[commit-bot: I haz the power, 2016-09-13 20:42:37]

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

[Bryan McQuade, 2016-09-13 21:36:02]

Thanks! I addressed the things we chatted about. PTAL when you have a chance.

[commit-bot: I haz the power, 2016-09-13 22:59:28]

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

[commit-bot: I haz the power, 2016-09-13 22:59:33]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Charlie Harrison, 2016-09-14 13:20:05]

Forgot to send comments out last night. Overall looks great.

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/browser_page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/browser_page_track_decider.h:29: 
// PageTrackDecider:

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:1068: if
(navigation_handle->HasCommitted() && navigation_handle->IsSamePage())
Why can't this go into the tracker?

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.cc:12: bool
PageTrackDecider::ShouldTrack() {
I wonder if this is easier to read:

if (HasCommitted()) {
  return !IsChromeErrorPage() &&
         !IsHTTPErrorPage() &&
         IsHTMLOrXHTMLPage();
} else {
  return IsHTTPOrHTTPSUrl() && !IsNewTabPageUrl();
}

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:31: virtual bool
IsHTTPOrHTTPSUrl() = 0;
I think this should this be "HttpOrHttps".

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:51: virtual bool
IsHTTPErrorPage() = 0;
Is there any reason you didn't want to go with "IsNotHTTP2XXOr3XX" or something
similar? That's more clear to me.

https://codereview.chromium.org/2331053003/diff/100001/chrome/renderer/page_l...
File chrome/renderer/page_load_metrics/renderer_page_track_decider.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/renderer_page_track_decider.cc:32: const GURL
url = document_->url();
Can this method be just one line?

https://codereview.chromium.org/2331053003/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/renderer_page_track_decider.cc:58: std::string
mime_type = data_source_->response().mimeType().utf8();
Could you directly compare the WebString without copying to std::string?

https://codereview.chromium.org/2331053003/diff/100001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/404.html (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/404.html:3: I am a 404 page!
hehe :)

[Bryan McQuade, 2016-09-14 14:43:22]

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

[commit-bot: I haz the power, 2016-09-14 14:43:41]

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

[Bryan McQuade, 2016-09-14 15:10:23]

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

[commit-bot: I haz the power, 2016-09-14 15:10:46]

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

[Bryan McQuade, 2016-09-14 15:10:59]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/browser_page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/browser_page_track_decider.h:29: 
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> // PageTrackDecider:


I think this is a bit redundant in this case, as PageTrackDecider is the only
interface we inherit, so overridden methods must be from the base class, and we
don't add any new methods to the API.

If this is part of Chromium style guide I'm happy to add it but otherwise in
this case I don't think it adds value.

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:1068: if
(navigation_handle->HasCommitted() && navigation_handle->IsSamePage())
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> Why can't this go into the tracker?

Good question. I decided that the concept of a same page navigation isn't an
attribute of a page load, but rather is a detail specific to navigation.

Similarly to how we bail early for non-main-frame navigations, I updated the
logic to also bail early for same page navigations. This is consistent with our
bail early logic in the render process.

I think this is better overall - same page navs aren't (currently) interesting
to us and I think we want to ignore them and bail early before we even get into
deciding if we should track the page load. See what you think.

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.cc:12: bool
PageTrackDecider::ShouldTrack() {
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> I wonder if this is easier to read:
> 
> if (HasCommitted()) {
>   return !IsChromeErrorPage() &&
>          !IsHTTPErrorPage() &&
>          IsHTMLOrXHTMLPage();
> } else {
>   return IsHTTPOrHTTPSUrl() && !IsNewTabPageUrl();
> }

We want to run the non-committed checks in either committed or non-committed
mode so the if/else won't work here, but we could do something conceptually
similar. I'm mixed on which style is easier to read. I have a slight preference
for keeping it as-is since I think the comments associated with each case are
important and adding comments inline in a multi-line return is less readable
IMO. Additionally with the move to return the status code, the logic no longer
fits this style of chaining all calls as cleanly.

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:31: virtual bool
IsHTTPOrHTTPSUrl() = 0;
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> I think this should this be "HttpOrHttps".

Yeah, I prefer the style you note as well, however existing Chromium APIs use
all caps. In particular, GURL's API uses all caps, so I did this to be
consistent.

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:51: virtual bool
IsHTTPErrorPage() = 0;
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> Is there any reason you didn't want to go with "IsNotHTTP2XXOr3XX" or
something similar? That's more clear to me.

My concern with that is that it may be overly descriptive - to be honest I'm not
100% certain that we want to allow all 2xx and 3xx responses. For instance I've
wondered about 204 (we should exclude them), as well as 3xx that aren't 304 (all
other 3xx I'm aware of are for redirects - seems like an error to record those
as page loads).

For now I switched the API to return the status code, and the policy for
deciding if it's an error page now lives in the base class's ShouldTrack method.
I need to do a separate pass to decide about 204 and non-304 3xx, but I think
returning the status code is an improvement over the boolean method.

https://codereview.chromium.org/2331053003/diff/100001/chrome/renderer/page_l...
File chrome/renderer/page_load_metrics/renderer_page_track_decider.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/renderer_page_track_decider.cc:32: const GURL
url = document_->url();
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> Can this method be just one line?

Yeah - the reason I did this is a bit silly - WebDocument::url() returns a
WebUrl, which has a conversion operator to produce a GURL. So if I just wrote

document_->url().SchemeIsHTTPOrHTTPS();

compilation fails because there is no SchemeIsHTTPOrHTTPS on WebURL. But I can
do it in one line using a static cast, which is shorter.

https://codereview.chromium.org/2331053003/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/renderer_page_track_decider.cc:58: std::string
mime_type = data_source_->response().mimeType().utf8();
On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> Could you directly compare the WebString without copying to std::string?

Ah, I think so, yes. The default WebString equality comparison treats the mime
type as latin1 characters, which seems like it should be fine here. Done,
thanks!

[commit-bot: I haz the power, 2016-09-14 15:47:33]

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

[commit-bot: I haz the power, 2016-09-14 15:47:34]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Charlie Harrison, 2016-09-14 16:27:32]

Will do a final pass when bots are green.

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/browser_page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/browser_page_track_decider.h:29: 
On 2016/09/14 15:10:58, Bryan McQuade wrote:
> On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > // PageTrackDecider:
> 
> 
> I think this is a bit redundant in this case, as PageTrackDecider is the only
> interface we inherit, so overridden methods must be from the base class, and
we
> don't add any new methods to the API.
> 
> If this is part of Chromium style guide I'm happy to add it but otherwise in
> this case I don't think it adds value.

Nope, just a convention we've been using. Happy to defer to your personal style
in this case.

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:1068: if
(navigation_handle->HasCommitted() && navigation_handle->IsSamePage())
On 2016/09/14 15:10:58, Bryan McQuade wrote:
> On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > Why can't this go into the tracker?
> 
> Good question. I decided that the concept of a same page navigation isn't an
> attribute of a page load, but rather is a detail specific to navigation.
> 
> Similarly to how we bail early for non-main-frame navigations, I updated the
> logic to also bail early for same page navigations. This is consistent with
our
> bail early logic in the render process.
> 
> I think this is better overall - same page navs aren't (currently) interesting
> to us and I think we want to ignore them and bail early before we even get
into
> deciding if we should track the page load. See what you think.

Great explanation, I just wonder if we want to be explicit about also blocking
these in the renderer (I'm not entirely sure how that process works today).

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.cc:12: bool
PageTrackDecider::ShouldTrack() {
On 2016/09/14 15:10:58, Bryan McQuade wrote:
> On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > I wonder if this is easier to read:
> > 
> > if (HasCommitted()) {
> >   return !IsChromeErrorPage() &&
> >          !IsHTTPErrorPage() &&
> >          IsHTMLOrXHTMLPage();
> > } else {
> >   return IsHTTPOrHTTPSUrl() && !IsNewTabPageUrl();
> > }
> 
> We want to run the non-committed checks in either committed or non-committed
> mode so the if/else won't work here, but we could do something conceptually
> similar. I'm mixed on which style is easier to read. I have a slight
preference
> for keeping it as-is since I think the comments associated with each case are
> important and adding comments inline in a multi-line return is less readable
> IMO. Additionally with the move to return the status code, the logic no longer
> fits this style of chaining all calls as cleanly.

SGTM

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:31: virtual bool
IsHTTPOrHTTPSUrl() = 0;
On 2016/09/14 15:10:58, Bryan McQuade wrote:
> On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > I think this should this be "HttpOrHttps".
> 
> Yeah, I prefer the style you note as well, however existing Chromium APIs use
> all caps. In particular, GURL's API uses all caps, so I did this to be
> consistent.

The new style guide recommends this in new code:
http://google.github.io/styleguide/cppguide.html#Function_Names

Here is the associated chromium-dev thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/k8-ztIrWaUY
Consensus on the thread seems to stick to style guide for new code.

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:51: virtual bool
IsHTTPErrorPage() = 0;
On 2016/09/14 15:10:58, Bryan McQuade wrote:
> On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > Is there any reason you didn't want to go with "IsNotHTTP2XXOr3XX" or
> something similar? That's more clear to me.
> 
> My concern with that is that it may be overly descriptive - to be honest I'm
not
> 100% certain that we want to allow all 2xx and 3xx responses. For instance
I've
> wondered about 204 (we should exclude them), as well as 3xx that aren't 304
(all
> other 3xx I'm aware of are for redirects - seems like an error to record those
> as page loads).
> 
> For now I switched the API to return the status code, and the policy for
> deciding if it's an error page now lives in the base class's ShouldTrack
method.
> I need to do a separate pass to decide about 204 and non-304 3xx, but I think
> returning the status code is an improvement over the boolean method.

Your change sounds good. This makes it easier for an implementer to know what to
actually implement in the method :)

[Bryan McQuade, 2016-09-15 20:40:30]

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

[commit-bot: I haz the power, 2016-09-15 20:41:48]

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

[Bryan McQuade, 2016-09-15 20:45:45]

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

[commit-bot: I haz the power, 2016-09-15 20:46:04]

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

[Bryan McQuade, 2016-09-15 20:49:39]

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

[commit-bot: I haz the power, 2016-09-15 20:50:12]

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

[Bryan McQuade, 2016-09-15 22:28:10]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:1068: if
(navigation_handle->HasCommitted() && navigation_handle->IsSamePage())
On 2016/09/14 at 16:27:32, Charlie Harrison wrote:
> On 2016/09/14 15:10:58, Bryan McQuade wrote:
> > On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > > Why can't this go into the tracker?
> > 
> > Good question. I decided that the concept of a same page navigation isn't an
> > attribute of a page load, but rather is a detail specific to navigation.
> > 
> > Similarly to how we bail early for non-main-frame navigations, I updated the
> > logic to also bail early for same page navigations. This is consistent with
our
> > bail early logic in the render process.
> > 
> > I think this is better overall - same page navs aren't (currently)
interesting
> > to us and I think we want to ignore them and bail early before we even get
into
> > deciding if we should track the page load. See what you think.
> 
> Great explanation, I just wonder if we want to be explicit about also blocking
these in the renderer (I'm not entirely sure how that process works today).

Sure, we have code in place in the renderer to block these:

void MetricsRenderFrameObserver::DidCommitProvisionalLoad(
    bool is_new_navigation,
    bool is_same_page_navigation) {
  if (is_same_page_navigation)
    return;

  page_timing_metrics_sender_.reset();

  if (ShouldSendMetrics()) {
...

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_track_decider.h (right):

https://codereview.chromium.org/2331053003/diff/100001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_track_decider.h:31: virtual bool
IsHTTPOrHTTPSUrl() = 0;
On 2016/09/14 at 16:27:32, Charlie Harrison wrote:
> On 2016/09/14 15:10:58, Bryan McQuade wrote:
> > On 2016/09/14 at 13:20:05, Charlie Harrison wrote:
> > > I think this should this be "HttpOrHttps".
> > 
> > Yeah, I prefer the style you note as well, however existing Chromium APIs
use
> > all caps. In particular, GURL's API uses all caps, so I did this to be
> > consistent.
> 
> The new style guide recommends this in new code:
> http://google.github.io/styleguide/cppguide.html#Function_Names
> 
> Here is the associated chromium-dev thread:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/k8-ztIrWaUY
> Consensus on the thread seems to stick to style guide for new code.

Ah, great, thanks! I updated the code to use Http rather than HTTP.

[commit-bot: I haz the power, 2016-09-15 22:57:05]

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

[commit-bot: I haz the power, 2016-09-15 22:57:06]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-09-16 12:25:49]

LGTM appreciate the cleanup

[Bryan McQuade, 2016-09-16 17:46:51]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-09-16 17:47:18]

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, 2016-09-16 17:56:36]

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

[commit-bot: I haz the power, 2016-09-16 17:56:38]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bryan McQuade, 2016-09-16 21:05:39]

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

[Bryan McQuade, 2016-09-16 21:05:50]

jochen, PTAL for BUILD.gn changes, thanks!

[Charlie Harrison, 2016-09-16 21:10:32]

On 2016/09/16 21:05:50, Bryan McQuade wrote:
> jochen, PTAL for BUILD.gn changes, thanks!

On a meta note: is there a reason that the BUILD.gn files are not owned by
everyone? I seem to remember gypi ones didn't require owners review.

[jochen (gone - plz use gerrit), 2016-09-19 14:28:21]

lgtm

feel free to bring up the issue with BUILD.gn ownership on chromium-dev or so.

It used to be restricted while gn was still not very stable

[Bryan McQuade, 2016-09-19 14:35:01]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-09-19 14:35:12]

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, 2016-09-19 15:45:54]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-19 15:48:10]

Description was changed from

==========
Add common page filtering logic for page load metrics.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add common page filtering logic for page load metrics.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/80cf9541db8094f3b233d2b7b2590aaa25c5af1b
Cr-Commit-Position: refs/heads/master@{#419467}
==========

[commit-bot: I haz the power, 2016-09-19 15:48:12]

Patchset 11 (id:??) landed as
https://crrev.com/80cf9541db8094f3b233d2b7b2590aaa25c5af1b
Cr-Commit-Position: refs/heads/master@{#419467}