Make purge-and-suspend-time finch experiment parameter

chrome/browser/memory/tab_manager.cc

Index: chrome/browser/memory/tab_manager.cc
diff --git a/chrome/browser/memory/tab_manager.cc b/chrome/browser/memory/tab_manager.cc
index 207ae5d7bfd86e6a6951965eb4f1f980d4014ff3..c551175455530b5f700808ae935ade0e5fe66d32 100644
--- a/chrome/browser/memory/tab_manager.cc
+++ b/chrome/browser/memory/tab_manager.cc
@@ -221,6 +221,17 @@ void TabManager::Start() {
     }
   }
 #endif
+  std::string purge_and_suspend_time = variations::GetVariationParamValue(

[chrisha, 2016/11/24 17:13:09]

Should we have a default time that this is initialized to in the absence of a
variation param?

[tasak, 2016/11/25 01:15:24]

As far as I understand, base::TimeDelta() initializes delta_ as 0.
If purge-and-suspend-time is 0, purge-and-suspend is disabled (this is the same
behavior as the original purge-and-suspend-time).

Looking at minimum_protection_time_, it also depends on base::TimeDelta()'s
behavior (initializing delta_ as 0).
So do we need to explicitly initialize time_to_first_suspension_?

[chrisha, 2016/11/29 14:13:57]

Okay, that makes sense. I was thinking that there'd be a different on/off switch
for feature itself, and that only a non-zero purge-and-suspend delay would be
configurable via this variation. In which case having a non-zero default value
when the variation is not present would make sense.

This is fine, but I'd at least cleanly document that a zero purge and suspend
time indicates that the feature is disabled?

[tasak, 2016/12/02 02:49:24]

Yes... currently the zero means that the purge+suspend feature is disabled.

+      "PurgeAndSuspend", "purge-and-suspend-time");
+  if (!purge_and_suspend_time.empty()) {
+    int time_to_first_suspension_sec = 0;
+    if (base::StringToInt(purge_and_suspend_time,
+                          &time_to_first_suspension_sec)) {
+      if (time_to_first_suspension_sec > 0)
+        time_to_first_suspension_ =
+            base::TimeDelta::FromSeconds(time_to_first_suspension_sec);
+    }
+  }
 }
 
 void TabManager::Stop() {
@@ -731,21 +742,10 @@ TabManager::PurgeAndSuspendState TabManager::GetNextPurgeAndSuspendState(
 }
 
 void TabManager::PurgeAndSuspendBackgroundedTabs() {
-  const base::CommandLine& command_line =
-      *base::CommandLine::ForCurrentProcess();
-  if (!command_line.HasSwitch(switches::kPurgeAndSuspendTime))
-    return;
-  int purge_and_suspend_time = 0;
-  if (!base::StringToInt(
-          command_line.GetSwitchValueASCII(switches::kPurgeAndSuspendTime),
-          &purge_and_suspend_time)) {
-    return;
-  }
-  if (purge_and_suspend_time <= 0)
+  if (time_to_first_suspension_.InSeconds() <= 0)

[chrisha, 2016/11/24 17:13:09]

Do we need this check? This function simply shouldn't be called if
PurgeAndSuspend isn't enabled.

[tasak, 2016/11/25 01:15:24]

I would like to confirm. You mean, we should check
base::FeatureList::IsEnabled(kPurgeAndSuspend) before calling this function?

If so, I would like to explain again. I would like to do A/B testing. A is
"recording UMA when purge+suspend is really executed." and B is "recording UMA
at almost the same time when purge+suspend is executed but we don't
purge+suspend." (So B is "recording UMA when about purge-and-suspend time passes
after a renderer is backgrounded.)

If we don't call this function when
!base::FeatureList::IsEnabled(kPurgeAndSuspend), it is not possible to record B.

I'm not sure whether it is ok to always record B or not when we don't start any
finch experiment. So currently B's code (i.e.
RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when
purge-and-suspend-time=0 (and 0 is default value).

[tasak, 2016/11/25 01:19:44]

So my first patch is trying to always record B.
I thought, if someone wants not to change chrome behavior when
!IsEnabled(kPurgeAndSuspend), I will keep purge-and-suspend-time.

[chrisha, 2016/11/29 14:13:57]

I'm not sure I follow this entire chain of logic? This function is only ever
called from UpdateTimerCallback. If time_to_first_suspension_ is <= 0, then it
immediately returns, without recording anything.

Where are these UMAs you speak of? Will you be adding them to this function?

Regardless, I'm fine with leaving the code as is but more comments in the code
would be very useful here. Something as simple as:

// A zero-valued |time_to_first_suspension_| means the feature is disabled, so
return immediately.

[tasak, 2016/12/02 02:49:24]

I see.... I'm trying to keep the existing behavior... But now we have
purge+suspend feature. We don't need to control whether purge+suspend is enabled
or not by purge-and-suspend-time.
I also added the code to make default purge-and-suspend-time 30min.
Done.

     return;
+
   base::TimeTicks current_time = NowTicks();
-  base::TimeDelta time_to_first_suspension =
-      base::TimeDelta::FromSeconds(purge_and_suspend_time);
   auto tab_stats = GetUnsortedTabStats();
   for (auto& tab : tab_stats) {
     if (!tab.render_process_host->IsProcessBackgrounded())
@@ -766,7 +766,7 @@ void TabManager::PurgeAndSuspendBackgroundedTabs() {
            tab.last_hidden <
                GetWebContentsData(content)->LastPurgeAndSuspendModifiedTime());
     PurgeAndSuspendState next_state = GetNextPurgeAndSuspendState(
-        content, current_time, time_to_first_suspension);
+        content, current_time, time_to_first_suspension_);
     if (current_state == next_state)
       continue;