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) {
-  return info.started_in_foreground && !event.is_zero() &&
-         (info.first_background_time.is_zero() ||
-          event < info.first_background_time);
+namespace {
+
+// Returns true if:
+// - We have timing information for the event.
+// - The event occurred prior to the page being moved to the background.
+bool InForeground(base::TimeDelta event, const PageLoadExtraInfo& info) {

[Bryan McQuade, 2016/02/24 19:34:44]

i'm not super keen on this change. I think you're trying to do too much in these
functions.

given that you're adding one new UMA, and the
WasStartedInBackgroundEventInForeground is used exactly once, could we please
inline the logic for that function at the one call site and leave the existing
logic as is?

if we find a need to use WasStartedInBackgroundEventInForeground more than once
we can see about factoring it into common code at that time.

+  if (event.is_zero())
+    return false;
+
+  if (!info.started_in_foreground &&
+      (info.first_foreground_time.is_zero() ||
+       event < info.first_foreground_time)) {
+    return false;
+  }
+
+  return info.first_background_time.is_zero() ||
+         event < info.first_background_time;
+}
+
+}  // namespace
+
+bool WasStartedInForegroundEventInForeground(base::TimeDelta event,
+                                             const PageLoadExtraInfo& info) {
+  return info.started_in_foreground && InForeground(event, info);
+}
+
+bool WasStartedInBackgroundEventInForeground(base::TimeDelta event,

[Bryan McQuade, 2016/02/24 21:31:53]

So, this is going to be disappointing, but I'd like to reduce the amount of code
being changed here.

Unless you're fixing known bugs, I'd like to minimize the changes we make to
this code, in order to avoid possible regressions in metrics. We just had a bug
in our performance.timing metrics make it out to stable, so I'd like to be extra
cautious and try to minimize changes in our metrics code.

The old implementation of EventOccurredInForeground is substantially simpler
than the new one you've added which delegates to the more complex InForeground,
and given how frequently we use EventOccurredInForeground, I think simplicity is
preferred.

Could you restore the original implementation of
WasStartedInForegroundEventInForeground as it was in EventOccurredInForeground,
and implement the logic you need for WasStartedInBackgroundEventInForeground
separately, without any shared logic?

Let's keep the 2 fixes you made (the one for where we log
kHistogramBackgroundBeforePaint, and the other where we log
kHistogramFirstBackground). Thanks for those!

[pkotwicz, 2016/02/24 22:03:55]

Can you discuss with csharrison@? Patch set #1 does not modify this file at all.
I made the changes in page_load_metrics_util.cc based on csharrison@'s code
review feedback (Comment #11)

[Bryan McQuade, 2016/02/24 23:45:57]

I think I saw the comment you're referring to - Charles said:

"""
Possibly we could change to
page_load_metrics::StartInBackgroundEventInForeground
page_load_metrics::StartInForegroundEventInForeground

I don't think that's too too unreadable. You could also use a temporary bool.
"""

this was in response to your comment 

"""
this would require renaming
"""

I don't think that he was suggesting that you factor the implementation of
EventOccurredInForeground into a common method that ends up being more complex
than the original EventOccurredInForeground. I could be misunderstanding, but in
any case I don't want the existing method to become any more complex unless it's
to fix a bug.

Let's please revert to the original implementation used on
EventOccurredInForeground. I'm fine with renaming to
StartInForegroundEventInForeground as your change does. I don't think the
increased complexity of the implementation makes having shared code worthwhile,
however.

I'll take an AI to improve unit testing for these methods in a future CL so this
is marginally less fragile as we go forward.

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