Update PageLoadTiming to use base::Optional

chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
index 11c35bd338c559a6005ccd202c705dbe95442cc4..62e640fd4d8cc1d5c6547a422e2548e76296592a 100644
--- a/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
@@ -371,6 +371,8 @@ bool FromGWSPageLoadMetricsLogger::QueryContainsComponentHelper(
   return false;
 }
 
+FromGWSPageLoadMetricsLogger::FromGWSPageLoadMetricsLogger() {}
+
 void FromGWSPageLoadMetricsLogger::SetPreviouslyCommittedUrl(const GURL& url) {
   previously_committed_url_is_search_results_ = IsGoogleSearchResultUrl(url);
   previously_committed_url_is_search_redirector_ =
@@ -495,16 +497,14 @@ void FromGWSPageLoadMetricsLogger::OnComplete(
   if (timing.IsEmpty())
     return;
 
-  if (timing.first_paint.is_zero() || timing.first_paint >= time_to_abort)
+  if (!timing.first_paint || timing.first_paint >= time_to_abort) {
     LogCommittedAbortsBeforePaint(abort_type, time_to_abort);
-
-  // Temporary hack as we can't distinguish TimeDelta unset from zero
-  // TODO(bmcquade): change back to else if once crbug.com/616901 is addressed
-  if (first_paint_triggered_ && timing.first_paint <= time_to_abort &&
-      (!has_user_interaction_after_paint_ ||
-       WasAbortedBeforeInteraction(
-           abort_type, first_user_interaction_after_paint_, time_to_abort)))
+  } else if (!first_user_interaction_after_paint_ ||
+             WasAbortedBeforeInteraction(
+                 abort_type, first_user_interaction_after_paint_.value(),
+                 time_to_abort)) {

[mushan, 2016/07/07 03:14:22]

The condition of first_user_interaction_after_paint_ not existing is logically a
part of WasAbortedBeforeInteraction. Passing in the optional value and check in
that function might be better?

[Bryan McQuade, 2016/07/08 18:39:35]

Good point - migrated this check to WasAbortedBeforeInteraction. Thanks!

     LogAbortsAfterPaintBeforeInteraction(abort_type, time_to_abort);
+  }
 }
 
 bool FromGWSPageLoadMetricsLogger::ShouldLogMetrics(const GURL& committed_url) {
@@ -546,12 +546,12 @@ bool FromGWSPageLoadMetricsLogger::ShouldLogMetrics(const GURL& committed_url) {
 }
 
 bool FromGWSPageLoadMetricsLogger::ShouldLogForegroundEventAfterCommit(
-    base::TimeDelta event,
+    const base::Optional<base::TimeDelta>& event,
     const page_load_metrics::PageLoadExtraInfo& info) {
   DCHECK(!info.committed_url.is_empty())
       << "ShouldLogForegroundEventAfterCommit called without committed URL.";
   return ShouldLogMetrics(info.committed_url) &&
-         WasStartedInForegroundEventInForeground(event, info);
+         WasStartedInForegroundOptionalEventInForeground(event, info);
 }
 
 void FromGWSPageLoadMetricsLogger::OnDomContentLoadedEventStart(
@@ -560,7 +560,7 @@ void FromGWSPageLoadMetricsLogger::OnDomContentLoadedEventStart(
   if (ShouldLogForegroundEventAfterCommit(timing.dom_content_loaded_event_start,
                                           extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSDomContentLoaded,
-                        timing.dom_content_loaded_event_start);
+                        timing.dom_content_loaded_event_start.value());
   }
 }
 
@@ -570,7 +570,7 @@ void FromGWSPageLoadMetricsLogger::OnLoadEventStart(
   if (ShouldLogForegroundEventAfterCommit(timing.load_event_start,
                                           extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSLoad,
-                        timing.load_event_start);
+                        timing.load_event_start.value());
   }
 }
 
@@ -579,7 +579,7 @@ void FromGWSPageLoadMetricsLogger::OnFirstPaint(
     const page_load_metrics::PageLoadExtraInfo& extra_info) {
   if (ShouldLogForegroundEventAfterCommit(timing.first_paint, extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSFirstPaint,
-                        timing.first_paint);
+                        timing.first_paint.value());
   }
   first_paint_triggered_ = true;
 }
@@ -590,7 +590,7 @@ void FromGWSPageLoadMetricsLogger::OnFirstTextPaint(
   if (ShouldLogForegroundEventAfterCommit(timing.first_text_paint,
                                           extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSFirstTextPaint,
-                        timing.first_text_paint);
+                        timing.first_text_paint.value());
   }
 }
 
@@ -600,7 +600,7 @@ void FromGWSPageLoadMetricsLogger::OnFirstImagePaint(
   if (ShouldLogForegroundEventAfterCommit(timing.first_image_paint,
                                           extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSFirstImagePaint,
-                        timing.first_image_paint);
+                        timing.first_image_paint.value());
   }
 }
 
@@ -610,15 +610,15 @@ void FromGWSPageLoadMetricsLogger::OnFirstContentfulPaint(
   if (ShouldLogForegroundEventAfterCommit(timing.first_contentful_paint,
                                           extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSFirstContentfulPaint,
-                        timing.first_contentful_paint);
+                        timing.first_contentful_paint.value());
 
     // If we have a foreground paint, we should have a foreground parse start,
     // since paints can't happen until after parsing starts.
-    DCHECK(WasStartedInForegroundEventInForeground(timing.parse_start,
-                                                   extra_info));
+    DCHECK(WasStartedInForegroundOptionalEventInForeground(timing.parse_start,
+                                                           extra_info));
     PAGE_LOAD_HISTOGRAM(
         internal::kHistogramFromGWSParseStartToFirstContentfulPaint,
-        timing.first_contentful_paint - timing.parse_start);
+        timing.first_contentful_paint.value() - timing.parse_start.value());
   }
 }
 
@@ -627,7 +627,7 @@ void FromGWSPageLoadMetricsLogger::OnParseStart(
     const page_load_metrics::PageLoadExtraInfo& extra_info) {
   if (ShouldLogForegroundEventAfterCommit(timing.parse_start, extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSParseStart,
-                        timing.parse_start);
+                        timing.parse_start.value());
   }
 }
 
@@ -636,14 +636,13 @@ void FromGWSPageLoadMetricsLogger::OnParseStop(
     const page_load_metrics::PageLoadExtraInfo& extra_info) {
   if (ShouldLogForegroundEventAfterCommit(timing.parse_stop, extra_info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramFromGWSParseDuration,
-                        timing.parse_stop - timing.parse_start);
+                        timing.parse_stop.value() - timing.parse_start.value());
   }
 }
 
 void FromGWSPageLoadMetricsLogger::OnUserInput(
     const blink::WebInputEvent& event) {
-  if (first_paint_triggered_ && first_user_interaction_after_paint_.is_zero()) {
-    has_user_interaction_after_paint_ = true;
+  if (first_paint_triggered_ && !first_user_interaction_after_paint_) {
     DCHECK(!navigation_start_.is_null());
     first_user_interaction_after_paint_ =
         base::TimeTicks::Now() - navigation_start_;