[Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore

chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cd1c1afbca0aa719d56a9d4d0438d105c7a066c1
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc
@@ -0,0 +1,80 @@
+// Copyright 2017 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/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h"
+
+#include <base/debug/stack_trace.h>

[Charlie Harrison, 2017/08/07 14:30:07]

nit: remove stack_trace

[ducbui, 2017/08/08 16:29:47]

Done.

+#include "base/logging.h"
+#include "base/memory/ptr_util.h"

[Charlie Harrison, 2017/08/07 14:30:07]

nit: unused.

[ducbui, 2017/08/08 16:29:47]

Done.

+#include "base/metrics/histogram_macros.h"

[Charlie Harrison, 2017/08/07 14:30:08]

nit: unused (you're just using our own custom macros).

[ducbui, 2017/08/08 16:29:47]

Done.

+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
+#include "chrome/browser/resource_coordinator/tab_manager.h"
+#include "chrome/browser/sessions/session_restore.h"

[Charlie Harrison, 2017/08/07 14:30:07]

nit: is it used?

[ducbui, 2017/08/08 16:29:47]

Acknowledged. It is not used so I will remove it.

+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents.h"
+#include "url/gurl.h"

[Charlie Harrison, 2017/08/07 14:30:07]

nit: unused.

[ducbui, 2017/08/08 16:29:47]

Done.

+
+namespace internal {
+
+const char kHistogramSessionRestoreForegroundTabFirstPaint[] =
+    "TabManager.Experimental.SessionRestore.ForegroundTab.FirstPaint";
+const char kHistogramSessionRestoreForegroundTabFirstContentfulPaint[] =
+    "TabManager.Experimental.SessionRestore.ForegroundTab.FirstContentfulPaint";
+const char kHistogramSessionRestoreForegroundTabFirstMeaningfulPaint[] =
+    "TabManager.Experimental.SessionRestore.ForegroundTab.FirstMeaningfulPaint";
+
+}  // namespace internal
+
+SessionRestorePageLoadMetricsObserver::SessionRestorePageLoadMetricsObserver() {
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+SessionRestorePageLoadMetricsObserver::OnStart(
+    content::NavigationHandle* navigation_handle,
+    const GURL& currently_committed_url,
+    bool started_in_foreground) {
+  content::WebContents* contents = navigation_handle->GetWebContents();
+  resource_coordinator::TabManager* tab_manager =
+      g_browser_process->GetTabManager();
+  // Should not be null because this is used only on supported platforms.
+  DCHECK(tab_manager);
+
+  return (tab_manager->IsTabInSessionRestore(contents) &&
+          tab_manager->IsTabRestoredInForeground(contents) &&
+          started_in_foreground)
+             ? CONTINUE_OBSERVING
+             : STOP_OBSERVING;
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+SessionRestorePageLoadMetricsObserver::OnHidden(
+    const page_load_metrics::mojom::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
+  return STOP_OBSERVING;

[Charlie Harrison, 2017/08/07 14:30:08]

Note: OnHidden() is called immediately when the tab is backgrounded. However,
timing updates are dispatched to this class asynchronously and batched at ~1s
intervals.

So, it is possible for the following sequence to occur:
1. First paint in foreground (in renderer)
2. OnHidden()
3. Timestamp from (1) is dispatched to this class, which is no longer observing
updates

Up to you whether you want to handle this case, just making sure you're aware of
it. If you do want to handle it, you can remove this method and instead gate the
metrics logging code with:
if (page_load_metrics::WasStartedInForegroundOptionalEventInForeground(event,
info)) {
 PAGE_LOAD_HISTOGRAM(...);
}

[ducbui, 2017/08/08 16:29:47]

Thank you for the suggestion! I did not aware of this issue. I will remove the
OnStart() and use WasStartedInForegroundOptionalEventInForeground() to check the
foreground state of the tab.

[Bryan McQuade, 2017/08/09 15:41:34]

Good point, yes. There are corner case timing issues with either - some
observers use the OnHidden approach, others the
WasStartedInForegroundOptionalEventInForeground. In practice we don't see
significant differences between the 2, so I think either is ok.

+}
+
+void SessionRestorePageLoadMetricsObserver::OnFirstPaintInPage(
+    const page_load_metrics::mojom::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  PAGE_LOAD_HISTOGRAM(internal::kHistogramSessionRestoreForegroundTabFirstPaint,
+                      timing.paint_timing->first_paint.value());

[Charlie Harrison, 2017/08/07 14:30:08]

#include page_load_metrics.mojom.h

[ducbui, 2017/08/08 16:29:47]

Done.

+}
+
+void SessionRestorePageLoadMetricsObserver::OnFirstContentfulPaintInPage(
+    const page_load_metrics::mojom::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  PAGE_LOAD_HISTOGRAM(
+      internal::kHistogramSessionRestoreForegroundTabFirstContentfulPaint,
+      timing.paint_timing->first_contentful_paint.value());
+}
+
+void SessionRestorePageLoadMetricsObserver::
+    OnFirstMeaningfulPaintInMainFrameDocument(
+        const page_load_metrics::mojom::PageLoadTiming& timing,
+        const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  PAGE_LOAD_HISTOGRAM(
+      internal::kHistogramSessionRestoreForegroundTabFirstMeaningfulPaint,
+      timing.paint_timing->first_meaningful_paint.value());
+}