[Sessions] Add detailed logging of SessionRestore events.

chrome/browser/sessions/session_restore_stats_collector.h

Index: chrome/browser/sessions/session_restore_stats_collector.h
diff --git a/chrome/browser/sessions/session_restore_stats_collector.h b/chrome/browser/sessions/session_restore_stats_collector.h
index 668e2c01b77a52e1a1ed57ea4017da9d870f44fd..85bd42ffb962e1618971d1d7908dd8f6aa4d893b 100644
--- a/chrome/browser/sessions/session_restore_stats_collector.h
+++ b/chrome/browser/sessions/session_restore_stats_collector.h
@@ -1,10 +1,30 @@
 // 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.
+//
+// Declares a utility class for tracking tabs involved in a session restore.

[sky, 2015/05/14 15:47:04]

Move this description above class description.

[chrisha, 2015/05/26 20:00:18]

Done.

+// If multiple session restores occur simultaneously (ie: multiple profiles)
+// they will use the same instance session restore stats tracker, as they
+// actually share a single TabLoader and are effectively visually
+// indistinguishable. This is done by effectively enforcing a singleton
+// pattern, with static entry points into the class allocating a collector if
+// necessary and repeatedly reusing the same collector.
+//
+// Since loading of deferred tabs is necessary statistics collection can be a
+// long term thing. To avoid having a stats collector that is only observing
+// deferred tabs get reused for a new session restore instance, the stats
+// collector will detach itself from being the current 'shared collector', and
+// allow a new collector to be created. This means that multiple stats
+// collectors may exist simultaneously, however at most one may be the active

[sky, 2015/05/14 15:47:04]

It's not clear what 'active' means here.

[chrisha, 2015/05/26 20:00:19]

Acknowledged.

+// shared collector.
+//
+// When all tabs involved in a session restore have loaded and painted, or been
+// closed, the session restore will destroy itself.

[sky, 2015/05/14 15:47:04]

'session restore' -> stats collector. But it appears you aren't consistent in
your comments here, eg "session restore stats tracker" on 7, "collector" on
10... I recommend using SessionRestoreStatsCollector everywhere.

[chrisha, 2015/05/26 20:00:18]

Done.

 
 #ifndef CHROME_BROWSER_SESSIONS_SESSION_RESTORE_STATS_COLLECTOR_H_
 #define CHROME_BROWSER_SESSIONS_SESSION_RESTORE_STATS_COLLECTOR_H_
 
+#include <map>
 #include <set>
 
 #include "base/callback_list.h"
@@ -30,15 +50,16 @@ class SessionRestoreStatsCollector
       const std::vector<SessionRestoreDelegate::RestoredTab>& tabs,
       const base::TimeTicks& restore_started);
 
-  // Called to start tracking only active tabs. If a restore is already
-  // occuring, the tabs are added to the existing list of tracked tabs.
-  static void TrackActiveTabs(
-      const std::vector<SessionRestoreDelegate::RestoredTab>& tabs,
-      const base::TimeTicks& restore_started);
+  // Called to indicate that the loading of a tab has been deferred by session
+  // restore. If the reason for the deferral was memory pressure this will be
+  // recorded.
+  static void DeferTab(content::NavigationController* tab);
 
  private:
   friend class base::RefCounted<SessionRestoreStatsCollector>;
 
+  // Maps a RenderWidgetHost to its deferred state.
+  using RenderWidgetHostMap = std::map<content::RenderWidgetHost*, bool>;

[sky, 2015/05/14 15:47:04]

Bools can be a bit mysterious, where as enums are generally less confusing.
Could you use an enum here?

[chrisha, 2015/05/26 20:00:18]

Done.

   using RenderWidgetHostSet = std::set<content::RenderWidgetHost*>;
 
   explicit SessionRestoreStatsCollector(const base::TimeTicks& restore_started);
@@ -52,7 +73,12 @@ class SessionRestoreStatsCollector
   // Adds new tabs to the list of tracked tabs.
   void AddTabs(const std::vector<SessionRestoreDelegate::RestoredTab>& tabs);
 
-  // Called when a tab is no longer tracked.
+  // Called to indicate that the loading of a tab has been deferred by session
+  // restore. Called by the static DeferTab.
+  void DeferTabImpl(content::NavigationController* tab);
+
+  // Called when a tab is no longer tracked. This is called by the 'Observe'
+  // notification callback.
   void RemoveTab(content::NavigationController* tab);
 
   // Registers for relevant notifications for a tab.
@@ -62,28 +88,71 @@ class SessionRestoreStatsCollector
   content::RenderWidgetHost* GetRenderWidgetHost(
       content::NavigationController* tab);
 
-  // Have we recorded the times for a foreground tab load?
+  // Returns true if the specified tab had it's loading deferred by an active
+  // session restore.
+  bool IsDeferred(content::NavigationController* tab) const;
+
+  // Returns true if done tracking non-deferred tabs. When this returns true
+  // the collector will detach itself from being the active shared collector
+  // and continue observing deferred tabs only. Called from Observe.
+  bool DoneTrackingNonDeferredTabs() const;
+
+  // Returns true when no longer tracking any tabs. When this returns true the
+  // collector will destory itself. Called from Observe.
+  bool DoneTracking() const;
+
+  // Has the the time for foreground tab load been recorded?
   bool got_first_foreground_load_;
 
-  // Have we recorded the times for a foreground tab paint?
+  // Has the time for foreground tab paint been recorded?
   bool got_first_paint_;
 
   // The time the restore process started.
   base::TimeTicks restore_started_;
 
-  // The renderers we have started loading into.
-  RenderWidgetHostSet render_widget_hosts_loading_;
-
-  // The renderers we have loaded and are waiting on to paint.
+  // The following 3 maps are used to track tabs at different points in their

[sky, 2015/05/14 15:47:04]

Seems like you're tracking a bunch of state per tab in numerous maps that would
be better served by a dedicated structure with a map from tab to state, eg:
struct TabState {
  bool was_deferred;  A bool is fine here as you always reference via
.was_deferred
  // I'm not positive you need both of these, an enum may better capture the
state.
  bool waiting_for_paint;
  bool is_loading;
};

[chrisha, 2015/05/26 20:00:18]

Unfortunately we need efficient lookup by 2 indices: NavigationControllers and
RenderWidgetHosts (hence the 2 separate maps for now).

We also need to know how many tabs are in each state, which is tracked for us
automatically via the size of the corresponding set. (We would be tracking this
explicitly ourselves.)

I thought this sounded like a reasonable suggestion and started refactoring the
code, but I quickly found it less readable. We could still go this route, but it
there's significantly more bookkeeping code and to be readable it would require
a bunch of helpers functions for performing state transitions, and a secondary
index.

+  // lifetime:
+  // 1. Tabs start in the tabs_tracked_ map.
+  // 2. When a tab starts loading it will be inserted into the
+  //    render_widget_hosts_loading_ map, and also remain in the tabs_tracked_
+  //    map.
+  // 3. Once a tab stops loading it will be removed from both the tabs_tracked_
+  //    and render_widget_hosts_loading_ maps, and be placed into the
+  //    render_widget_hosts_to_paint_ set.
+  // 4. When a tab has loaded and been painted it will be removed from the
+  //    render_widget_hosts_to_paint_ set and no longer be explicitly tracked
+  //    by the stats collector.
+  // Tabs are only tracked for paint events if got_first_paint_ is false. If
+  // first paint has already been observed then tabs no longer migrate into the
+  // render_widget_hosts_to_paint_ set.
+
+  // The renderers that have started loading, mapped to their deferred state
+  // and the total number of deferred tabs in the map. Render widgets in this
+  // map will also have their corresponding tabs in the tabs_tracked_ map.
+  RenderWidgetHostMap render_widget_hosts_loading_;
+  size_t render_widget_hosts_loading_deferred_count_;
+
+  // The renderers that have loaded and are waiting to paint A render widget in
+  // this map will not also be in the render_widget_hosts_loading_ map. The
+  // deferred state of a tab is not considered for paint events.
   RenderWidgetHostSet render_widget_hosts_to_paint_;
 
-  // List of tracked tabs.
-  std::set<content::NavigationController*> tabs_tracked_;
+  // List of tracked tabs, mapped to their deferred status, and the total
+  // number of deferred tabs in the map.
+  std::map<content::NavigationController*, bool> tabs_tracked_;
+  size_t tabs_tracked_deferred_count_;
+
+  // The total number of tabs that have been restored. This may be greater than
+  // the number of currently tracked tabs as tabs migrate out of the
+  // tabs_tracked_ map.

[sky, 2015/05/14 15:47:04]

Generally we use | around variable and parameter names in comments, eg
|tabs_tracked_|.

[chrisha, 2015/05/26 20:00:19]

Done.

+  size_t tab_count_;
 
-  // The number of tabs that have been restored.
-  int tab_count_;
+  // The total number of tabs that have been deferred. This may be greater than
+  // the number of currently tracked tabs as tabs migrate out of the
+  // tabs_tracked_ map.
+  size_t tab_deferred_count_;
 
-  // Max number of tabs that were loaded in parallel (for metrics).
+  // Max number of tabs that were observed to load in parallel (for metrics).
   size_t max_parallel_tab_loads_;
 
   // Notification registrar.