Add common page filtering logic for page load metrics.

chrome/common/page_load_metrics/page_track_decider.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/common/page_load_metrics/page_track_decider.h"
+
+namespace page_load_metrics {
+
+PageTrackDecider::PageTrackDecider() {}
+PageTrackDecider::~PageTrackDecider() {}
+
+bool PageTrackDecider::ShouldTrack() {

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

I wonder if this is easier to read:

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

[Bryan McQuade, 2016/09/14 15:10:58]

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.

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

SGTM

+  // Ignore non-HTTP schemes (e.g. chrome://).
+  if (!IsHTTPOrHTTPSUrl())
+    return false;
+
+  // Ignore NTP loads.
+  if (IsNewTabPageUrl())
+    return false;
+
+  if (HasCommitted()) {
+    // Ignore Chrome error pages (e.g. No Internet connection).
+    if (IsChromeErrorPage())
+      return false;
+
+    // Ignore network error pages (e.g. 4xx, 5xx).
+    if (IsHTTPErrorPage())
+      return false;
+
+    // Ignore non-HTML documents (e.g. SVG, images).
+    if (!IsHTMLOrXHTMLPage())
+      return false;
+  }
+
+  return true;
+}
+
+}  // namespace page_load_metrics