Moved Init() startup_data_->event.Signal() because derived classes may...

Moved Init() startup_data_->event.Signal() because derived classes may
not be safe to use until Init() has been called.  As an example,
RenderThread() creates it's IPC::SyncChannel in Init(), so it isn't
safe to call Send() method until after.

Change tested explicitly on Mac Win Linux.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8485

[agl, 2009-01-22 19:05:24]

http://codereview.chromium.org/18508/diff/15/16
File base/thread_unittest.cc (right):

http://codereview.chromium.org/18508/diff/15/16#newcode42
Line 42: SleepInsideInitThread() : Thread("none") { init_called_ = true; }
It seemed a little odd to call this "init_called_". It's more like
"init_running_", no?

http://codereview.chromium.org/18508/diff/15/16#newcode43
Line 43: virtual ~SleepInsideInitThread() { init_called_ = false; }
You're testing that Thread::Start doesn't return till after Init() has
completed, right? In which case I believe you want to set init_called_ to false
after the Sleep call. Since your SleepInsideInitThread is stack allocated,
below, the destructor will only be called once the test returns.

[John Grabowski, 2009-01-22 19:18:05]

http://codereview.chromium.org/18508/diff/15/16
File base/thread_unittest.cc (right):

http://codereview.chromium.org/18508/diff/15/16#newcode42
Line 42: SleepInsideInitThread() : Thread("none") { init_called_ = true; }
On 2009/01/22 19:05:25, agl wrote:
> It seemed a little odd to call this "init_called_". It's more like
> "init_running_", no?

Not exactly.  The thread is running before we get to Init().  This is more an
"has Init() been called", hence "init_called_".

http://codereview.chromium.org/18508/diff/15/16#newcode43
Line 43: virtual ~SleepInsideInitThread() { init_called_ = false; }
On 2009/01/22 19:05:25, agl wrote:
> You're testing that Thread::Start doesn't return till after Init() has
> completed, right? In which case I believe you want to set init_called_ to
false
> after the Sleep call. Since your SleepInsideInitThread is stack allocated,
> below, the destructor will only be called once the test returns.

Yes, make sure Start() doesn't return until after Init().
But I don't want init_called_ to be false after the sleep; that will cause it to
always fail.
But you are right, I had it modified in the wrong spot (want constructor to set
it to false; I arranged this wrong in response to a comment in the other CL).
Should be good now.

[agl, 2009-01-22 19:20:42]

This makes sense to me now. LGTM.


AGL

[Mark Mentovai, 2009-01-22 19:36:10]

LGTM, good catch.

http://codereview.chromium.org/18508/diff/21/212
File base/thread_unittest.cc (right):

http://codereview.chromium.org/18508/diff/21/212#newcode42
Line 42: SleepInsideInitThread() : Thread("none") { init_called_ = false; }
Prefer |init_called_(false) { }| for constructors.

http://codereview.chromium.org/18508/diff/21/212#newcode49
Line 49: bool InitCalled() { return init_called_; }
It's almost not worth mentioning for this because it's just a silly test class,
but normal style would be to name this init_called (simple inline getter) and
make the method const.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/18508/diff/21/212#newcode132
Line 132: 
Nit: kill trailing blank line.