Break dependencies preventing move of test_server down to net.

Break dependencies preventing move of test_server down to net.

BUG=96594
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196188

[Paweł Hajdan Jr., 2013-04-19 17:39:26]

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/PH8p8SdnvLc/xaWuF...
for more context.

I'm going to move it down to net, but net_unittests wouldn't use it (that was
Ryan's concern).

[satorux1, 2013-04-22 01:12:01]

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:33: public
base::NonThreadSafe {
In favor of less inheritance, please use base::ThreadWatcher.

non_thread_safe.h says:


// Note that base::ThreadChecker offers identical functionality to              
 
// NonThreadSafe, but does not require inheritence. In general, it is preferable
 
// to have a base::ThreadChecker as a member, rather than inherit from          
 
// NonThreadSafe. For more details about when to choose one over the other, see 
 
// the documentation for base::ThreadChecker.

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:115: void
InitializeOnIOThread(base::WaitableEvent* event);
Could you avoid using WaitableEvent? My experience with WaitableEvent was horrid
so I'd like not to see it in this code.

[Paweł Hajdan Jr., 2013-04-22 18:53:44]

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:33: public
base::NonThreadSafe {
On 2013/04/22 01:12:01, satorux1 wrote:
> In favor of less inheritance, please use base::ThreadWatcher.
> 
> non_thread_safe.h says:
> 
> 
> // Note that base::ThreadChecker offers identical functionality to            
 
>  
> // NonThreadSafe, but does not require inheritence. In general, it is
preferable
>  
> // to have a base::ThreadChecker as a member, rather than inherit from        
 
>  
> // NonThreadSafe. For more details about when to choose one over the other,
see 
>  
> // the documentation for base::ThreadChecker.                                 
 
>  

Done.

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:115: void
InitializeOnIOThread(base::WaitableEvent* event);
On 2013/04/22 01:12:01, satorux1 wrote:
> Could you avoid using WaitableEvent? My experience with WaitableEvent was
horrid
> so I'd like not to see it in this code.

It'd be great to have a constructive alternative.

WaitableEvent is the best tool I know for this job, i.e. blocking execution of
the method until the other thread unblocks it.

Note that IMHO it's not OK to "dislike" base components in that way. My
experience was good, it's in base, reviewed and maintained.

Please provide what would be an alternative acceptable for you. I think taking
this as-is would also be fine.

[satorux1, 2013-04-23 01:31:04]

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:115: void
InitializeOnIOThread(base::WaitableEvent* event);
On 2013/04/22 18:53:44, Paweł Hajdan Jr. wrote:
> On 2013/04/22 01:12:01, satorux1 wrote:
> > Could you avoid using WaitableEvent? My experience with WaitableEvent was
> horrid
> > so I'd like not to see it in this code.
> 
> It'd be great to have a constructive alternative.
> 
> WaitableEvent is the best tool I know for this job, i.e. blocking execution of
> the method until the other thread unblocks it.
> 
> Note that IMHO it's not OK to "dislike" base components in that way. My
> experience was good, it's in base, reviewed and maintained.

Sorry I wasn't clear. I'm not disliking WaitableEvent. What I concern about is
the complexity added by use of WaitableEvent. Synchronization between threads is
tricky. WaitableEvent is useful for that, but it'd be nicer if can avoid doing
that from the beginning (I spent quite some time getting rid of WaitableEvent
from some code in Chrome to fix all the obscure issues introduced by
WaitableEvent, hence my experience was bad).

The original code was solving this problem by
content::RunAllPendingInMessageLoop(), which was simpler than WaitableEvent
IMHO.


If there is no better alternative (I don't have a good idea at this moment),
WaitableEvent may be the way to go, but just wanted to raise a concern.

> 
> Please provide what would be an alternative acceptable for you. I think taking
> this as-is would also be fine.

[satorux1, 2013-04-23 07:57:05]

Came up with an idea. Hope you'll like it.

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_operations_unittest.cc (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_operations_unittest.cc:51:
content::BrowserThread::IO)) {
indentation is off?

test_server_(content::BrowserThread::GetMessageLoopProxyForThread(
                 content::BrowserThread::IO)) {

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/test_server/http_server.cc (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/test_server/http_server.cc:92: event.Wait();
What about doing this:

base::RunLoop run_loop;
io_thread_->PostTaskAndReply(
    FROM_HERE,
    base::Bind(&HttpServer::ShutdownOnIOThread,
               base::Unretained(this))),
    run_loop.QuitClosure())
run_loop.RunUntilIdle();

This way, we don't need to pass WaitableEvent to ShutdownOnIOThread.

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/test_server/http_server.h:77: // Creates a http test
server. InitializeAndWaitUntilReady() must be called
Please add some comment about |io_thread| parameter.

[satorux1, 2013-04-23 08:08:50]

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:115: void
InitializeOnIOThread(base::WaitableEvent* event);
On 2013/04/23 01:31:04, satorux1 wrote:
> On 2013/04/22 18:53:44, Paweł Hajdan Jr. wrote:
> > On 2013/04/22 01:12:01, satorux1 wrote:
> > > Could you avoid using WaitableEvent? My experience with WaitableEvent was
> > horrid
> > > so I'd like not to see it in this code.
> > 
> > It'd be great to have a constructive alternative.
> > 
> > WaitableEvent is the best tool I know for this job, i.e. blocking execution
of
> > the method until the other thread unblocks it.
> > 
> > Note that IMHO it's not OK to "dislike" base components in that way. My
> > experience was good, it's in base, reviewed and maintained.
> 
> Sorry I wasn't clear. I'm not disliking WaitableEvent. What I concern about is
> the complexity added by use of WaitableEvent. Synchronization between threads
is
> tricky. WaitableEvent is useful for that, but it'd be nicer if can avoid doing
> that from the beginning (I spent quite some time getting rid of WaitableEvent
> from some code in Chrome to fix all the obscure issues introduced by
> WaitableEvent, hence my experience was bad).

FWIW, just to make it clearer:

issues introduced by WaitableEvent => issues introduced by *undesirable* use of
WaitableEvent.


> 
> The original code was solving this problem by
> content::RunAllPendingInMessageLoop(), which was simpler than WaitableEvent
> IMHO.
> 
> 
> If there is no better alternative (I don't have a good idea at this moment),
> WaitableEvent may be the way to go, but just wanted to raise a concern.
> 
> > 
> > Please provide what would be an alternative acceptable for you. I think
taking
> > this as-is would also be fine.
>

[Paweł Hajdan Jr., 2013-04-23 22:12:42]

PTAL

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/drive_api_operations_unittest.cc (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/drive_api_operations_unittest.cc:51:
content::BrowserThread::IO)) {
On 2013/04/23 07:57:05, satorux1 wrote:
> indentation is off?
> 
> test_server_(content::BrowserThread::GetMessageLoopProxyForThread(
>                  content::BrowserThread::IO)) {

Done.

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/test_server/http_server.cc (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/test_server/http_server.cc:92: event.Wait();
On 2013/04/23 07:57:05, satorux1 wrote:
> What about doing this:
> 
> base::RunLoop run_loop;
> io_thread_->PostTaskAndReply(
>     FROM_HERE,
>     base::Bind(&HttpServer::ShutdownOnIOThread,
>                base::Unretained(this))),
>     run_loop.QuitClosure())
> run_loop.RunUntilIdle();
> 
> This way, we don't need to pass WaitableEvent to ShutdownOnIOThread.

Done! Thank you for the great suggestion, it looks like a useful technique.

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/test_server/http_server.h:77: // Creates a http test
server. InitializeAndWaitUntilReady() must be called
On 2013/04/23 07:57:05, satorux1 wrote:
> Please add some comment about |io_thread| parameter.

Done.

[tfarina, 2013-04-23 22:14:06]

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server.h:21: class WaitableEvent;
looks like you can remove this now.

[satorux1, 2013-04-23 23:39:53]

LGTM with nits.

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
File chrome/browser/google_apis/test_server/http_server.cc (right):

https://codereview.chromium.org/14365019/diff/6001/chrome/browser/google_apis...
chrome/browser/google_apis/test_server/http_server.cc:92: event.Wait();
On 2013/04/23 22:12:42, Paweł Hajdan Jr. wrote:
> On 2013/04/23 07:57:05, satorux1 wrote:
> > What about doing this:
> > 
> > base::RunLoop run_loop;
> > io_thread_->PostTaskAndReply(
> >     FROM_HERE,
> >     base::Bind(&HttpServer::ShutdownOnIOThread,
> >                base::Unretained(this))),
> >     run_loop.QuitClosure())
> > run_loop.RunUntilIdle();
> > 
> > This way, we don't need to pass WaitableEvent to ShutdownOnIOThread.
> 
> Done! Thank you for the great suggestion, it looks like a useful technique.

Glad you liked it.

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server.h:21: class WaitableEvent;
On 2013/04/23 22:14:06, tfarina wrote:
> looks like you can remove this now.

good catch! please remove before submit.

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server.h:54: //  
test_server_.reset(new HttpServer());
please update the example code.

[Paweł Hajdan Jr., 2013-04-24 18:10:37]

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server.h:21: class WaitableEvent;
On 2013/04/23 23:39:53, satorux1 wrote:
> On 2013/04/23 22:14:06, tfarina wrote:
> > looks like you can remove this now.
> 
> good catch! please remove before submit.

Done.

https://codereview.chromium.org/14365019/diff/16001/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server.h:54: //  
test_server_.reset(new HttpServer());
On 2013/04/23 23:39:53, satorux1 wrote:
> please update the example code.

Done.

[Paweł Hajdan Jr., 2013-04-24 18:24:02]

Committed patchset #4 manually as r196188.