Allow for pausing background tabs on desktop via about:flags.

Allow for pausing timers in background tabs on desktop via about:flags.

This is a first step towards trying to get a flag that's good
enough for us to dogfood ourselves to see if there's something
we could ship regarding pausing background tabs that's web
compatible enough.

Speculatively plumb through the site engagement levels so that the followup
patch can hook them up to the scheduler.

Just shipping this flag as is would clearly not be compatible
enough though.

BUG=709082
Review-Url: https://codereview.chromium.org/2805683003
Cr-Commit-Position: refs/heads/master@{#463749}
Committed: https://chromium.googlesource.com/chromium/src/+/38c67835dc36e9378584fde1da493eccb906db26

[ojan, 2017-04-06 16:51:37]

ojan@chromium.org changed reviewers:
+ jochen@chromium.org

[ojan, 2017-04-06 16:51:37]

Jochen, putting you as reviewer for OWNERS reasons.

panicker, skyostil, altimin: FYI. I'm not trying to drive this project to
completion, but I'm hoping to get enough in place that we can turn it on
ourselves via about:flags and see what the experience is like so it can inform
our intuition of what's reasonably shippable. Next steps for this will be to a)
make the suspending be at the webview level instead of the renderer level and b)
only suspend background tabs with low site engagement.

[jochen (gone - plz use gerrit), 2017-04-07 06:57:08]

please ask a metrics owner to approve the histograms.xml change - I can only
approve UseCounter additions. The rest lgtm

[ojan, 2017-04-07 17:01:33]

ojan@chromium.org changed reviewers:
+ asvitkine@chromium.org

[ojan, 2017-04-07 17:01:34]

Alexei, lgty?

[Alexei Svitkine (slow), 2017-04-07 17:09:07]

For new flags, the best practice is to use a base::Feature. See
go/feature-api-announcement

Suggest using that mechanism instead of adding a flag.

[Sami, 2017-04-07 17:11:12]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2017-04-07 17:11:12]

lgtm % Alexei's comment.

https://codereview.chromium.org/2805683003/diff/1/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2805683003/diff/1/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:657: const char kPauseBackgroundTabs[] =
"pause-background-tabs";
Please add a comment here for the automatically generated flag documentation.

[panicker, 2017-04-07 19:44:05]

panicker@chromium.org changed reviewers:
+ panicker@chromium.org

[panicker, 2017-04-07 19:44:06]

LGTM
This suspends timers only right? If so - could you update the CL description and
maybe flag name. Otherwise currently it sounds like you are suspending all CPU
activity.

https://codereview.chromium.org/2805683003/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2805683003/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.cc:2726: "Pause timers in background tabs after
5 minutes like on mobile.";
bit confusing, maybe mention desktop explicitly in description (as its not in
the name) ?

[ojan, 2017-04-08 02:13:50]

Description was changed from

==========
Allow for pausing background tabs on desktop via about:flags.

This is a first step towards trying to get a flag that's good
enough for us to dogfood ourselves to see if there's something
we could ship regarding pausing background tabs that's web
compatible enough.

Just shipping this flag as is would clearly not be compatible
enough though.

BUG=709082
==========

to

==========
Allow for pausing timers in background tabs on desktop via about:flags.

This is a first step towards trying to get a flag that's good
enough for us to dogfood ourselves to see if there's something
we could ship regarding pausing background tabs that's web
compatible enough.

Speculatively plumb through the site engagement levels so that the followup
patch can hook them up to the scheduler.

Just shipping this flag as is would clearly not be compatible
enough though.

BUG=709082
==========

[ojan, 2017-04-08 02:28:53]

I switched to using base::Feature, but I also went ahead and added the site
engagement values I'm going to want in my next patch. At the moment they're not
hooked up, so they just act like disabling the feature, but I'll hook it up in
the next patch.

Alexei, PTAL when you have a chance.

https://codereview.chromium.org/2805683003/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2805683003/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.cc:2726: "Pause timers in background tabs after
5 minutes like on mobile.";
On 2017/04/07 at 19:44:06, panicker wrote:
> bit confusing, maybe mention desktop explicitly in description (as its not in
the name) ?

Done. FWIW, it won't show up in about:flags on mobile.

https://codereview.chromium.org/2805683003/diff/1/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2805683003/diff/1/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:657: const char kPauseBackgroundTabs[] =
"pause-background-tabs";
On 2017/04/07 at 17:11:12, Sami wrote:
> Please add a comment here for the automatically generated flag documentation.

Not needed anymore now that I'm using base::Feature.

[Alexei Svitkine (slow), 2017-04-10 20:44:49]

lgtm % comments

https://codereview.chromium.org/2805683003/diff/20001/chrome/common/pause_tab...
File chrome/common/pause_tabs_field_trial.h (right):

https://codereview.chromium.org/2805683003/diff/20001/chrome/common/pause_tab...
chrome/common/pause_tabs_field_trial.h:22: const char
kPauseBackgroundTabsFeatureModeParameterMax[] = "max";
Nit: Given all of these already in pausetabs namespace, maybe names can be
shortened?

e.g. kModeParamMinimal, kModeParamLow, etc.

Same for the other ones. Even the feature could just be pausetabs::kFeature

[ojan, 2017-04-11 00:41:11]

The CQ bit was checked by ojan@chromium.org

[ojan, 2017-04-11 00:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org, panicker@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2805683003/#ps40001
(title: "address review feedback")

[commit-bot: I haz the power, 2017-04-11 00:42:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 00:45:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 00:45:28]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ojan, 2017-04-11 01:09:26]

The CQ bit was checked by ojan@chromium.org

[ojan, 2017-04-11 01:09:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, jochen@chromium.org, skyostil@chromium.org,
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2805683003/#ps60001
(title: "fix merge conflict.")

[commit-bot: I haz the power, 2017-04-11 01:10:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 02:50:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 02:50:53]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[ojan, 2017-04-11 16:47:28]

The CQ bit was checked by ojan@chromium.org

[ojan, 2017-04-11 16:47:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, jochen@chromium.org, skyostil@chromium.org,
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2805683003/#ps80001
(title: "Fix android build")

[commit-bot: I haz the power, 2017-04-11 16:47:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 18:03:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 18:03:39]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ojan, 2017-04-11 19:54:00]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2017-04-11 19:54:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 20:28:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491940440688970,
"parent_rev": "6aff6f114be3dea567abd0409e07ea7a394c7527", "commit_rev":
"38c67835dc36e9378584fde1da493eccb906db26"}

[commit-bot: I haz the power, 2017-04-11 20:28:53]

Description was changed from

==========
Allow for pausing timers in background tabs on desktop via about:flags.

This is a first step towards trying to get a flag that's good
enough for us to dogfood ourselves to see if there's something
we could ship regarding pausing background tabs that's web
compatible enough.

Speculatively plumb through the site engagement levels so that the followup
patch can hook them up to the scheduler.

Just shipping this flag as is would clearly not be compatible
enough though.

BUG=709082
==========

to

==========
Allow for pausing timers in background tabs on desktop via about:flags.

This is a first step towards trying to get a flag that's good
enough for us to dogfood ourselves to see if there's something
we could ship regarding pausing background tabs that's web
compatible enough.

Speculatively plumb through the site engagement levels so that the followup
patch can hook them up to the scheduler.

Just shipping this flag as is would clearly not be compatible
enough though.

BUG=709082

Review-Url: https://codereview.chromium.org/2805683003
Cr-Commit-Position: refs/heads/master@{#463749}
Committed:
https://chromium.googlesource.com/chromium/src/+/38c67835dc36e9378584fde1da49...
==========

[commit-bot: I haz the power, 2017-04-11 20:28:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/38c67835dc36e9378584fde1da49...