Set up the field trials before IOThread creation. We can't wait until after all the threads are c...

Set up the field trials before IOThread creation. We can't wait until after all the threads are created, because tasks on the IO thread reach out to IOThread which sets things up and depends on field trial values already being set. I've broken up the MetricsService initialization into two parts, one before the FILE thread starts and one after.

BUG=109863, 109864
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117313

[jam, 2012-01-11 20:26:49]

looks like I couldn't get away from setting up the IOThread before
metricsservice. tsan caught some races

[jam, 2012-01-11 20:39:01]

joi->jar since Joi is out now, and Jim you're the most familiar with this. for
background, I changed this in http://codereview.chromium.org/9150016/

[Jói, 2012-01-11 21:35:24]

I think it would be good to also get Jim to take a look, since the
last change I made that moved around the location of where metrics are
initialized turned out pretty badly (increased perceived crash rates).

I _think_ this change should be OK but if I'm the sole reviewer I'd
want to spend more time thinking about it, which I can't right now
(putting kids to bed).

This case was one of the several interleaved initialization things
that originally caused me to introduce the
PreStartThread/PostStartThread methods to the
content::BrowserMainParts interface.  I thought with the trial you
were able to remove, you might have no more interleaving, so I was
okay with removing them, but that turns out to have been wrong.  Now
I'm wondering if we should simply revert the part of
http://codereview.chromium.org/9150016/ that removes those methods
(and the Pre/PostStopThread methods) so that we can in fact have
initialization code explicitly "declare" its minimum dependencies by
running e.g. on PostStartThread(FILE) if that's exactly how much
initialization it needs.  Making the (Pre|Post)(Start|Stop)Thread
calls is not at all a burden on content, and the cognitive overhead of
having the methods in content::BrowserMainParts is very small, so if
we get more explicit startup sequencing by having them then maybe they
belong there.

Cheers,
Jói


On Wed, Jan 11, 2012 at 8:39 PM,  <jam@chromium.org> wrote:
> joi->jar since Joi is out now, and Jim you're the most familiar with this.
> for
> background, I changed this in http://codereview.chromium.org/9150016/
>
> http://codereview.chromium.org/9187011/

[jam, 2012-01-11 22:10:40]

On 2012/01/11 21:35:24, Jói wrote:
> I think it would be good to also get Jim to take a look, since the
> last change I made that moved around the location of where metrics are
> initialized turned out pretty badly (increased perceived crash rates).
> 
> I _think_ this change should be OK but if I'm the sole reviewer I'd
> want to spend more time thinking about it, which I can't right now
> (putting kids to bed).
> 
> This case was one of the several interleaved initialization things
> that originally caused me to introduce the
> PreStartThread/PostStartThread methods to the
> content::BrowserMainParts interface.  I thought with the trial you
> were able to remove, you might have no more interleaving, so I was
> okay with removing them, but that turns out to have been wrong.  Now
> I'm wondering if we should simply revert the part of
> http://codereview.chromium.org/9150016/ that removes those methods
> (and the Pre/PostStopThread methods) so that we can in fact have
> initialization code explicitly "declare" its minimum dependencies by
> running e.g. on PostStartThread(FILE) if that's exactly how much
> initialization it needs.  Making the (Pre|Post)(Start|Stop)Thread
> calls is not at all a burden on content, and the cognitive overhead of
> having the methods in content::BrowserMainParts is very small, so if
> we get more explicit startup sequencing by having them then maybe they
> belong there.

Thanks for the comments. A goal for me has always been to make the embedder APIs
as simple as possible. We should aim to hide as much of content's complexity
from chrome. The way I think about it is how would this code be structured if we
initially wrote chrome with the chrome/content separation? I doubt we would make
chrome know before/after each thread is created. I think doing that was helpful
when refactoring BrowserThread, as it's impossible to do everything at once. But
now that we have a chance to simplify this, we should take advantage of it.

> 
> Cheers,
> Jói
> 
> 
> On Wed, Jan 11, 2012 at 8:39 PM,  <mailto:jam@chromium.org> wrote:
> > joi->jar since Joi is out now, and Jim you're the most familiar with this.
> > for
> > background, I changed this in http://codereview.chromium.org/9150016/
> >
> > http://codereview.chromium.org/9187011/

[jar (doing other things), 2012-01-11 22:46:08]

lgtm

[jam, 2012-01-11 22:51:35]

hey Joi, I'm going to commit this after Jim's lgtm because i want to fix the
issue asap (not sure what other issues these race conditions can cause). but
definitely i'll respond to any comments you have in a followup change if
necessary. Thanks.

[Jói, 2012-01-11 23:45:42]

lgtm
 On Jan 11, 2012 10:51 PM, <jam@chromium.org> wrote:

> hey Joi, I'm going to commit this after Jim's lgtm because i want to fix
> the
> issue asap (not sure what other issues these race conditions can cause).
> but
> definitely i'll respond to any comments you have in a followup change if
> necessary. Thanks.
>
>
http://codereview.chromium.**org/9187011/<http://codereview.chromium.org/9187...
>