Purge once random minutes(between 30min and 60min) after backgrounded.

chrome/browser/memory/tab_manager.h

Index: chrome/browser/memory/tab_manager.h
diff --git a/chrome/browser/memory/tab_manager.h b/chrome/browser/memory/tab_manager.h
index c384b7e054b07fa3eb20ad6b794bd748fcaf5665..0354410a049a21fd34a9447626698e4b96c023fb 100644
--- a/chrome/browser/memory/tab_manager.h
+++ b/chrome/browser/memory/tab_manager.h
@@ -147,7 +147,8 @@ class TabManager : public TabStripModelObserver {
   // Sets/clears the auto-discardable state of the tab.
   void SetTabAutoDiscardableState(content::WebContents* contents, bool state);
 
-  // Returns true when a given renderer can suspend when it is backgrounded.
+  // Returns true when a given renderer can be purged when it is backgrounded.

[Wez, 2017/03/02 02:28:32]

nit: Suggest ".. if the specified rendered is eligible for purging."

Also note that the name implies that it's invalid to call this for a
non-backgrounded renderer - is that the case?

[tasak, 2017/03/02 09:22:47]

Done.
I think, the name implies that the method doesn't check whether the renderer is
background one or not.
We need to move the check into the method? So we could rename the name into
CanPurgeRenderer or something.

+  // TODO(tasak): rename this to CanPurgeBackgroundedRenderer.
   bool CanSuspendBackgroundedRenderer(int render_process_id) const;
 
   // Returns true if |first| is considered less desirable to be killed than
@@ -159,9 +160,8 @@ class TabManager : public TabStripModelObserver {
   static int64_t IdFromWebContents(content::WebContents* web_contents);
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(TabManagerTest,
-                           ActivateTabResetPurgeAndSuspendState);
-  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, NextPurgeAndSuspendState);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ActivateTabResetPurgeState);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, NextPurgeState);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, AutoDiscardable);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, CanOnlyDiscardOnce);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ChildProcessNotifications);
@@ -176,16 +176,16 @@ class TabManager : public TabStripModelObserver {
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ProtectVideoTabs);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ReloadDiscardedTabContextMenu);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, TabManagerBasics);
-  FRIEND_TEST_ALL_PREFIXES(TabManagerWebContentsDataTest, PurgeAndSuspendState);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerWebContentsDataTest, PurgeState);
 
   // The time of the first purging after a renderer is backgrounded.
   // The initial value was chosen because most of users activate backgrounded
   // tabs within 30 minutes. (c.f. Tabs.StateTransfer.Time_Inactive_Active)
-  static constexpr base::TimeDelta kDefaultTimeToFirstPurge =
+  static constexpr base::TimeDelta kDefaultMinTimeToPurge =
       base::TimeDelta::FromMinutes(30);
 
   // This is needed so WebContentsData can call OnDiscardedStateChange, and
-  // can use PurgeAndSuspendState.
+  // can use PurgeState.
   friend class WebContentsData;
 
   // Called by WebContentsData whenever the discard state of a WebContents
@@ -258,38 +258,27 @@ class TabManager : public TabStripModelObserver {
   // that need to be run periodically (see comment in implementation).
   void UpdateTimerCallback();
 
-  // Initially PurgeAndSuspendState is RUNNING.
-  // RUNNING => SUSPENDED
-  // - A tab has been backgrounded for more than purge-and-suspend-time
-  //   seconds.
-  // SUSPENDED => RESUMED
-  // - A suspended tab is still suspended (i.e. last active time < last
-  //   purge-and-suspend modified time), and
-  // - kMaxTimeRendererAllowedToBeSuspendedBeforeResume time passes since
-  //   since the tab was suspended.
-  // RESUMED => SUSPENDED
-  // - A resumed tab is still backgrounded (i.e. last active time < last
-  //   purge-and-suspend modified time), and
-  // - kSuspendedRendererLengthOfResumption time passes since the tab was
-  //   resumed.
-  // SUSPENDED, RESUMED, RUNNING => RUNNING
-  // - When ActiveTabChaged, the newly activated tab's state will be RUNNING.
-  enum PurgeAndSuspendState {
-    RUNNING,
-    RESUMED,
-    SUSPENDED,
-  };
   // Returns WebContents whose contents id matches the given tab_contents_id.
   content::WebContents* GetWebContentsById(int64_t tab_contents_id) const;
 
-  // Returns the next state of the purge and suspend.
-  PurgeAndSuspendState GetNextPurgeAndSuspendState(
-      content::WebContents* content,
-      base::TimeTicks current_time,
-      const base::TimeDelta& time_to_first_suspension) const;
-
-  // Purges and suspends renderers in backgrounded tabs.
-  void PurgeAndSuspendBackgroundedTabs();
+  // Returns a random time-to-purge whose min value is min_time_to_purge and max
+  // value is min_time_to_purge + kRangeBetweenMinAndMaxTimeToPurge.

[Wez, 2017/03/02 02:28:32]

kRangeBetween... isn't defined anywhere in this header.

It's also not clear why this function needs to be public; IMO it would be better
to expose MinTimeUntilPurge and MaxTimeUntilPurge instead - you can then have
one test which lets the test choose, and ensures it's within that range, and a
separate test which uses a fixed value to verify that the should-purge result
changes correctly.

[tasak, 2017/03/02 09:22:47]

I would like to confirm your suggestion.

So:
- The GetTimeToPurge is a private method. So what the "public" means?

- What MinTimeUntilPurge and MaxTimeUntilPurge mean? They are default
TimeUntilPurge? Or could be updated by finch parameter?

- If we test GetTimeToPurge and use fixed value for ShouldPurgeAtDefaultTime
test (the old name is NextPurgeState), we could test enough?

- MaxTimeUntilPurge is also updated by finch parameter? If so, we should plan to
change MaxTimeUntilPurge at the future finch experiment?
  (I think, the range is fixed value, i.e. 30 minutes. I have no plan to test
the range at the future finch experiment.)

[Wez, 2017/03/03 21:06:51]

Sorry; I meant why does it need to be a member function, rather than a static
function in the .cc file?
I'm suggesting that rather than TimeToPurge and RangeOfTimeToPurge you have
MinTimeToPurge and MaxTimeToPurge as the parameters to the purge-timing logic;
the implementation is basically the same, but it's easier to talk about the
minimum and maximum time-to-purge.
Yes, I think that's what I was suggesting; the two tests are:

1. Given the configured Min and Max time-to-purge, do we choose a value that is
*sensible* - basically a sanity-check for whether we accidentally set it to 30
hours again!

2. Given a test-specified max and min, do we see the process purged when we
expect it to be? (i.e. definitely not purged until min, and definitely is purged
after max)
If you prefer to have a single parameter then I'd suggest:

- Have TimeToPurge as that parameter's name,
- When choosing the actual time-to-purge, choose a value in the range
TimeToPurge..2*TimeToPurge.

i.e. if we set time-to-purge to 30m then it'll purge between 30- and 60- minutes
after backgrounding.  If we set it to 5m then the range will be 5m-10m.

Make sense?

[tasak, 2017/03/06 10:30:19]

On 2017/03/03 21:06:51, Wez wrote:
range at the future finch experiment.)
Yeah, this looks good idea. I tried this.

So the reason why I prefer a single parameter is that I have no idea about how
to measure user experience changes when changing [min..max-time-to-purge].
We know, at least, the range should not be small.

+  base::TimeDelta GetTimeToPurge(base::TimeDelta min_time_to_purge) const;
+
+  // Returns whether to purge or not at the given time.
+  bool ShouldPurgeAtTime(content::WebContents* content,
+                         base::TimeTicks time) const;
+
+  // Purges renderers in backgrounded tabs if the following conditions are
+  // satisfied:
+  // - the renderers are not purged yet,
+  // - the renderers are not playing media,
+  //   (CanPurgeBackgroundedRenderer returns true)
+  // - the renderers are left inactive and background for time-to-purge.
+  // If renderers are purged, their internal states become 'purged'.
+  // The state is reset to be 'not purged' only when they are activated
+  // (=ActiveTabChanged is invoked).
+  void PurgeBackgroundedTabsIfNeeded();
 
   // Does the actual discard by destroying the WebContents in |model| at |index|
   // and replacing it by an empty one. Returns the new WebContents or NULL if
@@ -372,8 +361,8 @@ class TabManager : public TabStripModelObserver {
   // backgrounded.
   base::TimeDelta minimum_protection_time_;
 
-  // A backgrounded renderer will be suspended when this time passes.
-  base::TimeDelta time_to_first_suspension_;
+  // A backgrounded renderer will be purged when this time passes.
+  base::TimeDelta min_time_to_purge_;
 
 #if defined(OS_CHROMEOS)
   std::unique_ptr<TabManagerDelegate> delegate_;