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..98756c14529b534fd973ab654868992a54b7d540 100644
--- a/chrome/browser/memory/tab_manager.h
+++ b/chrome/browser/memory/tab_manager.h
@@ -159,9 +159,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,7 +175,7 @@ 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
@@ -185,7 +184,7 @@ class TabManager : public TabStripModelObserver {
       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 +257,25 @@ class TabManager : public TabStripModelObserver {
   // that need to be run periodically (see comment in implementation).
   void UpdateTimerCallback();
 
-  // Initially PurgeAndSuspendState is RUNNING.
-  // RUNNING => SUSPENDED
+  // Initially PurgeState is NOT_PURGED.
+  // NOT_PURGED => PURGED
   // - 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,
+  // PURGED => NOT_PURGED
+  // - When ActiveTabChaged, the newly activated tab's state will be NOT_PURGED.

[Wez, 2017/02/27 22:20:56]

typo: ActiveTabChanged

The naming here implies that purging is a one-off operation, i.e. that we purge
one, at the point of switching between states - but I think we actually purge
every 20 minutes?

[tasak, 2017/02/28 09:45:37]

The new code doesn't purge every 20 minutes, because I think, ActiveTabChanged
is invoked when a user moves a background tab to foreground.

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

I thought the idea was to re-purge each background tab every 20 minutes or so,
though?

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

As far as I understand, we have a strong concern that re-purge occurs under
critical memory pressure situation.
I agree that 1 purge also occurs under the same situation, but next purge
doesn't.
So I would like to start from 1 purge implementation.

+  enum PurgeState {

[Wez, 2017/02/27 22:20:56]

Note that as per style-guide, this and the other enums and types defined in this
private section should preferably come before function and method definitions.

[tasak, 2017/02/28 09:45:37]

Removed.

+    NOT_PURGED,
+    PURGED,
   };
   // Returns WebContents whose contents id matches the given tab_contents_id.
   content::WebContents* GetWebContentsById(int64_t tab_contents_id) const;

[Wez, 2017/02/27 22:20:55]

Why is this getter placed randomly in-between the enum typedef and
GetNextPurgeState()?

[tasak, 2017/02/28 09:45:37]

Removed.

 
   // Returns the next state of the purge and suspend.

[Wez, 2017/02/27 22:20:55]

The function is called GetNextPurgeState() so the reader can see that it returns
the next state - this comment should clarify what that _means_, and how it is
used.

e.g. "Returns PURGED if |content| should be asked to purge memory at time
|current_time|".

Why do we need a |current_time| parameter - why not use Now()? If the point is
that we sometimes pass in something other than |current_time| then rename it to
just |time| or similar?

[tasak, 2017/02/28 09:45:37]

Replaced the method with ShouldPurgeAtTime.

On 2017/02/27 22:20:55, Wez wrote:
For testing. If we use Now() in the method, it is difficult to pass test_clock.

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

The class has a set_test_tick_clock() method, though, so surely we just set
that, and then call this function after having fast-forwarded the clock?

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

I see.
Done.

-  PurgeAndSuspendState GetNextPurgeAndSuspendState(
-      content::WebContents* content,
-      base::TimeTicks current_time,
-      const base::TimeDelta& time_to_first_suspension) const;
+  PurgeState GetNextPurgeState(content::WebContents* content,
+                               base::TimeTicks current_time) const;
 
-  // Purges and suspends renderers in backgrounded tabs.
-  void PurgeAndSuspendBackgroundedTabs();
+  // Purges renderers in backgrounded tabs.

[Wez, 2017/02/27 22:20:56]

Again, the reader can tell that from the name; what's important is what that
means, and how it happens, e.g. is this called periodically by some internal
timer? Or is it called whenever a tab state changes?

Also, the comment is not strictly accurate - this function only purges tabs
which are not already in the PURGED state.

[tasak, 2017/02/28 09:45:37]

Done.

+  void PurgeBackgroundedTabs();
 
   // 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 +358,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 time_to_first_purge_;

[Wez, 2017/02/27 22:20:56]

This doesn't change over the lifetime of the TabManager, so you could initialize
it in the constructor and make the member const.

[tasak, 2017/02/28 09:45:39]

So is it possible to initialize the value by using GetVariationParamValue and
StringToUint in the constructor?

As far as I understand, TabManager::Start() is invoked at almost the same time
as TabManager is created. I think, it is ok to initialize the value in Start().

 
 #if defined(OS_CHROMEOS)
   std::unique_ptr<TabManagerDelegate> delegate_;