[scheduler] Use Finch to control background throttling.

[scheduler] Use Finch to control background throttling.

BUG=639852
Committed: https://crrev.com/56bb594b1b341695c09b9041cdf02a76af306cc3
Cr-Commit-Position: refs/heads/master@{#431013}

[altimin, 2016-11-02 15:54:08]

Description was changed from

==========
[scheduler] Use Finch to control background throttling.

BUG=639852
==========

to

==========
[scheduler] Use Finch to control background throttling.

BUG=639852
==========

[altimin, 2016-11-02 15:54:09]

altimin@chromium.org changed reviewers:
+ alexclarke@chromium.org, skyostil@chromium.org

[altimin, 2016-11-02 16:20:34]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 16:20:49]

Dry run: 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, 2016-11-02 17:33:00]

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

[commit-bot: I haz the power, 2016-11-02 17:33:01]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[alex clarke (OOO till 29th), 2016-11-02 17:53:49]

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:421: const
std::map<std::string, std::string>& settings,
Are you aware of base::DictionaryValue?

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:424: auto find_it =
settings.find(setting_name);
const auto

https://codereview.chromium.org/2471153002/diff/1/content/public/common/web_p...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2471153002/diff/1/content/public/common/web_p...
content/public/common/web_preferences.cc:184:
expensive_background_throttling_cpu_budget(-1),
-1.0f etc? (to match code below)

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:29:
if (web_settings_budget < 0)
< 0.0? (here and below)

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:223:
background_time_budget_pool_ =
DCHECK background_time_budget_pool_ isn't already initialized?

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h
(right):

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:90:
WebSettingsImpl* web_settings_;  // Not owned.
should this be const?

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:435: m_scheduler(nullptr),
I think we can omit this (odd the line above exists)

[altimin, 2016-11-02 21:10:01]

altimin@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[altimin, 2016-11-02 21:10:02]

PTAL

+jochen@ for content/ and blink changes
+mkwst@ for IPC changes.

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:421: const
std::map<std::string, std::string>& settings,
On 2016/11/02 17:53:48, alex clarke wrote:
> Are you aware of base::DictionaryValue?

Could you clarify this, please?
I'm aware of base::DictionaryValue, but Finch API returns map of strings.

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:424: auto find_it =
settings.find(setting_name);
On 2016/11/02 17:53:48, alex clarke wrote:
> const auto

Done.

https://codereview.chromium.org/2471153002/diff/1/content/public/common/web_p...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2471153002/diff/1/content/public/common/web_p...
content/public/common/web_preferences.cc:184:
expensive_background_throttling_cpu_budget(-1),
On 2016/11/02 17:53:48, alex clarke wrote:
> -1.0f etc? (to match code below)

Done.

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:29:
if (web_settings_budget < 0)
On 2016/11/02 17:53:48, alex clarke wrote:
> < 0.0? (here and below)

Done.

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:223:
background_time_budget_pool_ =
On 2016/11/02 17:53:49, alex clarke wrote:
> DCHECK background_time_budget_pool_ isn't already initialized?

Replaced with checking and exiting the function.

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h
(right):

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:90:
WebSettingsImpl* web_settings_;  // Not owned.
On 2016/11/02 17:53:49, alex clarke wrote:
> should this be const?

Done.

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2471153002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:435: m_scheduler(nullptr),
On 2016/11/02 17:53:49, alex clarke wrote:
> I think we can omit this (odd the line above exists)

Done.

[jochen (gone - plz use gerrit), 2016-11-03 11:56:58]

I'll wait for alexclarke@ and skyostil@ to finish their reviews first

[Sami, 2016-11-03 13:21:15]

https://codereview.chromium.org/2471153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:420: void
MaybeSetParameterFromMap(
MaybeSetFloat/DoubleParameterFromMap?

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
content/public/common/web_preferences.cc:185:
expensive_background_throttling_initial_budget(1.0f),
Should all of these be negative by default so the scheduler gets to choose the
defaults?

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
content/public/common/web_preferences.h:213: // Specifies aggressiveness of
background tab throttling.
Please mention that these are in seconds. Ideally the names would say that too
but they are getting long.

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right):

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/DEPS:7:
"+web/WebSettingsImpl.h",
I don't think we can depend on web/ here. Another option would be to add a
scheduler API for configuring these settings.

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:232:
// Disable throttling because page is visible by default.
What do you mean by default? We're checking the current visibility state here,
right?

[alex clarke (OOO till 29th), 2016-11-03 15:10:06]

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:421: const
std::map<std::string, std::string>& settings,
On 2016/11/02 21:10:01, altimin wrote:
> On 2016/11/02 17:53:48, alex clarke wrote:
> > Are you aware of base::DictionaryValue?
> 
> Could you clarify this, please?
> I'm aware of base::DictionaryValue, but Finch API returns map of strings.

I was thinking this rather steps on the toes of base::DictionaryValue, however
you are right that the API is at 'fault'.

[altimin, 2016-11-04 10:41:25]

PTAL

https://codereview.chromium.org/2471153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:420: void
MaybeSetParameterFromMap(
On 2016/11/03 13:21:15, Sami wrote:
> MaybeSetFloat/DoubleParameterFromMap?

Done.

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
content/public/common/web_preferences.cc:185:
expensive_background_throttling_initial_budget(1.0f),
On 2016/11/03 13:21:15, Sami wrote:
> Should all of these be negative by default so the scheduler gets to choose the
> defaults?

In scheduler these values are base::Optional<base::TimeDelta>. Given that we
want to be able to disable these settings via Finch the best way IMO is to set a
sensible default here and treat negative values as base::nullopt and do not
override it in the scheduler.

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/20001/content/public/common/w...
content/public/common/web_preferences.h:213: // Specifies aggressiveness of
background tab throttling.
On 2016/11/03 13:21:15, Sami wrote:
> Please mention that these are in seconds. Ideally the names would say that too
> but they are getting long.

Done.

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right):

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/DEPS:7:
"+web/WebSettingsImpl.h",
On 2016/11/03 13:21:15, Sami wrote:
> I don't think we can depend on web/ here. Another option would be to add a
> scheduler API for configuring these settings.

Done.

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2471153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:232:
// Disable throttling because page is visible by default.
On 2016/11/03 13:21:15, Sami wrote:
> What do you mean by default? We're checking the current visibility state here,
> right?

Old comment, fixed.

[altimin, 2016-11-04 14:03:31]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 14:03:44]

Dry run: 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, 2016-11-04 14:06:26]

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

[commit-bot: I haz the power, 2016-11-04 14:06:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[altimin, 2016-11-04 14:29:34]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[Sami, 2016-11-04 16:50:28]

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
content/public/common/web_preferences.cc:184:
expensive_background_throttling_cpu_budget(-1.0f),
Agreed offline that we'll try to find a way to keep the scheduler in charge of
defaults while letting these settings override things only when needed.

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
content/public/common/web_preferences.h:214: // cpu_budget is given in
percentages, other values are in seconds.
nit: Capitalize CPU.

https://codereview.chromium.org/2471153002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScheduler.h (right):

https://codereview.chromium.org/2471153002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScheduler.h:34: class
BLINK_PLATFORM_EXPORT WebSchedulerSettings {
Instead of adding this generic interface, how would you feel about just adding a
configureBackgroundThrottling() method that takes all four parameters? The
reason I think I'd prefer that is that I don't want to introduce a new dumping
ground for settings but rather something more narrowly focused that we can blow
away once we're done with the Finch trials. WDYT?

[altimin, 2016-11-07 15:19:49]

PTAL

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
content/public/common/web_preferences.cc:184:
expensive_background_throttling_cpu_budget(-1.0f),
On 2016/11/04 16:50:28, Sami wrote:
> Agreed offline that we'll try to find a way to keep the scheduler in charge of
> defaults while letting these settings override things only when needed.

Done.

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/60001/content/public/common/w...
content/public/common/web_preferences.h:214: // cpu_budget is given in
percentages, other values are in seconds.
On 2016/11/04 16:50:28, Sami wrote:
> nit: Capitalize CPU.

Done.

https://codereview.chromium.org/2471153002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScheduler.h (right):

https://codereview.chromium.org/2471153002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScheduler.h:34: class
BLINK_PLATFORM_EXPORT WebSchedulerSettings {
On 2016/11/04 16:50:28, Sami wrote:
> Instead of adding this generic interface, how would you feel about just adding
a
> configureBackgroundThrottling() method that takes all four parameters? The
> reason I think I'd prefer that is that I don't want to introduce a new dumping
> ground for settings but rather something more narrowly focused that we can
blow
> away once we're done with the Finch trials. WDYT?

Per offline discussion, renamed to WebViewSchedulerSettings and moved to
WebViewScheduler.

[Sami, 2016-11-07 16:33:42]

Thanks, I think the plumbing looks good now.
third_party/WebKit/Source/platform/scheduler/ lgtm with a handful of nits.

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_web_scheduler_impl.h
(right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_web_scheduler_impl.h:11:
class WebSettingsImpl;
Unneeded?

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:27:
// Values coming from WebSchedulerSettings are interpreted as follows:
WebViewSchedulerSettings

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h
(left):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:70:
private:
I think we want to keep this :)

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (left):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:752: const
std::unique_ptr<WebViewScheduler> m_scheduler;
Did you mean to change this?

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebViewScheduler.h (right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebViewScheduler.h:20: class
BLINK_PLATFORM_EXPORT WebViewSchedulerSettings {
nit: To improve readability how about just calling this |Settings|?

[altimin, 2016-11-07 17:01:33]

PTAL
Given that jochen@ is out,
+clamy@ for content
+foolip@ for blink

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_web_scheduler_impl.h
(right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_web_scheduler_impl.h:11:
class WebSettingsImpl;
On 2016/11/07 16:33:41, Sami wrote:
> Unneeded?

Done.

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:27:
// Values coming from WebSchedulerSettings are interpreted as follows:
On 2016/11/07 16:33:41, Sami wrote:
> WebViewSchedulerSettings

Done.

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h
(left):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:70:
private:
On 2016/11/07 16:33:41, Sami wrote:
> I think we want to keep this :)

Nope, we have "private:" four lines above, this a result of a merge conflict.

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (left):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:752: const
std::unique_ptr<WebViewScheduler> m_scheduler;
On 2016/11/07 16:33:42, Sami wrote:
> Did you mean to change this?

It was needed in the first version, not anymore.

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebViewScheduler.h (right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebViewScheduler.h:20: class
BLINK_PLATFORM_EXPORT WebViewSchedulerSettings {
On 2016/11/07 16:33:42, Sami wrote:
> nit: To improve readability how about just calling this |Settings|?

I considered doing this, but given that WebViewImpl inherits this interface, it
will interfere with Settings from WebKit (core/frame/Settings.h), which is ugly.

[altimin, 2016-11-07 17:02:06]

altimin@chromium.org changed reviewers:
+ clamy@chromium.org, foolip@chromium.org

[altimin, 2016-11-07 17:02:07]

+clamy@ for content
+foolip@ for blink

[Sami, 2016-11-07 17:08:35]

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebViewScheduler.h (right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebViewScheduler.h:20: class
BLINK_PLATFORM_EXPORT WebViewSchedulerSettings {
On 2016/11/07 17:01:33, altimin wrote:
> On 2016/11/07 16:33:42, Sami wrote:
> > nit: To improve readability how about just calling this |Settings|?
> 
> I considered doing this, but given that WebViewImpl inherits this interface,
it
> will interfere with Settings from WebKit (core/frame/Settings.h), which is
ugly.

Hmm, it will inherit WebViewScheduler::Settings, not Settings, so there
shouldn't be any confusion?

[altimin, 2016-11-07 17:10:29]

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebViewScheduler.h (right):

https://codereview.chromium.org/2471153002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebViewScheduler.h:20: class
BLINK_PLATFORM_EXPORT WebViewSchedulerSettings {
On 2016/11/07 17:08:34, Sami wrote:
> On 2016/11/07 17:01:33, altimin wrote:
> > On 2016/11/07 16:33:42, Sami wrote:
> > > nit: To improve readability how about just calling this |Settings|?
> > 
> > I considered doing this, but given that WebViewImpl inherits this interface,
> it
> > will interfere with Settings from WebKit (core/frame/Settings.h), which is
> ugly.
> 
> Hmm, it will inherit WebViewScheduler::Settings, not Settings, so there
> shouldn't be any confusion?

My understanding is that Settings inside WebViewScheduler will start to refer to
WebViewScheduler::Settings, which will cause confusion. (I tried and experienced
that).

[Sami, 2016-11-07 17:25:30]

On 2016/11/07 17:10:29, altimin wrote:
> My understanding is that Settings inside WebViewScheduler will start to refer
to
> WebViewScheduler::Settings, which will cause confusion. (I tried and
experienced
> that).

Hmm okay. I guess we could have some |using| trickery to avoid that, but let's
go with what you have here instead.

[ojan, 2016-11-08 19:41:14]

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

[ojan, 2016-11-08 19:41:16]

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:420: void
MaybeSetFloatParameterFromMap(
Drive-by pet-peeve nit: I don't feel like this "Maybe" is very valuable. I have
a pet-peeve against our increasing overuse of Maybe in method names, so feel
free to ignore me. Every method with an if-statement in it could be a Maybe
method. Makes the code bloated and hard to read. Better to just never use that
term IMO. SetFloatParameterFromMap is plenty clear IMO. :)

I'd make the same argument about MaybeInitializeBackgroundTimeBudgetPool.
InitializeBackgroundTimeBudgetPool is plenty clear.

[foolip, 2016-11-09 11:08:18]

third_party/WebKit/ LGTM outside of
	third_party/WebKit/Source/platform/scheduler/, where I didn't look but
skyostil@ did.

https://codereview.chromium.org/2471153002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebScheduler.h (left):

https://codereview.chromium.org/2471153002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebScheduler.h:17: 
OK if clang-format doesn't care, but blank line after namespace is all I've
seen. (Selective memory, perhaps.)

[clamy, 2016-11-09 14:28:51]

Thanks! A few comments below.

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:595:
variations::GetVariationParamsByFeature(
We've tried to avoid using things from components inside content/ unless there's
a good reason. What's the reasoning behind this one?

If possible, I would prefer the setting of preferences to match what's done with
all other preferences, for consistency.

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
content/public/common/web_preferences.h:214: // CPU budget is given in
percentages, other values are in seconds.
Other values means just max_delay right? I'm thinking the way the comment is
phrased might be confusing.

[altimin, 2016-11-09 14:46:14]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 14:46:27]

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

[altimin, 2016-11-09 14:46:30]

PTAL

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:420: void
MaybeSetFloatParameterFromMap(
On 2016/11/08 19:41:15, ojan wrote:
> Drive-by pet-peeve nit: I don't feel like this "Maybe" is very valuable. I
have
> a pet-peeve against our increasing overuse of Maybe in method names, so feel
> free to ignore me. Every method with an if-statement in it could be a Maybe
> method. Makes the code bloated and hard to read. Better to just never use that
> term IMO. SetFloatParameterFromMap is plenty clear IMO. :)
> 
> I'd make the same argument about MaybeInitializeBackgroundTimeBudgetPool.
> InitializeBackgroundTimeBudgetPool is plenty clear.

Done.

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:595:
variations::GetVariationParamsByFeature(
On 2016/11/09 14:28:51, clamy wrote:
> We've tried to avoid using things from components inside content/ unless
there's
> a good reason. What's the reasoning behind this one?
> 
> If possible, I would prefer the setting of preferences to match what's done
with
> all other preferences, for consistency.

I didn't find any other cases of plumbing parameters from variations to webkit:
all cases that I know of are binary on/off features which are implemented with
switches and WebRuntimeFeatures. So almost all other preferences here use
switches.

I believe that it's right to use variations component from here, for example,
because GetV8Cache options check for field trial settings with
base::FieldTrialList and variations::GetVariationParams is an extension of that
mechanism.

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
content/public/common/web_preferences.h:214: // CPU budget is given in
percentages, other values are in seconds.
On 2016/11/09 14:28:51, clamy wrote:
> Other values means just max_delay right? I'm thinking the way the comment is
> phrased might be confusing.

No, "other values" means everything which is not cpu_budget: initial_budget,
max_budget and max_delay.

https://codereview.chromium.org/2471153002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebScheduler.h (left):

https://codereview.chromium.org/2471153002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebScheduler.h:17: 
On 2016/11/09 11:08:18, foolip wrote:
> OK if clang-format doesn't care, but blank line after namespace is all I've
> seen. (Selective memory, perhaps.)

Done.

[commit-bot: I haz the power, 2016-11-09 14:48:18]

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

[commit-bot: I haz the power, 2016-11-09 14:48:19]

Dry run: 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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[clamy, 2016-11-09 15:27:07]

Thanks! One more comment below and this should be good.

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2471153002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:595:
variations::GetVariationParamsByFeature(
On 2016/11/09 14:46:29, altimin wrote:
> On 2016/11/09 14:28:51, clamy wrote:
> > We've tried to avoid using things from components inside content/ unless
> there's
> > a good reason. What's the reasoning behind this one?
> > 
> > If possible, I would prefer the setting of preferences to match what's done
> with
> > all other preferences, for consistency.
> 
> I didn't find any other cases of plumbing parameters from variations to
webkit:
> all cases that I know of are binary on/off features which are implemented with
> switches and WebRuntimeFeatures. So almost all other preferences here use
> switches.
> 
> I believe that it's right to use variations component from here, for example,
> because GetV8Cache options check for field trial settings with
> base::FieldTrialList and variations::GetVariationParams is an extension of
that
> mechanism. 

Acknowledged.

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
content/public/common/web_preferences.h:214: // CPU budget is given in
percentages, other values are in seconds.
On 2016/11/09 14:46:29, altimin wrote:
> On 2016/11/09 14:28:51, clamy wrote:
> > Other values means just max_delay right? I'm thinking the way the comment is
> > phrased might be confusing.
> 
> No, "other values" means everything which is not cpu_budget: initial_budget,
> max_budget and max_delay.

Ok so that's not super clear. Can you s/CPU
budget/|expensive_background_throttling_cpu_budget| so that there is no
confusion?

[altimin, 2016-11-09 15:31:28]

PTAL

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2471153002/diff/100001/content/public/common/...
content/public/common/web_preferences.h:214: // CPU budget is given in
percentages, other values are in seconds.
On 2016/11/09 15:27:07, clamy wrote:
> On 2016/11/09 14:46:29, altimin wrote:
> > On 2016/11/09 14:28:51, clamy wrote:
> > > Other values means just max_delay right? I'm thinking the way the comment
is
> > > phrased might be confusing.
> > 
> > No, "other values" means everything which is not cpu_budget: initial_budget,
> > max_budget and max_delay.
> 
> Ok so that's not super clear. Can you s/CPU
> budget/|expensive_background_throttling_cpu_budget| so that there is no
> confusion?

Sure!

[altimin, 2016-11-09 15:31:30]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 15:31:48]

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

[Mike West, 2016-11-09 15:36:23]

Adding members to the IPC's struct LGTM.

[clamy, 2016-11-09 15:38:49]

Thanks! Lgtm.

[commit-bot: I haz the power, 2016-11-09 18:13:13]

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

[commit-bot: I haz the power, 2016-11-09 18:13:15]

Dry run: This issue passed the CQ dry run.

[altimin, 2016-11-09 18:15:26]

The CQ bit was checked by altimin@chromium.org

[altimin, 2016-11-09 18:15:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2471153002/#ps180001
(title: "Fix test and address comments")

[commit-bot: I haz the power, 2016-11-09 18:15:45]

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, 2016-11-09 18:24:15]

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

[commit-bot: I haz the power, 2016-11-09 18:24:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[altimin, 2016-11-09 19:16:47]

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

[altimin, 2016-11-09 19:17:51]

+asvitkine@ could you please approve dependency on components/variations?

[Alexei Svitkine (slow), 2016-11-09 19:24:38]

lgtm if content owners are ok with the dependency

[altimin, 2016-11-09 19:38:20]

The CQ bit was checked by altimin@chromium.org

[commit-bot: I haz the power, 2016-11-09 19:38:41]

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, 2016-11-09 19:45:43]

Description was changed from

==========
[scheduler] Use Finch to control background throttling.

BUG=639852
==========

to

==========
[scheduler] Use Finch to control background throttling.

BUG=639852
==========

[commit-bot: I haz the power, 2016-11-09 19:45:46]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-09 19:59:10]

Description was changed from

==========
[scheduler] Use Finch to control background throttling.

BUG=639852
==========

to

==========
[scheduler] Use Finch to control background throttling.

BUG=639852

Committed: https://crrev.com/56bb594b1b341695c09b9041cdf02a76af306cc3
Cr-Commit-Position: refs/heads/master@{#431013}
==========

[commit-bot: I haz the power, 2016-11-09 19:59:12]

Patchset 10 (id:??) landed as
https://crrev.com/56bb594b1b341695c09b9041cdf02a76af306cc3
Cr-Commit-Position: refs/heads/master@{#431013}