Move Task Scheduler Initialization From chrome/browser to Content

Move Task Scheduler Initialization From chrome/browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
Committed: https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1
Cr-Commit-Position: refs/heads/master@{#437452}

[robliao, 2016-11-30 22:19:57]

robliao@chromium.org changed reviewers:
+ fdoray@chromium.org, gab@chromium.org

[robliao, 2016-11-30 22:23:29]

Description was changed from

==========
Move Task Scheduler Initialization From Browser to Content

Content appears to actually be the owner of the process whe running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

to

==========
Move Task Scheduler Initialization From Browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

[robliao, 2016-11-30 22:23:35]

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

[commit-bot: I haz the power, 2016-11-30 22:24:06]

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-30 22:27:25]

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

[commit-bot: I haz the power, 2016-11-30 22:27:26]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-11-30 22:36:58]

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1200: // IMPORTANT
Does this comment apply anymore? i.e. it's relevant to this whole method but the
things at the end here are no longer special?

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1207: // initialize field trials and setup
metrics. The field trials are needed by
No longer sets up metrics

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
File components/task_scheduler_util/initialization_util.cc (right):

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
components/task_scheduler_util/initialization_util.cc:173: void
InitializeDefaultBrowserTaskScheduler() {
This is still used by iOS I guess?

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
components/task_scheduler_util/initialization_util.cc:210: if
(!variations::GetVariationParams(kFieldTrialName, &variation_params))
I think we should have defaults for individual fields instead of a big default
when we have nothing (i.e. we shouldn't have to specify fields for things that
are enabled by default when we want to override a field that isn't on by default
-- i.e. checked in chromium config should only have to enable redirection, not
re-specify defaults already hardcoded in client)

Basically, I'd like to have what we consider the best client-side defaults be
hardcoded in only one place in our codebase, right now I can think of 3
(content_browser_client.cc, task_scheduler_util.cc and the Chromium config?)

https://codereview.chromium.org/2539263003/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/2539263003/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.cc:476:
DCHECK(params_vector->size() == WorkerPoolType::WORKER_POOL_COUNT);
DCHECK_EQ

[robliao, 2016-11-30 22:46:57]

Responding to the larger question first. Will address the rest of the comments
once that's resolved.

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
File components/task_scheduler_util/initialization_util.cc (right):

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
components/task_scheduler_util/initialization_util.cc:173: void
InitializeDefaultBrowserTaskScheduler() {
On 2016/11/30 22:36:58, gab wrote:
> This is still used by iOS I guess?

Yep! This goes away when iOS moves, which will be after this CL.

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
components/task_scheduler_util/initialization_util.cc:210: if
(!variations::GetVariationParams(kFieldTrialName, &variation_params))
On 2016/11/30 22:36:58, gab wrote:
> I think we should have defaults for individual fields instead of a big default
> when we have nothing (i.e. we shouldn't have to specify fields for things that
> are enabled by default when we want to override a field that isn't on by
default
> -- i.e. checked in chromium config should only have to enable redirection, not
> re-specify defaults already hardcoded in client)
> 
> Basically, I'd like to have what we consider the best client-side defaults be
> hardcoded in only one place in our codebase, right now I can think of 3
> (content_browser_client.cc, task_scheduler_util.cc and the Chromium config?)

I believe this is the nature of how things are structured.

content_browser_client is used as the default implementation for everyone. As
such, it needs to have its own defaults because those dependencies must be
hermetic or live in base.

chrome_content_browser_client is able to use variations, and so it defers to
that. When variations fails, we need to fall back onto something. From
components, we have to be hermetic, and so another set of defaults lives here.

This stuff lives in components so we can share between iOS and everywhere else.

From what I can see at the moment, the only way to have a one-stop-shop is to
put all of this in base, which seems strange.

And if we're going to do the hardcoding, might as well specify all of the
parameters up front so that it's easy to tell what is actually getting used.

Thoughts?

[gab, 2016-11-30 23:10:38]

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
File components/task_scheduler_util/initialization_util.cc (right):

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
components/task_scheduler_util/initialization_util.cc:210: if
(!variations::GetVariationParams(kFieldTrialName, &variation_params))
On 2016/11/30 22:46:57, robliao wrote:
> On 2016/11/30 22:36:58, gab wrote:
> > I think we should have defaults for individual fields instead of a big
default
> > when we have nothing (i.e. we shouldn't have to specify fields for things
that
> > are enabled by default when we want to override a field that isn't on by
> default
> > -- i.e. checked in chromium config should only have to enable redirection,
not
> > re-specify defaults already hardcoded in client)
> > 
> > Basically, I'd like to have what we consider the best client-side defaults
be
> > hardcoded in only one place in our codebase, right now I can think of 3
> > (content_browser_client.cc, task_scheduler_util.cc and the Chromium config?)
> 
> I believe this is the nature of how things are structured.
> 
> content_browser_client is used as the default implementation for everyone. As
> such, it needs to have its own defaults because those dependencies must be
> hermetic or live in base.
> 
> chrome_content_browser_client is able to use variations, and so it defers to
> that. When variations fails, we need to fall back onto something. From
> components, we have to be hermetic, and so another set of defaults lives here.
> 
> This stuff lives in components so we can share between iOS and everywhere
else.
> 
> From what I can see at the moment, the only way to have a one-stop-shop is to
> put all of this in base, which seems strange.
> 
> And if we're going to do the hardcoding, might as well specify all of the
> parameters up front so that it's easy to tell what is actually getting used.
> 
> Thoughts?

If we fail to get variations then ChromeContentBrowserClient can defer to
ContentBrowserClient instead of having its own fallback default?

I guess that doesn't work for iOS per lack of content::..? But then iOS probably
wants different defaults anyways so maybe it doesn't matter..?

As for the configs, I really think they should only need to specify the things
they want to override, not all or nothing. Or they become very wordy copies of
local defaults and it also stays very wordy to drive variations from the
cmd-line.

[robliao, 2016-11-30 23:45:22]

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
File components/task_scheduler_util/initialization_util.cc (right):

https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
components/task_scheduler_util/initialization_util.cc:210: if
(!variations::GetVariationParams(kFieldTrialName, &variation_params))
On 2016/11/30 23:10:38, gab wrote:
> On 2016/11/30 22:46:57, robliao wrote:
> > On 2016/11/30 22:36:58, gab wrote:
> > > I think we should have defaults for individual fields instead of a big
> default
> > > when we have nothing (i.e. we shouldn't have to specify fields for things
> that
> > > are enabled by default when we want to override a field that isn't on by
> > default
> > > -- i.e. checked in chromium config should only have to enable redirection,
> not
> > > re-specify defaults already hardcoded in client)
> > > 
> > > Basically, I'd like to have what we consider the best client-side defaults
> be
> > > hardcoded in only one place in our codebase, right now I can think of 3
> > > (content_browser_client.cc, task_scheduler_util.cc and the Chromium
config?)
> > 
> > I believe this is the nature of how things are structured.
> > 
> > content_browser_client is used as the default implementation for everyone.
As
> > such, it needs to have its own defaults because those dependencies must be
> > hermetic or live in base.
> > 
> > chrome_content_browser_client is able to use variations, and so it defers to
> > that. When variations fails, we need to fall back onto something. From
> > components, we have to be hermetic, and so another set of defaults lives
here.
> > 
> > This stuff lives in components so we can share between iOS and everywhere
> else.
> > 
> > From what I can see at the moment, the only way to have a one-stop-shop is
to
> > put all of this in base, which seems strange.
> > 
> > And if we're going to do the hardcoding, might as well specify all of the
> > parameters up front so that it's easy to tell what is actually getting used.
> > 
> > Thoughts?
> 
> If we fail to get variations then ChromeContentBrowserClient can defer to
> ContentBrowserClient instead of having its own fallback default?
> 
> I guess that doesn't work for iOS per lack of content::..? But then iOS
probably
> wants different defaults anyways so maybe it doesn't matter..?
> 
> As for the configs, I really think they should only need to specify the things
> they want to override, not all or nothing. Or they become very wordy copies of
> local defaults and it also stays very wordy to drive variations from the
> cmd-line.
> 
The main reason for this is that the failure path:

- If getting the variations fails, use the default.
- If the variations we got is invalid for any reason, use the default.

To avoid bouncing back and forth between content and components on success and
failure, we do all of that work in components.

On configs: I'm not sure what this would look like. The worker pool
configuration set tends to be atomic. We wouldn't want to fall back on failure
for say BackgroundFileIO and then use the config that happened to look good for
Background for example. We wouldn't be able to easily dictate the valid
configurations out there in the event of corruption.

Redirection can plausibly be more piecemeal, but those are already true/false.

[chromium-reviews, 2016-12-01 01:03:35]

I did mean piecemeal for redirection (not specifying a partial set of
pools). I want to be able to enable redirection without being forced to
re-specify the whole configuration of pools.

On Wed, Nov 30, 2016 at 6:45 PM <robliao@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
> File components/task_scheduler_util/initialization_util.cc (right):
>
>
>
https://codereview.chromium.org/2539263003/diff/1/components/task_scheduler_u...
> components/task_scheduler_util/initialization_util.cc:210: if
> (!variations::GetVariationParams(kFieldTrialName, &variation_params))
> On 2016/11/30 23:10:38, gab wrote:
> > On 2016/11/30 22:46:57, robliao wrote:
> > > On 2016/11/30 22:36:58, gab wrote:
> > > > I think we should have defaults for individual fields instead of a
> big
> > default
> > > > when we have nothing (i.e. we shouldn't have to specify fields for
> things
> > that
> > > > are enabled by default when we want to override a field that isn't
> on by
> > > default
> > > > -- i.e. checked in chromium config should only have to enable
> redirection,
> > not
> > > > re-specify defaults already hardcoded in client)
> > > >
> > > > Basically, I'd like to have what we consider the best client-side
> defaults
> > be
> > > > hardcoded in only one place in our codebase, right now I can think
> of 3
> > > > (content_browser_client.cc, task_scheduler_util.cc and the
> Chromium config?)
> > >
> > > I believe this is the nature of how things are structured.
> > >
> > > content_browser_client is used as the default implementation for
> everyone. As
> > > such, it needs to have its own defaults because those dependencies
> must be
> > > hermetic or live in base.
> > >
> > > chrome_content_browser_client is able to use variations, and so it
> defers to
> > > that. When variations fails, we need to fall back onto something.
> From
> > > components, we have to be hermetic, and so another set of defaults
> lives here.
> > >
> > > This stuff lives in components so we can share between iOS and
> everywhere
> > else.
> > >
> > > From what I can see at the moment, the only way to have a
> one-stop-shop is to
> > > put all of this in base, which seems strange.
> > >
> > > And if we're going to do the hardcoding, might as well specify all
> of the
> > > parameters up front so that it's easy to tell what is actually
> getting used.
> > >
> > > Thoughts?
> >
> > If we fail to get variations then ChromeContentBrowserClient can defer
> to
> > ContentBrowserClient instead of having its own fallback default?
> >
> > I guess that doesn't work for iOS per lack of content::..? But then
> iOS probably
> > wants different defaults anyways so maybe it doesn't matter..?
> >
> > As for the configs, I really think they should only need to specify
> the things
> > they want to override, not all or nothing. Or they become very wordy
> copies of
> > local defaults and it also stays very wordy to drive variations from
> the
> > cmd-line.
> >
> The main reason for this is that the failure path:
>
> - If getting the variations fails, use the default.
> - If the variations we got is invalid for any reason, use the default.
>
> To avoid bouncing back and forth between content and components on
> success and failure, we do all of that work in components.
>
> On configs: I'm not sure what this would look like. The worker pool
> configuration set tends to be atomic. We wouldn't want to fall back on
> failure for say BackgroundFileIO and then use the config that happened
> to look good for Background for example. We wouldn't be able to easily
> dictate the valid configurations out there in the event of corruption.
>
> Redirection can plausibly be more piecemeal, but those are already
> true/false.
>
> https://codereview.chromium.org/2539263003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fdoray, 2016-12-01 15:26:21]

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:3315:
task_scheduler_util::GetBrowserTaskSchedulerInitParameters(
InitializationParams vs InitParameters: Use the same wording everywhere?

https://codereview.chromium.org/2539263003/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2539263003/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1215: base::TaskScheduler* task_scheduler =
base::TaskScheduler::GetInstance();
There should always be a TaskScheduler.

[robliao, 2016-12-06 01:31:30]

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1200: // IMPORTANT
On 2016/11/30 22:36:58, gab wrote:
> Does this comment apply anymore? i.e. it's relevant to this whole method but
the
> things at the end here are no longer special?

Moved to top and reworded.

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1207: // initialize field trials and setup
metrics. The field trials are needed by
On 2016/11/30 22:36:58, gab wrote:
> No longer sets up metrics

Reworded. The only implication here is that we can now setup metrics but we wait
for PreMainMessageLoopRunImpl.

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2539263003/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:3315:
task_scheduler_util::GetBrowserTaskSchedulerInitParameters(
On 2016/12/01 15:26:21, fdoray wrote:
> InitializationParams vs InitParameters: Use the same wording everywhere?

Done.

https://codereview.chromium.org/2539263003/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2539263003/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1215: base::TaskScheduler* task_scheduler =
base::TaskScheduler::GetInstance();
On 2016/12/01 15:26:21, fdoray wrote:
> There should always be a TaskScheduler.

Done.

https://codereview.chromium.org/2539263003/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/2539263003/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.cc:476:
DCHECK(params_vector->size() == WorkerPoolType::WORKER_POOL_COUNT);
On 2016/11/30 22:36:58, gab wrote:
> DCHECK_EQ

Done.

[fdoray, 2016-12-06 21:14:12]

lgtm w/ comments

https://codereview.chromium.org/2539263003/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2539263003/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:1215:
base::TaskScheduler::GetInstance()->Shutdown();
TaskScheduler is initialized just before browser threads. Any reason why it's
not shutdown immediately after browser threads?

https://codereview.chromium.org/2539263003/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2539263003/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:796: 
// Comment?

https://codereview.chromium.org/2539263003/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:801: 
// Comment?

[robliao, 2016-12-07 00:57:33]

https://codereview.chromium.org/2539263003/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2539263003/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:1215:
base::TaskScheduler::GetInstance()->Shutdown();
On 2016/12/06 21:14:12, fdoray wrote:
> TaskScheduler is initialized just before browser threads. Any reason why it's
> not shutdown immediately after browser threads?

Looks like I was too aggressive with the lifetime as some of the components
above assume a no-threads world. I've moved it up to right after the
BrowserThreadImpl::ShutdownThreadPool.

https://codereview.chromium.org/2539263003/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2539263003/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:796: 
On 2016/12/06 21:14:12, fdoray wrote:
> // Comment?

Done.

https://codereview.chromium.org/2539263003/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:801: 
On 2016/12/06 21:14:12, fdoray wrote:
> // Comment?

Done.

[robliao, 2016-12-07 08:01:01]

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

[commit-bot: I haz the power, 2016-12-07 08:01:18]

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

[robliao, 2016-12-07 08:06:51]

Patchset #4 (id:60001) has been deleted

[robliao, 2016-12-07 08:06:57]

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

[commit-bot: I haz the power, 2016-12-07 08:07:09]

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-12-07 08:20:00]

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

[commit-bot: I haz the power, 2016-12-07 08:20:01]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2016-12-07 13:46:12]

lgtm w/ comments

https://codereview.chromium.org/2539263003/diff/80001/components/task_schedul...
File components/task_scheduler_util/initialization/browser_util.cc (right):

https://codereview.chromium.org/2539263003/diff/80001/components/task_schedul...
components/task_scheduler_util/initialization/browser_util.cc:87: 
#if defined(OS_IOS) the whole function, to get a build error it it's called on
another platform than iOS

https://codereview.chromium.org/2539263003/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2539263003/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:797: // Provides parameters for
initializing the global task scheduler.
If |params_vector| is empty or |index_to_traits_callback| is null when this
returns, default parameters are used.

[gab, 2016-12-07 16:05:20]

Very nice, this is great :), there is pretty much no duplication now (or did I
miss it?!). LGTM w/ naming nits.

https://codereview.chromium.org/2539263003/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/2539263003/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:324: void
PerformRedirectionToTaskScheduler() override;
PerformExperimentalTaskSchedulerRedirections() ?

(to implicitly say that it can be zero-to-many redirections and also that there
can be more -- e.g. once SWP is redirected by default I'd assume the redirection
to be kicked off from content directly)

https://codereview.chromium.org/2539263003/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2539263003/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:439: size_t
BrowserWorkerPoolIndexForTraits(const base::TaskTraits& traits) {
DefaultBrowserWorkerPoolIndexForTraits() or something to highlight beyond the
comment that it works in sync with the other method?

[robliao, 2016-12-07 22:49:35]

Patchset #5 (id:100001) has been deleted

[robliao, 2016-12-07 23:03:10]

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

[commit-bot: I haz the power, 2016-12-07 23:03:30]

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-12-07 23:07:08]

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

[commit-bot: I haz the power, 2016-12-07 23:07:09]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[robliao, 2016-12-08 00:34:33]

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

[commit-bot: I haz the power, 2016-12-08 00:35:08]

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

[robliao, 2016-12-08 01:04:20]

robliao@chromium.org changed reviewers:
+ jam@chromium.org

[robliao, 2016-12-08 01:04:22]

jam: Please review chrome/browser/* and content/* in this CL. Thanks!

https://codereview.chromium.org/2539263003/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/2539263003/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:324: void
PerformRedirectionToTaskScheduler() override;
On 2016/12/07 16:05:20, gab wrote:
> PerformExperimentalTaskSchedulerRedirections() ?
> 
> (to implicitly say that it can be zero-to-many redirections and also that
there
> can be more -- e.g. once SWP is redirected by default I'd assume the
redirection
> to be kicked off from content directly)

Done.

https://codereview.chromium.org/2539263003/diff/80001/components/task_schedul...
File components/task_scheduler_util/initialization/browser_util.cc (right):

https://codereview.chromium.org/2539263003/diff/80001/components/task_schedul...
components/task_scheduler_util/initialization/browser_util.cc:87: 
On 2016/12/07 13:46:12, fdoray wrote:
> #if defined(OS_IOS) the whole function, to get a build error it it's called on
> another platform than iOS

Done. For correctness, this is also propagated to initialization_util.

https://codereview.chromium.org/2539263003/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2539263003/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:439: size_t
BrowserWorkerPoolIndexForTraits(const base::TaskTraits& traits) {
On 2016/12/07 16:05:20, gab wrote:
> DefaultBrowserWorkerPoolIndexForTraits() or something to highlight beyond the
> comment that it works in sync with the other method?

Done.

https://codereview.chromium.org/2539263003/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2539263003/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:797: // Provides parameters for
initializing the global task scheduler.
On 2016/12/07 13:46:12, fdoray wrote:
> If |params_vector| is empty or |index_to_traits_callback| is null when this
> returns, default parameters are used.

Done.

[commit-bot: I haz the power, 2016-12-08 02:12:39]

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

[commit-bot: I haz the power, 2016-12-08 02:12:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2016-12-08 16:58:36]

why do we still need components/task_scheduler_util? I was under the impression
that it could go away after our discussion. Did I misunderstand.

minor nit: "Browser" is ambiguous, please use "chrome/browser" instead.

[robliao, 2016-12-08 17:58:03]

On 2016/12/08 16:58:36, jam wrote:
> why do we still need components/task_scheduler_util? I was under the
impression
> that it could go away after our discussion. Did I misunderstand.
> 
> minor nit: "Browser" is ambiguous, please use "chrome/browser" instead.

content/browser no longer needs components/task_scheduler_util, but it's still
useful for chrome/browser and ios/chrome to share the variations lookup and
processing code.

[robliao, 2016-12-08 22:07:01]

Description was changed from

==========
Move Task Scheduler Initialization From Browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

to

==========
Move Task Scheduler Initialization From chrome/browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

[robliao, 2016-12-08 22:07:26]

On 2016/12/08 17:58:03, robliao wrote:
> On 2016/12/08 16:58:36, jam wrote:
> > why do we still need components/task_scheduler_util? I was under the
> impression
> > that it could go away after our discussion. Did I misunderstand.
> > 
> > minor nit: "Browser" is ambiguous, please use "chrome/browser" instead.
> 
> content/browser no longer needs components/task_scheduler_util, but it's still
> useful for chrome/browser and ios/chrome to share the variations lookup and
> processing code.

Looks like my CL description got blown away in the fix. browser ->
chrome/browser fix applies.

[jam, 2016-12-08 23:51:59]

lgtm

[robliao, 2016-12-08 23:57:02]

On 2016/12/08 23:51:59, jam wrote:
> lgtm

Offline Discussion Notes:
* We want to minimize what we put in components if at all possible (there are a 
  lot of components already).
* components/task_scheduler_util exists solely to parse variations parameters
* At the conclusion of the experiment when we figure out the right sizing for 
  the scheduler worker pools, the variations experiment is no longer necessary, 
  and thus, components/task_scheduler_util is no longer necessary
* As a result, we will remove components/task_scheduler_util when the final
  parameters are set based off of results from the experiment.

[robliao, 2016-12-08 23:57:13]

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

[commit-bot: I haz the power, 2016-12-08 23:57:43]

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-12-09 00:24:48]

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

[commit-bot: I haz the power, 2016-12-09 00:24:49]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-12-09 00:30:13]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-12-09 00:30:13]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/2539263003/#ps140001
(title: "Rebase to bf8e2f1")

[commit-bot: I haz the power, 2016-12-09 00:30: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-12-09 03:35:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481243413253250,
"parent_rev": "cd97e055f062ac4157357a7911331aab231b0207", "commit_rev":
"cd8876fae74e1940ac4883a4c60c859fbd15ecc3"}

[commit-bot: I haz the power, 2016-12-09 03:36:18]

Description was changed from

==========
Move Task Scheduler Initialization From chrome/browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

to

==========
Move Task Scheduler Initialization From chrome/browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

[commit-bot: I haz the power, 2016-12-09 03:36:19]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-12-09 03:40:28]

Description was changed from

==========
Move Task Scheduler Initialization From chrome/browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052
==========

to

==========
Move Task Scheduler Initialization From chrome/browser to Content

Content appears to actually be the owner of the process running a
browser. As such, it stands to reason that it should be the one
initializing and controlling the lifetime of the task scheduler.

BUG=662052

Committed: https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1
Cr-Commit-Position: refs/heads/master@{#437452}
==========

[commit-bot: I haz the power, 2016-12-09 03:40:29]

Patchset 6 (id:??) landed as
https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1
Cr-Commit-Position: refs/heads/master@{#437452}