Optional <TimeDelta> since they may be non existent.

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 12287f6e72813bb3bb55189b88f3cf2262847582..57d50be46249fa59af270b90439d78cd2e9691d2 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
@@ -166,19 +166,23 @@ void LogPerformanceMetrics(
   }
 }
 
-bool WasAbortedInForeground(UserAbortType abort_type,
-                            base::TimeDelta time_to_abort,
-                            const page_load_metrics::PageLoadExtraInfo& info) {
-  if (time_to_abort.is_zero())
+bool WasAbortedInForeground(
+    UserAbortType abort_type,
+    const base::Optional<base::TimeDelta>& time_to_abort,

[Charlie Harrison, 2016/05/19 18:20:48]

Is it best practice to take const references of optionals?

[shivanisha, 2016/05/23 15:06:43]

Since base::Optional <base::TimeDelta> is bigger than an integer: contains and
integer and a boolean, thus passing by reference.

+    const page_load_metrics::PageLoadExtraInfo& info) {
+  if (!time_to_abort)
     return false;
   if (abort_type == UserAbortType::ABORT_NONE)
     return false;
-  if (WasStartedInForegroundEventInForeground(time_to_abort, info))
+  if (WasStartedInForegroundOptionalEventInForeground(time_to_abort, info))
     return true;
   if (!info.started_in_foreground)
     return false;
-  DCHECK_GT(time_to_abort, info.first_background_time);
-  base::TimeDelta bg_abort_delta = time_to_abort - info.first_background_time;
+
+  const base::TimeDelta& time_to_abort_val = time_to_abort.value();

[Charlie Harrison, 2016/05/19 18:20:48]

no const ref of TimeDelta.

[shivanisha, 2016/05/23 15:06:43]

done

+  const base::TimeDelta& time_to_first_bg = info.first_background_time.value();
+  DCHECK_GT(time_to_abort_val, time_to_first_bg);
+  base::TimeDelta bg_abort_delta = time_to_abort_val - time_to_first_bg;

[Charlie Harrison, 2016/05/19 18:20:48]

Don't use "bg" to abbreviate background. 

[shivanisha, 2016/05/23 15:06:43]

changed

   // Consider this a foregrounded abort if it occurred within 100ms of a
   // background. This is needed for closing some tabs, where the signal for
   // background is often slightly ahead of the signal for close.
@@ -384,20 +388,22 @@ void FromGWSPageLoadMetricsLogger::OnComplete(
   // consistency with PageLoad.Timing2 metrics, we ignore non-render-tracked
   // loads when tracking aborts after commit.
   UserAbortType abort_type = extra_info.abort_type;
-  base::TimeDelta time_to_abort = extra_info.time_to_abort;
   bool aborted_in_foreground =
-      WasAbortedInForeground(abort_type, time_to_abort, extra_info);
+      WasAbortedInForeground(abort_type, extra_info.time_to_abort, extra_info);
+
+  const base::Optional<base::TimeDelta>& time_to_abort =
+      extra_info.time_to_abort;
 
   if (!extra_info.committed_url.is_empty()) {
     LogPerformanceMetrics(timing, extra_info);
-    bool aborted_before_paint =
-        aborted_in_foreground && !timing.IsEmpty() &&
-        (timing.first_paint.is_zero() || timing.first_paint >= time_to_abort);
+    bool aborted_before_paint = aborted_in_foreground && !timing.IsEmpty() &&
+                                (timing.first_paint.is_zero() ||
+                                 timing.first_paint >= time_to_abort.value());
     if (aborted_before_paint)
-      LogCommittedAbortsBeforePaint(abort_type, time_to_abort);
+      LogCommittedAbortsBeforePaint(abort_type, time_to_abort.value());
   } else {
     if (aborted_in_foreground)
-      LogProvisionalAborts(abort_type, time_to_abort);
+      LogProvisionalAborts(abort_type, time_to_abort.value());
   }
 }