Notify page load metrics when the app enters the background.

chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

Index: chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
diff --git a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
index 9f927511dcde2c2122bcbf117d29eded4a9bbb0d..d3a5b9de344d3d72e9d8baf33f55c7290e1b4c66 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
@@ -22,6 +22,8 @@
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_widget_host.h"
+#include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -53,6 +55,8 @@ const char kClientRedirectWithoutPaint[] =
     "PageLoad.Internal.ClientRedirect.NavigationWithoutPaint";
 const char kCommitToCompleteNoTimingIPCs[] =
     "PageLoad.Internal.CommitToComplete.NoTimingIPCs";
+const char kPageLoadCompletedAfterAppBackground[] =
+    "PageLoad.Internal.PageLoadCompleted.AfterAppBackground";
 
 }  // namespace internal
 
@@ -247,6 +251,7 @@ PageLoadTracker::PageLoadTracker(
     int aborted_chain_size,
     int aborted_chain_size_same_url)
     : did_stop_tracking_(false),
+      app_entered_background_(false),
       navigation_start_(navigation_handle->NavigationStart()),
       url_(navigation_handle->GetURL()),
       abort_type_(ABORT_NONE),
@@ -263,6 +268,10 @@ PageLoadTracker::PageLoadTracker(
 }
 
 PageLoadTracker::~PageLoadTracker() {
+  if (app_entered_background_) {

[Charlie Harrison, 2016/07/28 14:45:27]

Hm. Can you log a 3-state histogram instead of a 2 state one? I'd like a count
of how many page load trackers destruct with app_entered_background_ = true.

[Bryan McQuade, 2016/07/28 16:26:11]

I think the third state you describe should just be the difference between the
false counts and the true counts. Does that sound right to you?

[Bryan McQuade, 2016/07/28 16:29:15]

Ah, sorry, re-reading your comment, I think this histogram logging is the thing
you're asking for "a count of how many page load trackers destruct with
app_entered_background_ = true". Did you mean something else?

[Charlie Harrison, 2016/07/28 16:38:04]

Ah you're right, my mistake.

+    UMA_HISTOGRAM_BOOLEAN(internal::kPageLoadCompletedAfterAppBackground, true);
+  }
+
   if (did_stop_tracking_)
     return;
 
@@ -398,6 +407,14 @@ void PageLoadTracker::OnInputEvent(const blink::WebInputEvent& event) {
   }
 }
 
+void PageLoadTracker::FlushMetricsOnAppEnterBackground() {
+  if (!app_entered_background_) {
+    UMA_HISTOGRAM_BOOLEAN(internal::kPageLoadCompletedAfterAppBackground,
+                          false);
+    app_entered_background_ = true;
+  }
+}
+
 void PageLoadTracker::NotifyClientRedirectTo(
     const PageLoadTracker& destination) {
   if (timing_.first_paint) {
@@ -615,6 +632,33 @@ MetricsWebContentsObserver::~MetricsWebContentsObserver() {
   NotifyAbortAllLoads(ABORT_CLOSE);
 }
 
+// static
+std::vector<MetricsWebContentsObserver*>
+MetricsWebContentsObserver::GetAllObservers() {
+  std::vector<MetricsWebContentsObserver*> result;
+  std::unique_ptr<content::RenderWidgetHostIterator> widgets(
+      content::RenderWidgetHost::GetRenderWidgetHosts());
+  while (content::RenderWidgetHost* rwh = widgets->GetNextHost()) {
+    content::RenderViewHost* rvh = content::RenderViewHost::From(rwh);
+    if (!rvh)
+      continue;
+    content::WebContents* web_contents =
+        content::WebContents::FromRenderViewHost(rvh);
+    if (!web_contents)
+      continue;
+    if (web_contents->GetRenderViewHost() != rvh)

[Charlie Harrison, 2016/07/28 14:45:27]

How can this happen?

[Bryan McQuade, 2016/07/28 16:26:11]

I actually lifted this logic from
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
which I would otherwise have just called directly, except it's in
WebContentsImpl instead of WebContents.

I don't see a reason though not to just move this to WebContents so code outside
of content/ can use it. WDYT? If you think it's reasonable I could ask a content
owner what they think. 

[Charlie Harrison, 2016/07/28 16:38:04]

Yeah, let's see if we can expose this. No point duplicating the logic.

[Charlie Reis, 2016/07/28 17:36:49]

Adding jam@, who has been against having a public way to iterate over all
WebContents.  I think the idea is that the embedder should have a way to know
about all the tabs it has created?  (Obviously, copy/pasting the code isn't
desirable either.)

[Charlie Harrison, 2016/07/28 17:42:17]

Ah that makes sense. Bryan, have you look at doing this from the tab strip?

[Charlie Reis, 2016/07/28 17:56:44]

That would be great if it covered all the cases you need.  Are you just looking
for tabs in a single window, or would you loop through all |browser| objects as
well?
There are lots of WebContents which don't appear in a tab strip, like Devtools,
prerender, and other non-tab cases.  (Not sure if we still have notifications or
background WebContents anymore, but I think so.)  Your approach would be nice if
you only care about the ones in the tab strip.

+      continue;
+    MetricsWebContentsObserver* observer =
+        MetricsWebContentsObserver::FromWebContents(web_contents);
+    if (!observer)
+      continue;
+    // Verify there are no duplicates.
+    DCHECK(std::find(result.begin(), result.end(), observer) == result.end());

[Charlie Harrison, 2016/07/28 14:45:27]

If you want this feature let's use an std::set instead of vector, then you can
check this upon insertion.

[Bryan McQuade, 2016/07/28 16:26:11]

The drawback to set<> is that iteration order will vary depending on pointer
order, which I've seen lead to subtle issues in some cases. Using vector<> means
they'll consistently be returned in RWHIterator order.

This DCHECK is essentially a code version of the comment in the WebContentsImpl:
    // Because a WebContents can only have one current RVH at a time, there will
    // be no duplicate WebContents here.
I could just include that comment instead if you prefer.

[Charlie Harrison, 2016/07/28 16:38:04]

No, your reasoning makes sense. Keeping this DCHECK SGTM.

+    result.push_back(observer);
+  }
+  return result;
+}
+
 void MetricsWebContentsObserver::RegisterInputEventObserver(
     content::RenderViewHost* host) {
   if (host != nullptr)
@@ -805,6 +849,17 @@ void MetricsWebContentsObserver::OnInputEvent(
     committed_load_->OnInputEvent(event);
 }
 
+void MetricsWebContentsObserver::FlushMetricsOnAppEnterBackground() {
+  if (committed_load_)
+    committed_load_->FlushMetricsOnAppEnterBackground();
+  for (const auto& kv : provisional_loads_) {
+    kv.second->FlushMetricsOnAppEnterBackground();
+  }
+  for (const auto& tracker : aborted_provisional_loads_) {
+    tracker->FlushMetricsOnAppEnterBackground();
+  }
+}
+
 void MetricsWebContentsObserver::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())