Lazily initialize MessageLoop for faster thread startup

Lazily initialize MessageLoop for faster thread startup

Summary of the change and background discussion:
https://docs.google.com/a/chromium.org/document/d/1o1vUUOjX3tC7pV5-nxchaGtElo4NwtzKOAb4Zm09ezw/edit#

This implements approach 1 in the doc.
Approach 2: https://codereview.chromium.org/1086663002/
Approach 3: https://codereview.chromium.org/1058603004/

Discussion thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/2t6lB8hUgYw

BUG=465458
Committed: https://crrev.com/f1f70cb5ebe24d85c575396f026a415ed0fe9afc
Cr-Commit-Position: refs/heads/master@{#328347}

[kinuko, 2015-03-16 13:06:50]

Patchset #1 (id:1) has been deleted

[kinuko, 2015-03-17 01:58:42]

Patchset #3 (id:60001) has been deleted

[kinuko, 2015-03-17 01:59:38]

Patchset #3 (id:80001) has been deleted

[kinuko, 2015-03-18 02:24:41]

Patchset #9 (id:220001) has been deleted

[DaleCurtis, 2015-03-18 22:04:27]

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

[DaleCurtis, 2015-03-18 22:04:28]

A bit more invasive than I'd hoped, but seems reasonable. Are you able to notice
any local startup improvements with this patch? Perhaps using the large profile
generator / lots of tabs?

https://codereview.chromium.org/1011683002/diff/300001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/300001/base/message_loop/mess...
base/message_loop/message_loop.h:144: // Note that only "PostTask" family
methods can be called and posted tasks
Seems error prone; what methods exactly shouldn't be called? We'll get flaky
failures if someone accidentally calls something else, no?

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread....
base/threading/thread.cc:97: scoped_ptr<MessageLoop::LazyInitOptions>
message_loop_options(
No need for a scoped_ptr allocation? Seems simple enough to allow pass-by-copy.

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread....
base/threading/thread.cc:225: {
You can combine running_ and start_event_ into a running_event_ and check the
signaled state.

https://codereview.chromium.org/1011683002/diff/300001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1011683002/diff/300001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:99: // We check COM initializer status right
after calling this, so wait here
Ah dang, I was hoping this one would be one we could remove since it's on the
BrowserMainLoop startup path.

[kinuko, 2015-03-19 03:08:16]

Thanks!

I'm also going to write a short doc that summarizes the change and discussion so
far so that we can involve right people & can give right context.

https://codereview.chromium.org/1011683002/diff/300001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/300001/base/message_loop/mess...
base/message_loop/message_loop.h:144: // Note that only "PostTask" family
methods can be called and posted tasks
On 2015/03/18 22:04:28, DaleCurtis wrote:
> Seems error prone; what methods exactly shouldn't be called? We'll get flaky
> failures if someone accidentally calls something else, no?

To be sure, almost all other methods (except for ScheduleWork()) are not
expected to be called on a different thread from the 'current' one, and until
this is called no task can run on the 'current' thread, so this shouldn't be
usually a problem (I think).

I elaborated the comment a little more.

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread....
base/threading/thread.cc:97: scoped_ptr<MessageLoop::LazyInitOptions>
message_loop_options(
On 2015/03/18 22:04:28, DaleCurtis wrote:
> No need for a scoped_ptr allocation? Seems simple enough to allow
pass-by-copy.

Not really, but I want to keep around this object in MessageLoop class only
until message loop is initialized on the target thread, so allocating this here
and just passing around the scoped_ptr seems most efficient. (Are you worrying
about slight jank by allocation?)

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread....
base/threading/thread.cc:225: {
On 2015/03/18 22:04:28, DaleCurtis wrote:
> You can combine running_ and start_event_ into a running_event_ and check the
> signaled state.

Sounds like a good idea, done.

https://codereview.chromium.org/1011683002/diff/300001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1011683002/diff/300001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:99: // We check COM initializer status right
after calling this, so wait here
On 2015/03/18 22:04:28, DaleCurtis wrote:
> Ah dang, I was hoping this one would be one we could remove since it's on the
> BrowserMainLoop startup path.

I guess we can probably fix this but with some refactoring in audio code. We'll
need another bug/CL for this anyway.

https://codereview.chromium.org/1011683002/diff/300001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:99: // We check COM initializer status right
after calling this, so wait here
On 2015/03/18 22:04:28, DaleCurtis wrote:
> Ah dang, I was hoping this one would be one we could remove since it's on the
> BrowserMainLoop startup path.

I haven't taken a deeper look but I guess we can probably fix this with some
refactoring in audio code, but so it'll need to be done in a separate bug/CL.

[kinuko, 2015-03-20 08:44:22]

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/300001/base/threading/thread....
base/threading/thread.cc:225: {
On 2015/03/19 03:08:16, kinuko wrote:
> On 2015/03/18 22:04:28, DaleCurtis wrote:
> > You can combine running_ and start_event_ into a running_event_ and check
the
> > signaled state.
> 
> Sounds like a good idea, done.

Hmm, replacing running_ with event_->IsSignaled() somehow breaks an Android
test, let me revert this change for now. (Instead I combined started_ into
start_event_)

[kinuko, 2015-03-23 13:39:14]

Patchset #16 (id:380001) has been deleted

[kinuko, 2015-03-23 13:39:32]

Patchset #16 (id:380001) has been deleted

[kinuko, 2015-03-23 14:59:15]

Patchset #16 (id:400001) has been deleted

[kinuko, 2015-03-25 12:57:25]

kinuko@chromium.org changed reviewers:
+ rvargas@chromium.org

[kinuko, 2015-03-25 13:00:17]

Hi rvargas@, would you be able to review this change and/or add appropriate
base/ reviewers?  Design doc and intent-to-implement links can be found in CL
description.

Thanks!

[rvargas (doing something else), 2015-03-25 18:12:47]

On 2015/03/25 13:00:17, kinuko wrote:
> Hi rvargas@, would you be able to review this change and/or add appropriate
> base/ reviewers?  Design doc and intent-to-implement links can be found in CL
> description.
> 
> Thanks!

I'll send comments today

[DaleCurtis, 2015-03-26 00:12:37]

dalecurtis@chromium.org changed reviewers:
+ tommi@chromium.org

[DaleCurtis, 2015-03-26 00:12:38]

https://codereview.chromium.org/1011683002/diff/440001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:99: // We check COM initializer status right
after calling this, so wait here
Can you elaborate on why this is necessary? CoreAudioUtil::IsSupported() was
previously running on the UI thread, what piece of code runs after this and
checks the COM status from the UI thread of the audio thread? 

...which as I think about it may not actually be correct. +tommi for his
recollection on why we initialize the device enumerator on the UI thread instead
of the audio thread. I seem to remember some race condition somewhere, but I
don't properly recall.

[kinuko, 2015-03-26 00:25:59]

On 2015/03/26 00:12:38, DaleCurtis wrote:
>
https://codereview.chromium.org/1011683002/diff/440001/media/audio/audio_mana...
> File media/audio/audio_manager_base.cc (right):
> 
>
https://codereview.chromium.org/1011683002/diff/440001/media/audio/audio_mana...
> media/audio/audio_manager_base.cc:99: // We check COM initializer status right
> after calling this, so wait here
> Can you elaborate on why this is necessary? CoreAudioUtil::IsSupported() was
> previously running on the UI thread, what piece of code runs after this and
> checks the COM status from the UI thread of the audio thread? 
> 
> ...which as I think about it may not actually be correct. +tommi for his
> recollection on why we initialize the device enumerator on the UI thread
instead
> of the audio thread. I seem to remember some race condition somewhere, but I
> don't properly recall.

Actually I haven't dig into this deeply enough. I noticed almost all audio tests
were failing and it looks it was because CHECK failure in COM initializer in
IsSupported(), so I looked into the code and thought it was the cause. (And I
also wondered why initialization on audio thread matters on UI thread.) I'll
look into this a bit more, but if I could get more expertise / insight here from
Windows/audio guru it's REALLY helpful.

[DaleCurtis, 2015-03-26 00:31:23]

Ah, I see, unfortunately it looks like those test logs have already been
deleted. I wonder if it has to do with the initialization of the
MediaStreamManager  or VideoCaptureDeviceManagers which happen soon after and
may be accessing the AudioManager from their own threads.

[rvargas (doing something else), 2015-03-26 02:19:28]

Sorry for the delay. It's taking me a long time to go over the code.

Mostly nits.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:30: // (i.e. created by
CreateForLazyInit()).
nit: try to keep cross-refs to a minimum (as in document what this function does
and not who call this). (and below)

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:31: static IncomingTaskQueue*
CreateForLazyInitMessageLoop(
return a scoped ptr (and same comment about this name suggesting that this is a
special case instead of the default)

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:64: void DidInitializeMessageLoop();
Maybe StartScheduling() ?

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:66: bool empty();
nit: doc

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.cc:302: pump_ =
lazy_init_options_->message_pump_factory.Run().Pass();
no need to Pass()

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.cc:305:
CreateMessagePumpForType(lazy_init_options_->message_loop_type).Pass();
no need to Pass()

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.h:182: static MessageLoop*
CreateForLazyInit(scoped_ptr<LazyInitOptions> options);
should return a scoped_ptr

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.h:182: static MessageLoop*
CreateForLazyInit(scoped_ptr<LazyInitOptions> options);
nit: Do we really need the "ForLazyInit" part?. This should be the default way
to create MessageLoops (except for tests or the main thread) and the name sort
of implies that this is a special case.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:54: bool lazy_init)
Maybe it's better to remove this argument and set valid_thread_id_ from the
factory method (if called from the proxied thread)

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.h:24: static MessageLoopProxyImpl*
Create(
return scoped ptr

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.h:31: static MessageLoopProxyImpl*
CreateForLazyInit(
nit: Make this the default (Create) and hint that the other method is the
uncommon one.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.h:46: void LazyInit();
Init() ?

https://codereview.chromium.org/1011683002/diff/440001/base/threading/platfor...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/threading/platfor...
base/threading/platform_thread_win.cc:119: if (out_thread_handle) {
nit: no need for {}

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:170: if (stopping_)
Now I see... there's a giant lock in Stop (Join), can we just use the event
without stopping_ and lock_?. I presume a test complained.

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:178: DCHECK(!!start_event_);
nit: shouldn't need !! (scoped_ptr is testable)

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:199: {
nit: remove this

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:207: scoped_ptr<MessageLoop>
message_loop(message_loop_);
Is this intentional?

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.h:116: bool WaitUntilThreadStarted();
nit: Add ForTest(/ing) to the name?

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.h:219: bool IsInRunLoop() const;
Why is this not a replacement for IsRunning()? And if it is, why change the
name? ...as in, IsInRunLoop tells me more about what to expect, but I don't
think the implementation fulfills my new expectation :)

[kinuko, 2015-04-09 16:06:03]

Patchset #18 (id:460001) has been deleted

[kinuko, 2015-04-10 04:34:46]

Patchset #19 (id:500001) has been deleted

[kinuko, 2015-04-11 16:02:57]

Patchset #19 (id:520001) has been deleted

[kinuko, 2015-04-12 02:43:12]

Patchset #19 (id:540001) has been deleted

[kinuko, 2015-04-12 02:51:04]

Patchset #20 (id:580001) has been deleted

[kinuko, 2015-04-12 02:51:31]

Patchset #20 (id:580001) has been deleted

[kinuko, 2015-04-12 02:51:45]

Patchset #19 (id:560001) has been deleted

[kinuko, 2015-04-12 02:51:59]

Patchset #18 (id:480001) has been deleted

[kinuko, 2015-04-13 02:03:00]

Resumed working on this, updated the code. I think the newer version's a bit
simpler. Since the discussion on chromium-dev hasn't settled yet I'll also
explorer another approach.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:30: // (i.e. created by
CreateForLazyInit()).
On 2015/03/26 02:19:26, rvargas (soon out of office) wrote:
> nit: try to keep cross-refs to a minimum (as in document what this function
does
> and not who call this). (and below)

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:31: static IncomingTaskQueue*
CreateForLazyInitMessageLoop(
On 2015/03/26 02:19:26, rvargas (out of office) wrote:
> return a scoped ptr (and same comment about this name suggesting that this is
a
> special case instead of the default)

Done. (This is ref-counted so returning scoped_refptr)

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:64: void DidInitializeMessageLoop();
On 2015/03/26 02:19:26, rvargas (soon out of office) wrote:
> Maybe StartScheduling() ? 

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:66: bool empty();
On 2015/03/26 02:19:26, rvargas (soon out of office) wrote:
> nit: doc

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.cc:302: pump_ =
lazy_init_options_->message_pump_factory.Run().Pass();
On 2015/03/26 02:19:26, rvargas (out of office) wrote:
> no need to Pass()

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.cc:305:
CreateMessagePumpForType(lazy_init_options_->message_loop_type).Pass();
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> no need to Pass()

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop.h:182: static MessageLoop*
CreateForLazyInit(scoped_ptr<LazyInitOptions> options);
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> nit: Do we really need the "ForLazyInit" part?. This should be the default way
> to create MessageLoops (except for tests or the main thread) and the name sort
> of implies that this is a special case.

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:54: bool lazy_init)
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> Maybe it's better to remove this argument and set valid_thread_id_ from the
> factory method (if called from the proxied thread)

Removed.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.h:24: static MessageLoopProxyImpl*
Create(
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> return scoped ptr

Removed this ctor.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.h:31: static MessageLoopProxyImpl*
CreateForLazyInit(
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> nit: Make this the default (Create) and hint that the other method is the
> uncommon one.

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.h:46: void LazyInit();
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> Init() ?

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/threading/platfor...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/threading/platfor...
base/threading/platform_thread_win.cc:119: if (out_thread_handle) {
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> nit: no need for {}

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:170: if (stopping_)
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> Now I see... there's a giant lock in Stop (Join), can we just use the event
> without stopping_ and lock_?. I presume a test complained.

Not only for testing but also in production code (e.g.
PrintJob::ControlledWorkerShutdown()) this behavior seems to be expected.  We
can probably change the callsite code but I'd like to keep the existing behavior
during the refactoring.

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:178: DCHECK(!!start_event_);
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> nit: shouldn't need !! (scoped_ptr is testable)

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:199: {
On 2015/03/26 02:19:27, rvargas (out of office) wrote:
> nit: remove this

Done.

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.cc:207: scoped_ptr<MessageLoop>
message_loop(message_loop_);
On 2015/03/26 02:19:28, rvargas (out of office) wrote:
> Is this intentional?

Sort of. (This needs to be freed at the end of main)

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1011683002/diff/440001/base/threading/thread....
base/threading/thread.h:116: bool WaitUntilThreadStarted();
On 2015/03/26 02:19:28, rvargas (out of office) wrote:
> nit: Add ForTest(/ing) to the name?

It's also internally called in StartAndWait().. added a bit more comment.

[kinuko, 2015-04-13 15:00:19]

Patchset #19 (id:620001) has been deleted

[kinuko, 2015-04-16 14:50:27]

kinuko@chromium.org changed reviewers:
+ mark@chromium.org, thakis@chromium.org

[kinuko, 2015-04-16 14:50:29]

Since Ricardo seems out-of-office let me add a few more base/ owners.  Mark,
Nico, (or Ricard if you're back) would you be able to review this or suggest a
good reviewer?

[Nico, 2015-04-16 15:52:16]

I'm looking now. (Well…I read the discussion thread and the design doc
while on the shuttle, and I'll look at the code once I'm at my desk in 20
min or so. Out of battery at the moment.)

On Thu, Apr 16, 2015 at 7:50 AM, <kinuko@chromium.org> wrote:

> Since Ricardo seems out-of-office let me add a few more base/ owners.
> Mark,
> Nico, (or Ricard if you're back) would you be able to review this or
> suggest a
> good reviewer?
>
> https://codereview.chromium.org/1011683002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2015-04-16 16:41:07]

https://codereview.chromium.org/1011683002/diff/440001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1011683002/diff/440001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:99: // We check COM initializer status right
after calling this, so wait here
On 2015/03/26 00:12:38, DaleCurtis wrote:
> Can you elaborate on why this is necessary? CoreAudioUtil::IsSupported() was
> previously running on the UI thread, what piece of code runs after this and
> checks the COM status from the UI thread of the audio thread? 
> 
> ...which as I think about it may not actually be correct. +tommi for his
> recollection on why we initialize the device enumerator on the UI thread
instead
> of the audio thread. I seem to remember some race condition somewhere, but I
> don't properly recall.

Did you have a chance to dig into this further? If not do you remember which
tests were failing? I'll try and patch it in today or tomorrow and see why this
is necessary.

[kinuko, 2015-04-17 16:11:10]

On 2015/04/16 16:41:07, DaleCurtis wrote:
> Did you have a chance to dig into this further? If not do you remember which
> tests were failing? I'll try and patch it in today or tomorrow and see why
this
> is necessary.

Sorry didn't have a chance yet. As far as I remember almost all audio tests were
failing but don't really remember the exact test names either :(
Just started try-job without the work-around for audio thread here:
https://codereview.chromium.org/1097753002/
hopefully it may have tests result before you do so.

[DaleCurtis, 2015-04-17 18:25:36]

On 2015/04/17 16:11:10, kinuko wrote:
> On 2015/04/16 16:41:07, DaleCurtis wrote:
> > Did you have a chance to dig into this further? If not do you remember which
> > tests were failing? I'll try and patch it in today or tomorrow and see why
> this
> > is necessary.
> 
> Sorry didn't have a chance yet. As far as I remember almost all audio tests
were
> failing but don't really remember the exact test names either :(
> Just started try-job without the work-around for audio thread here:
> https://codereview.chromium.org/1097753002/
> hopefully it may have tests result before you do so.

Looks like they passed :) I suspect I may have fixed this in the process of my
work for http://crbug.com/422522 -- which is still in progress.

[kinuko, 2015-04-17 22:01:24]

On 2015/04/17 18:25:36, DaleCurtis wrote:
> On 2015/04/17 16:11:10, kinuko wrote:
> > On 2015/04/16 16:41:07, DaleCurtis wrote:
> > > Did you have a chance to dig into this further? If not do you remember
which
> > > tests were failing? I'll try and patch it in today or tomorrow and see why
> > this
> > > is necessary.
> > 
> > Sorry didn't have a chance yet. As far as I remember almost all audio tests
> were
> > failing but don't really remember the exact test names either :(
> > Just started try-job without the work-around for audio thread here:
> > https://codereview.chromium.org/1097753002/
> > hopefully it may have tests result before you do so.
> 
> Looks like they passed :) I suspect I may have fixed this in the process of my
> work for http://crbug.com/422522 -- which is still in progress.

Aha that's super cool! Thanks for fixing the issue, I'll update this patch too
then :)

[kinuko, 2015-04-21 08:17:33]

Updated the code, as we no longer need to call StartAndWait() in audio code it's
now renamed to StartAndWaitForTesting() and only used by tests.

[kinuko, 2015-04-24 04:51:30]

kinuko@chromium.org changed reviewers:
+ danakj@chromium.org

[kinuko, 2015-04-24 04:55:06]

It looks I'm having a bit tough time to get reviewers who are not ooo ;) Adding
a few more base/ owners and top-level owners to to/cc, could some of you review
or can suggest a good reviewer?

(Sorry for spamming, feel free to remove yourself if you think you should not be
pinged by this change)

[danakj, 2015-04-24 20:54:40]

Before I go in too deep, some comments/questions on MessageLoop

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.cc:115: is_ready_for_scheduling_ = true;
Can you DCHECK !message_loop_scheduled_ here?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.cc:168: // Wake up the message loop.
Can you break this into a helper method so you can share the code (and more
especially the comments)?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:60: bool empty();
IsEmpty

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:102: bool is_ready_for_scheduling_ =
false;
please initialize it in the constructor where others are init

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:104: if
(options.message_pump_factory.is_null())
this was a little bit of work to parse in my head can you maybe use a temp to
make this read a little easier?

bool have_message_pump_factory = !options.message_pump_factory.is_null();
if (have_message_pump_factory)
  return CUSTOM;
return options.type;

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:167: Init();
it'd be nice if these other constructors set up an InitOptions and so just
delegated work to the first constructor.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:193: // This message loop is destructed before
we call Init.
Why do you do this early out path, what are you trying to avoid? You end up
avoiding things like the observers.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:114: using MessagePumpFactory =
Callback<scoped_ptr<MessagePump>()>;
nit: no () for the type name

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:118: // Specify the type of message loop.
Comments for "why" not for "what", this doesn't say anything that l-120 doesn't
already say.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:119: // This is ignored if
message_pump_factory.is_null() is false.
"This is only used if message_pump_factory is null" is easier to parse IMO.

Can you put it below message_pump_factory, since it depends on it?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:122: // Specify timer slack for the message
loop.
Can you say what timer slack is, instead of just repeating the name in the
comment?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:126: // If message_pump_factory.is_null() is
true, then a MessagePump appropriate
If message_pump_factory is null, then

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:140: explicit MessageLoop(const InitOptions&
options);
I'm a little surprised to see us using a struct here for just 3 arguments. Is
there a reason you went that route?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:143: // constructor is used only for testing.
No need to call Init if the
Do we need to keep these test-only constructors? Or could we add a test-only
subclass (in test code) and provide helper constructors there instead?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:183: void Init();
BindToCurrentThread() ?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:527: scoped_ptr<InitOptions> init_options_;
why scoped_ptr and not just a member? i'd prefer a bool initialized_ or
something instead. you could also guard it by DCHECK_IS_ON() if that helps.

[Nico, 2015-04-24 20:57:47]

Dana, I'm looking at this right now. (Was OOO sick the last 4 days, sorry)

On Fri, Apr 24, 2015 at 1:54 PM, <danakj@chromium.org> wrote:

> Before I go in too deep, some comments/questions on MessageLoop
>
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
> File base/message_loop/incoming_task_queue.cc (right):
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
> base/message_loop/incoming_task_queue.cc:115: is_ready_for_scheduling_ =
> true;
> Can you DCHECK !message_loop_scheduled_ here?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
> base/message_loop/incoming_task_queue.cc:168: // Wake up the message
> loop.
> Can you break this into a helper method so you can share the code (and
> more especially the comments)?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
> File base/message_loop/incoming_task_queue.h (right):
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
> base/message_loop/incoming_task_queue.h:60: bool empty();
> IsEmpty
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
> base/message_loop/incoming_task_queue.h:102: bool
> is_ready_for_scheduling_ = false;
> please initialize it in the constructor where others are init
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> File base/message_loop/message_loop.cc (right):
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.cc:104: if
> (options.message_pump_factory.is_null())
> this was a little bit of work to parse in my head can you maybe use a
> temp to make this read a little easier?
>
> bool have_message_pump_factory =
> !options.message_pump_factory.is_null();
> if (have_message_pump_factory)
>   return CUSTOM;
> return options.type;
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.cc:167: Init();
> it'd be nice if these other constructors set up an InitOptions and so
> just delegated work to the first constructor.
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.cc:193: // This message loop is
> destructed before we call Init.
> Why do you do this early out path, what are you trying to avoid? You end
> up avoiding things like the observers.
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> File base/message_loop/message_loop.h (right):
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:114: using MessagePumpFactory =
> Callback<scoped_ptr<MessagePump>()>;
> nit: no () for the type name
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:118: // Specify the type of message
> loop.
> Comments for "why" not for "what", this doesn't say anything that l-120
> doesn't already say.
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:119: // This is ignored if
> message_pump_factory.is_null() is false.
> "This is only used if message_pump_factory is null" is easier to parse
> IMO.
>
> Can you put it below message_pump_factory, since it depends on it?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:122: // Specify timer slack for the
> message loop.
> Can you say what timer slack is, instead of just repeating the name in
> the comment?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:126: // If
> message_pump_factory.is_null() is true, then a MessagePump appropriate
> If message_pump_factory is null, then
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:140: explicit MessageLoop(const
> InitOptions& options);
> I'm a little surprised to see us using a struct here for just 3
> arguments. Is there a reason you went that route?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:143: // constructor is used only for
> testing. No need to call Init if the
> Do we need to keep these test-only constructors? Or could we add a
> test-only subclass (in test code) and provide helper constructors there
> instead?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:183: void Init();
> BindToCurrentThread() ?
>
>
>
https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
> base/message_loop/message_loop.h:527: scoped_ptr<InitOptions>
> init_options_;
> why scoped_ptr and not just a member? i'd prefer a bool initialized_ or
> something instead. you could also guard it by DCHECK_IS_ON() if that
> helps.
>
> https://codereview.chromium.org/1011683002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-04-24 21:31:16]

Thanks for writing the comprehensive document. I'm not sure if any approach is
"obviously better", so going with this approach seems ok to me.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.cc:114: DCHECK(!is_ready_for_scheduling_);
DCHECK(!message_loop_scheduled_) (…right?)

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:60: bool empty();
style nit: lower_names() only for trivial methods. this one takes a lock, so
it's not trivial

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:102: bool is_ready_for_scheduling_ =
false;
nit: let's not mix member initialization style in one class

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:144: init_options_(new InitOptions(options)),
Maybe this should be passed in as scoped_ptr? That would save a copy.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:167: Init();
It feels strange that one constructor doesn't create MessagePump while the other
two do. The interface should probably be uniform in all modes. Maybe the other
two constructors could delegate to the "main" constructor with an appropriate
InitOptions object?

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:133: // message loop on a different thread from
the final 'current' thread,
This sounds confusing to me. Maybe

"It is valid to create a new message loop on one thread, and then pass it to the
thread where the message loop actually runs. The message loop's Init() method
must be called on the thread the message loop runs on, before calling Run()"?

Maybe also say that it's ok to post tasks before Init() is called?

Maybe Init() isn't the best name as it's possible to use the object before it's
called. Maybe BindToCurrentThread()?

(…why doesn't Run() just call Init()?)

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_pump_perftest.cc (left):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_pump_perftest.cc:23: namespace {
?

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:95: start_event_.reset(new WaitableEvent(false,
false));
Move this below the next if, then you don't need the reset() call there

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:112: return result;
nit: `return false;` reads 0.2s faster

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:166: AutoLock lock(lock_);
Why is this lock needed now when it wasn't before? Why isn't it needed to read
stopping_

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.h:229: mutable base::Lock lock_;
comment what this guards

[kinuko, 2015-04-27 16:13:28]

Patchset #22 (id:700001) has been deleted

[kinuko, 2015-04-27 16:36:05]

Nico, Dana thanks for taking a look!  Updated the patch.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.cc:114: DCHECK(!is_ready_for_scheduling_);
On 2015/04/24 21:31:15, Nico wrote:
> DCHECK(!message_loop_scheduled_) (…right?)

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.cc:115: is_ready_for_scheduling_ = true;
On 2015/04/24 20:54:39, danakj wrote:
> Can you DCHECK !message_loop_scheduled_ here?

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.cc:168: // Wake up the message loop.
On 2015/04/24 20:54:39, danakj wrote:
> Can you break this into a helper method so you can share the code (and more
> especially the comments)?

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:60: bool empty();
On 2015/04/24 21:31:15, Nico wrote:
> style nit: lower_names() only for trivial methods. this one takes a lock, so
> it's not trivial

Done. (Removed this method)

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:102: bool is_ready_for_scheduling_ =
false;
On 2015/04/24 21:31:15, Nico wrote:
> nit: let's not mix member initialization style in one class

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:104: if
(options.message_pump_factory.is_null())
On 2015/04/24 20:54:39, danakj wrote:
> this was a little bit of work to parse in my head can you maybe use a temp to
> make this read a little easier?
> 
> bool have_message_pump_factory = !options.message_pump_factory.is_null();
> if (have_message_pump_factory)
>   return CUSTOM;
> return options.type;

(This code is now gone)

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:144: init_options_(new InitOptions(options)),
On 2015/04/24 21:31:15, Nico wrote:
> Maybe this should be passed in as scoped_ptr? That would save a copy.

(This struct is now gone)

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:167: Init();
On 2015/04/24 21:31:15, Nico wrote:
> It feels strange that one constructor doesn't create MessagePump while the
other
> two do. The interface should probably be uniform in all modes. Maybe the other
> two constructors could delegate to the "main" constructor with an appropriate
> InitOptions object?

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.cc:193: // This message loop is destructed before
we call Init.
On 2015/04/24 20:54:39, danakj wrote:
> Why do you do this early out path, what are you trying to avoid? You end up
> avoiding things like the observers.

The code below is not really meaningful when this loop is destructed before it's
bound to a thread.  We could just go through the same code for such cases too, I
removed this early path.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:114: using MessagePumpFactory =
Callback<scoped_ptr<MessagePump>()>;
On 2015/04/24 20:54:39, danakj wrote:
> nit: no () for the type name

It's for Callback typedef, it seems we need it.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:133: // message loop on a different thread from
the final 'current' thread,
On 2015/04/24 21:31:16, Nico wrote:
> This sounds confusing to me. Maybe
> 
> "It is valid to create a new message loop on one thread, and then pass it to
the
> thread where the message loop actually runs. The message loop's Init() method
> must be called on the thread the message loop runs on, before calling Run()"?
> 
> Maybe also say that it's ok to post tasks before Init() is called?
> 
> Maybe Init() isn't the best name as it's possible to use the object before
it's
> called. Maybe BindToCurrentThread()?
> 
> (…why doesn't Run() just call Init()?)

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:140: explicit MessageLoop(const InitOptions&
options);
On 2015/04/24 20:54:39, danakj wrote:
> I'm a little surprised to see us using a struct here for just 3 arguments. Is
> there a reason you went that route?

I wanted to keep them together and wanted to make it clear that they're
internally passed to Init.  I removed this struct anyway as I refactored the
ctor a bit.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:143: // constructor is used only for testing.
No need to call Init if the
On 2015/04/24 20:54:39, danakj wrote:
> Do we need to keep these test-only constructors? Or could we add a test-only
> subclass (in test code) and provide helper constructors there instead?

We need these for now, while I agree that having these test-only ctors here is a
bit undesirable. I'd like to try cleaning up these in a follow-up CL as it needs
to touch really large # of files.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:183: void Init();
On 2015/04/24 20:54:40, danakj wrote:
> BindToCurrentThread() ?

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:527: scoped_ptr<InitOptions> init_options_;
On 2015/04/24 20:54:39, danakj wrote:
> why scoped_ptr and not just a member? i'd prefer a bool initialized_ or
> something instead. you could also guard it by DCHECK_IS_ON() if that helps.

Removed this field.

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_pump_perftest.cc (left):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_pump_perftest.cc:23: namespace {
On 2015/04/24 21:31:16, Nico wrote:
> ?

For friending (to call ScheduleWork).

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:95: start_event_.reset(new WaitableEvent(false,
false));
On 2015/04/24 21:31:16, Nico wrote:
> Move this below the next if, then you don't need the reset() call there

I think we need to do this here since it'll be racy otherwise (new thread may
try to touch start_event_ before we create)

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:112: return result;
On 2015/04/24 21:31:16, Nico wrote:
> nit: `return false;` reads 0.2s faster

Done.

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:166: AutoLock lock(lock_);
On 2015/04/24 21:31:16, Nico wrote:
> Why is this lock needed now when it wasn't before? Why isn't it needed to read
> stopping_

Actually I think we should have had this lock even without this change and I'm
not fully sure why we didn't have.

stopping_ seems to be touched only on the same thread that started the thread,
hence no locking.

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.h:229: mutable base::Lock lock_;
On 2015/04/24 21:31:16, Nico wrote:
> comment what this guards

Done.

[danakj, 2015-04-27 16:46:40]

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/680001/base/message_loop/mess...
base/message_loop/message_loop.h:143: // constructor is used only for testing.
No need to call Init if the
On 2015/04/27 16:36:05, kinuko wrote:
> On 2015/04/24 20:54:39, danakj wrote:
> > Do we need to keep these test-only constructors? Or could we add a test-only
> > subclass (in test code) and provide helper constructors there instead?
> 
> We need these for now, while I agree that having these test-only ctors here is
a
> bit undesirable. I'd like to try cleaning up these in a follow-up CL as it
needs
> to touch really large # of files.

Can you leave TODOs (pointing to a bug) for things you will do in the future?

[Nico, 2015-04-27 22:49:34]

Code looks fine. I have a few questions on how to best handle the
half-initialized state in the MessageLoop.h comments (I suppose this is the
issue that your document is about).

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:166: AutoLock lock(lock_);
On 2015/04/27 16:36:05, kinuko wrote:
> On 2015/04/24 21:31:16, Nico wrote:
> > Why is this lock needed now when it wasn't before? Why isn't it needed to
read
> > stopping_
> 
> Actually I think we should have had this lock even without this change and I'm
> not fully sure why we didn't have.
> 
> stopping_ seems to be touched only on the same thread that started the thread,
> hence no locking.

Maybe add a comment that says why accessing message_loop_ and stopping_ outside
of the lock is safe.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:56: // This should be called when it's
ready for scheduling work.
s/it/the message loop/

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop.h:122: // the message loop runs on, before
calling Run().
I'm not sure how to best communicate what can be done with a message loop in
this state. Maybe there should be a paragraph like "Only the Post*Task()
functions can be called on the MessageLoop while it's in this state, all other
functions may only be called after the loop has been bound to a thread"?

Or should we make the BindToCurrentThread() method private, make Thread a
friend, and assume that MessageLoops are only ever created by Thread? (This CL
couples them somewhat tightly.)

Then people reading this header would know that they're always talking  to
fully-constructed MessageLoop objects.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop.h:136: // testing. No need to call Init if the
message loop is constructed this way.
s/Init/BindToCurrentThread/

(I still think it's weird that this is different for different constructors, but
ok if you're going to do a cleanup pass afterwards)

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop.h:303: // Set the timer slack for this message
loop.
// Must not be called before BindToCurrentThread() was called.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop.h:355: void SetNestableTasksAllowed(bool
allowed);
// Must not be called before BindToCurrentThread() was called.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:30: DCHECK(!task.is_null()) <<
from_here.ToString();
DCHECK_NE(valid_thread_id_, kInvalidThreadId); 

(?)

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:38: DCHECK(!task.is_null()) <<
from_here.ToString();
DCHECK_NE(valid_thread_id_, kInvalidThreadId); 

(?)

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:42: bool
MessageLoopProxyImpl::RunsTasksOnCurrentThread() const {
DCHECK_NE(valid_thread_id_, kInvalidThreadId); 

(?)

https://codereview.chromium.org/1011683002/diff/720001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1011683002/diff/720001/base/threading/thread....
base/threading/thread.h:239: TimerSlack message_loop_timer_slack_;
Add a comment what this is for. ("Stores Options::timer_slack_ until the message
loop has been bound to a thread." or something like that)

[kinuko, 2015-04-28 15:36:27]

Patchset #23 (id:740001) has been deleted

[kinuko, 2015-04-28 15:43:26]

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1011683002/diff/680001/base/threading/thread....
base/threading/thread.cc:166: AutoLock lock(lock_);
On 2015/04/27 22:49:34, Nico wrote:
> On 2015/04/27 16:36:05, kinuko wrote:
> > On 2015/04/24 21:31:16, Nico wrote:
> > > Why is this lock needed now when it wasn't before? Why isn't it needed to
> read
> > > stopping_
> > 
> > Actually I think we should have had this lock even without this change and
I'm
> > not fully sure why we didn't have.
> > 
> > stopping_ seems to be touched only on the same thread that started the
thread,
> > hence no locking.
> 
> Maybe add a comment that says why accessing message_loop_ and stopping_
outside
> of the lock is safe.

Done.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/inco...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/inco...
base/message_loop/incoming_task_queue.h:56: // This should be called when it's
ready for scheduling work.
On 2015/04/27 22:49:34, Nico wrote:
> s/it/the message loop/

Done.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop.h:122: // the message loop runs on, before
calling Run().
On 2015/04/27 22:49:34, Nico wrote:
> I'm not sure how to best communicate what can be done with a message loop in
> this state. Maybe there should be a paragraph like "Only the Post*Task()
> functions can be called on the MessageLoop while it's in this state, all other
> functions may only be called after the loop has been bound to a thread"?
> 
> Or should we make the BindToCurrentThread() method private, make Thread a
> friend, and assume that MessageLoops are only ever created by Thread? (This CL
> couples them somewhat tightly.)
> 
> Then people reading this header would know that they're always talking  to
> fully-constructed MessageLoop objects.

Yeah that might be less confusing.  In the new CL I also made this constructor a
private static with a specific name (i.e. CreateUnbound) to make it clear that
only friend'ed classes like Thread instantiates MessageLoop that way. How does
this look to you now?

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop.h:136: // testing. No need to call Init if the
message loop is constructed this way.
On 2015/04/27 22:49:34, Nico wrote:
> s/Init/BindToCurrentThread/
> 
> (I still think it's weird that this is different for different constructors,
but
> ok if you're going to do a cleanup pass afterwards)

Done.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:30: DCHECK(!task.is_null()) <<
from_here.ToString();
On 2015/04/27 22:49:34, Nico wrote:
> DCHECK_NE(valid_thread_id_, kInvalidThreadId); 
> 
> (?)

Done.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:38: DCHECK(!task.is_null()) <<
from_here.ToString();
On 2015/04/27 22:49:34, Nico wrote:
> DCHECK_NE(valid_thread_id_, kInvalidThreadId); 
> 
> (?)

Done.

https://codereview.chromium.org/1011683002/diff/720001/base/message_loop/mess...
base/message_loop/message_loop_proxy_impl.cc:42: bool
MessageLoopProxyImpl::RunsTasksOnCurrentThread() const {
On 2015/04/27 22:49:34, Nico wrote:
> DCHECK_NE(valid_thread_id_, kInvalidThreadId); 
> 
> (?)

Done.

https://codereview.chromium.org/1011683002/diff/720001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1011683002/diff/720001/base/threading/thread....
base/threading/thread.h:239: TimerSlack message_loop_timer_slack_;
On 2015/04/27 22:49:34, Nico wrote:
> Add a comment what this is for. ("Stores Options::timer_slack_ until the
message
> loop has been bound to a thread." or something like that)

Done.

[Nico, 2015-04-28 19:16:19]

Looks good.

Are the test failures on try trybots real?

[kinuko, 2015-04-29 14:14:50]

On 2015/04/28 19:16:19, Nico wrote:
> Looks good.
> 
> Are the test failures on try trybots real?

They were real, I removed DCHECK's in MessageLoopProxyImpl.Post*Task methods etc
(it is ok to call them before it's bound to a new thread).

It looks net_unittests failures on linux_android_rel_ng are real too, I'll need
some investigation there :(

[kinuko, 2015-04-30 12:36:45]

> It looks net_unittests failures on linux_android_rel_ng are real too, I'll
need
> some investigation there :(

Ok, I think I fixed the android issue
(net/android/network_change_notifier_android.cc).  I'm feeling that I'll
probably need to go through a few reverts after landing, but anyway... PTAL?

[Nico, 2015-04-30 18:00:52]

lgtm, but please have someone familiar with that code look at the
network_change_notifier_android change

[kinuko, 2015-05-01 05:40:47]

On 2015/04/30 18:00:52, Nico wrote:
> lgtm, but please have someone familiar with that code look at the
> network_change_notifier_android change

Yeah, thanks!

[kinuko, 2015-05-01 05:41:17]

kinuko@chromium.org changed reviewers:
+ pauljensen@chromium.org

[kinuko, 2015-05-01 05:43:01]

pauljensen@: could you review changes in network_change_notifier_android.cc?
After this patch Thread::Init() will start to be called asynchronously after
Thread::Start() is called, and the current code crashes without the change.

[kinuko, 2015-05-01 11:42:04]

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

[kinuko, 2015-05-01 11:42:23]

kinuko@chromium.org changed reviewers:
- jam@chromium.org

[pauljensen, 2015-05-01 11:54:44]

https://codereview.chromium.org/1011683002/diff/810001/net/android/network_ch...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/1011683002/diff/810001/net/android/network_ch...
net/android/network_change_notifier_android.cc:110:
base::Bind(&DnsConfigServiceThread::InitAfterStart, weak_ptr_));
This is not good.  This will allow changes to escape the DnsConfigService.
The tiny and fast code in InitAfterStart() must block creation of
NetworkChangeNotifierAndroid.
I think we could just get rid of the DCHECK(IsRunning()) in InitAfterStart(),
which is probably causing the crash you mention.  The code in OnNetworkChanged
may need to change however.  Is task_runner() guaranteed to return non-null
after Thread()?

[kinuko, 2015-05-01 13:58:06]

https://codereview.chromium.org/1011683002/diff/810001/net/android/network_ch...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/1011683002/diff/810001/net/android/network_ch...
net/android/network_change_notifier_android.cc:110:
base::Bind(&DnsConfigServiceThread::InitAfterStart, weak_ptr_));
On 2015/05/01 11:54:44, pauljensen wrote:
> This is not good.  This will allow changes to escape the DnsConfigService.
> The tiny and fast code in InitAfterStart() must block creation of
> NetworkChangeNotifierAndroid.

Ok thanks, I suspected it might be the case. I can modify this part block.

> I think we could just get rid of the DCHECK(IsRunning()) in InitAfterStart(),
> which is probably causing the crash you mention.  The code in OnNetworkChanged
> may need to change however.

No, the reason it was crashing was
NetworkChangeNotifier::AddNetworkChangeObserver(this) in InitAfterStart() gets
called and OnNetworkChanged() is called by the notifier before we initialize
dns_config_service_ in Init(), so the PostTask on line 132 tries to run
DnsConfigServicePosix::OnNetworkChanged on nullptr.

> Is task_runner() guaranteed to return non-null
> after Thread()?

Yes.

[kinuko, 2015-05-02 03:14:25]

Patchset #27 (id:830001) has been deleted

[kinuko, 2015-05-02 03:14:43]

Patchset #27 (id:830001) has been deleted

[kinuko, 2015-05-02 04:35:09]

Updated the network_change_notifier_android.cc, this time I made it simply
blocking after Thread::Start before it calls InitAfterStart (so preserving the
original behavior).  It might be possible to make the part non-blocking but I'm
not familiar with the logic and I'd like to leave it for now.  pauljensen@:
PTAL?

[pauljensen, 2015-05-04 11:20:03]

net/ LGTM

[kinuko, 2015-05-04 15:29:49]

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

[kinuko, 2015-05-04 15:31:23]

jam@: could you do owner review for content/ changes?

[jam, 2015-05-04 18:16:06]

On 2015/05/04 15:31:23, kinuko wrote:
> jam@: could you do owner review for content/ changes?

content lgtm

[kinuko, 2015-05-05 04:16:58]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-05-05 04:16:59]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1011683002/#ps850001
(title: "WaitUntilThreadStarted in NetworkChangeNotifierAndroid")

[commit-bot: I haz the power, 2015-05-05 04:18:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011683002/850001

[commit-bot: I haz the power, 2015-05-05 04:24:56]

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

[commit-bot: I haz the power, 2015-05-05 04:24:58]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kinuko, 2015-05-05 14:24:16]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-05-05 14:24:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
thakis@chromium.org, pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/1011683002/#ps870001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-05 14:24:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011683002/870001

[commit-bot: I haz the power, 2015-05-05 17:14:16]

Committed patchset #28 (id:870001)

[commit-bot: I haz the power, 2015-05-05 17:15:50]

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

[kinuko, 2015-05-07 01:26:50]

A revert of this CL (patchset #28 id:870001) has been created in
https://codereview.chromium.org/1122383003/ by kinuko@chromium.org.

The reason for reverting is: This introduced flaky assertion failure on some
tests  http://crbug.com/485157.

[kinuko, 2015-05-07 14:29:34]

Patchset #29 (id:890001) has been deleted