[page_load_metrics] User Initiated Abort Tracking (Observer version)

chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h

Index: chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h
diff --git a/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h b/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..c83815ae051990a1f48e096b5a879e88dc2f52b9
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h
@@ -0,0 +1,44 @@
+// 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.
+
+#ifndef CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ABORTS_PAGE_LOAD_METRICS_OBSERVER_H_
+#define CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ABORTS_PAGE_LOAD_METRICS_OBSERVER_H_
+
+#include "components/page_load_metrics/browser/page_load_metrics_observer.h"
+
+const char kHistogramAbortForwardBackBeforeCommit[] =
+    "PageLoad.AbortTiming.ForwardBackNavigation.BeforeCommit";

[Alexei Svitkine (slow), 2015/12/14 22:14:40]

Don't define constants in header files.

Instead, if you want to expose them, put their definition in the .cc file and
declare them in the header. See for example chrome_switches.cc/h.

Also, I'm not a big fun of dumping these in the global namespace. Can you put
them inside a namespace or a class? If you're exposing these only for testing, I
suggest putting them in an "internal" namespace.

[Charlie Harrison, 2015/12/14 23:16:30]

I used the extern pattern for this case, and added an internal namespace. Can
you point me to a resource that explains this rule? I couldn't find it in the
style guide.

[Alexei Svitkine (slow), 2015/12/15 16:42:33]

I think putting them in the header can cause linker errors as multiple .cc files
will end up defining the same symbols when more than one file includes the
header. See:
http://stackoverflow.com/questions/2328671/constant-variables-not-working-in-...

So it might not be covered in the style guide since it's not a matter of style,
but a matter of functionality.

[Charlie Harrison, 2015/12/15 19:35:45]

Ah okay that makes sense. I have to file a bug for other places in the code
where I've done this. I think this hasn't broken anything yet because we aren't
including from multiple translation units yet. Thanks for the help.

+const char kHistogramAbortReloadBeforeCommit[] =
+    "PageLoad.AbortTiming.Reload.BeforeCommit";
+const char kHistogramAbortNewNavigationBeforeCommit[] =
+    "PageLoad.AbortTiming.NewNavigation.BeforeCommit";
+const char kHistogramAbortStopBeforeCommit[] =
+    "PageLoad.AbortTiming.Stop.BeforeCommit";
+const char kHistogramAbortCloseBeforeCommit[] =
+    "PageLoad.AbortTiming.Close.BeforeCommit";
+const char kHistogramAbortOtherBeforeCommit[] =
+    "PageLoad.AbortTiming.Other.BeforeCommit";
+const char kHistogramAbortForwardBackBeforePaint[] =
+    "PageLoad.AbortTiming.ForwardBackNavigation.AfterCommit.BeforePaint";
+const char kHistogramAbortReloadBeforePaint[] =
+    "PageLoad.AbortTiming.Reload.AfterCommit.BeforePaint";
+const char kHistogramAbortNewNavigationBeforePaint[] =
+    "PageLoad.AbortTiming.NewNavigation.AfterCommit.BeforePaint";
+const char kHistogramAbortStopBeforePaint[] =
+    "PageLoad.AbortTiming.Stop.AfterCommit.BeforePaint";
+const char kHistogramAbortCloseBeforePaint[] =
+    "PageLoad.AbortTiming.Close.AfterCommit.BeforePaint";
+
+class AbortsPageLoadMetricsObserver
+    : public page_load_metrics::PageLoadMetricsObserver {
+ public:
+  AbortsPageLoadMetricsObserver();
+
+  // page_load_metrics::PageLoadMetricsObserver implementation:

[Alexei Svitkine (slow), 2015/12/14 22:14:40]

Nit: I think for new code, the comment convention is:

// page_load_metrics::PageLoadMetricsObserver:

[Charlie Harrison, 2015/12/14 23:16:30]

Done

+  void OnComplete(
+      const page_load_metrics::PageLoadTiming& timing,
+      const page_load_metrics::PageLoadExtraInfo& extra_info) override;
+};

[Alexei Svitkine (slow), 2015/12/14 22:14:40]

Nit: Add private: and DISALLOW_COPY_AND_ASSIGN()

[Charlie Harrison, 2015/12/14 23:16:30]

Done.

+
+#endif  // CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ABORTS_PAGE_LOAD_METRICS_OBSERVER_H_