Add metric with time between background tab being foregrounded and the first paint

components/page_load_metrics/browser/page_load_metrics_util.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 "components/page_load_metrics/browser/page_load_metrics_util.h"
 
 #include <algorithm>
 
 #include "components/page_load_metrics/browser/page_load_metrics_observer.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
 
 namespace page_load_metrics {
 
-bool EventOccurredInForeground(const base::TimeDelta& event,
-                               const PageLoadExtraInfo& info) {
+namespace {
+
+// Returns whether the page moved to the foreground prior to |event|. True if
+// the page was always in the foreground.

[Charlie Harrison, 2016/02/22 16:08:32]

Nit: Not quite true. This function will return true for a load that starts in
the foreground and immediately backgrounds. I would amend the comment to say
"True if the page was started in the foreground."

[pkotwicz, 2016/02/22 23:14:26]

I created a function InForeground() and made both
StartInForegroundEventInForeground() and StartInBackgroundEventInForeground()
call it.

I like the new InForeground() function because its return value is very clear.

+bool MovedToForegroundBefore(const base::TimeDelta& event,

[Charlie Harrison, 2016/02/22 16:08:32]

I think this function could also be renamed to something more clear, like
"InForegroundBeforeEvent." In this case, the transition is not as important as
the background case, because it captures the simpler notion of "was the page
ever in the foreground before the event occurred."

+                             const PageLoadExtraInfo& info) {
+  return info.started_in_foreground || (!info.first_foreground_time.is_zero() &&
+                                        event > info.first_foreground_time);
+}
+
+// Returns whether the page moved to the background after |event|. True if the
+// page never moved to the background.
+bool MovedToBackgroundAfter(const base::TimeDelta& event,

[Charlie Harrison, 2016/02/22 16:08:32]

This is a confusing name given the comment and what it actually represents.
Naming these things is really tricky though.

It seems like this is really trying to express "The event occurred before any
background transition," not "the page moved to the background after the event."
I'd suggest renaming this to more closely fit the former.

+                            const PageLoadExtraInfo& info) {
+  return info.first_background_time.is_zero() ||
+         event < info.first_background_time;
+}
+
+}  // namespace
+
+bool StartInForegroundEventInForeground(const base::TimeDelta& event,
+                                        const PageLoadExtraInfo& info) {
   return info.started_in_foreground && !event.is_zero() &&
-         (info.first_background_time.is_zero() ||
-          event < info.first_background_time);
+         MovedToBackgroundAfter(event, info);
+}
+
+bool StartInBackgroundEventInForeground(const base::TimeDelta& event,

[Charlie Harrison, 2016/02/22 16:08:32]

nit: do you mind replacing all "const base::TimeDelta&"s you touch with raw
"base::TimeDelta"s? The time classes are all POD and should not be passed by
const reference. This was a mistake we made earlier.

[pkotwicz, 2016/02/22 23:14:26]

Done.

+                                        const PageLoadExtraInfo& info) {
+  return !info.started_in_foreground && !event.is_zero() &&
+         MovedToForegroundBefore(event, info) &&
+         MovedToBackgroundAfter(event, info);
 }
 
 }  // namespace page_load_metrics