Add embedder::AsyncWait()

Add embedder::AsyncWait()

This allows embedders to wait on a handle without
having any extra thread.

R=viettrungluu@chromium.org
TEST=EmbedderTest.AsyncWait
BUG=436189
Committed: https://chromium.googlesource.com/external/mojo/+/7ab59808c6aec184ba1af819abb2a18c49005c33

[viettrungluu, 2014-12-12 21:39:31]

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
File mojo/edk/embedder/embedder_unittest.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:185: MojoResult wait_result_;
You should probably call this just "result_", or the lock "wait_result_lock_".

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:190: void ReadAndIgnore(MojoHandle pipe)
{
Since you only use this function once (and not as a posted task), maybe you
don't need a function at all.

(Also, it occurs to me that we should just use the C++ wrappers throughout this
file, since I let that ship sail a long time ago.[*])

[*] The embedder API actually uses the C++ wrappers. (I'm not entirely sure this
is a good thing.)

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:191: char buffer[1000] = {};
Since you're not actually going to look at |buffer| afterwards, there's no piont
in initializing it.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:199: const char kHello[] = "hello";
nit: static const char

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter.cc
File mojo/edk/system/async_waiter.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.cc:7: #include "base/logging.h"
Don't need either of these includes.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter.h
File mojo/edk/system/async_waiter.h (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:10: #include "base/memory/ref_counted.h"
Don't need this include.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:12: #include "mojo/edk/system/dispatcher.h"
Nor this.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:13: #include
"mojo/edk/system/handle_signals_state.h"
"

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:26: AsyncWaiter(AwakeCallback callback);
explicit

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:27: virtual ~AsyncWaiter();
This doesn't need to be virtual. (Possibly, you should declare this class to be
final.)

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/core.cc#newc...
mojo/edk/system/core.cc:101: MojoResult Core::AsyncWait(MojoHandle handle,
You should add a test in core_unittest.cc for this.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/core.cc#newc...
mojo/edk/system/core.cc:109: if (rv != MOJO_RESULT_OK)
Probably:

if (rv == MOJO_RESULT_OK)
  ignore_result(waiter.release());
return rv;

would be better.

Also, you should explicitly include "base/macros.h" for ignore_result.

[viettrungluu, 2014-12-12 21:40:22]

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
File mojo/edk/embedder/embedder_unittest.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:190: void ReadAndIgnore(MojoHandle pipe)
{
On 2014/12/12 21:39:30, viettrungluu wrote:
> (Also, it occurs to me that we should just use the C++ wrappers throughout
this
> file, since I let that ship sail a long time ago.[*])

I'm not implying that you need to do that here, BTW.

> 
> [*] The embedder API actually uses the C++ wrappers. (I'm not entirely sure
this
> is a good thing.)

[gmorrita, 2014-12-12 22:56:27]

morrita@google.com changed reviewers:
+ morrita@google.com

[gmorrita, 2014-12-12 22:56:28]

PTAL?

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
File mojo/edk/embedder/embedder_unittest.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:185: MojoResult wait_result_;
On 2014/12/12 21:39:30, viettrungluu wrote:
> You should probably call this just "result_", or the lock "wait_result_lock_".

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:190: void ReadAndIgnore(MojoHandle pipe)
{
On 2014/12/12 21:39:30, viettrungluu wrote:
> Since you only use this function once (and not as a posted task), maybe you
> don't need a function at all.
> 
Did this for readability but apparently not worth it :-/
Inlined.

> (Also, it occurs to me that we should just use the C++ wrappers throughout
this
> file, since I let that ship sail a long time ago.[*])
I was wondering this as well. Let me go the C++ way.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:191: char buffer[1000] = {};
On 2014/12/12 21:39:30, viettrungluu wrote:
> Since you're not actually going to look at |buffer| afterwards, there's no
piont
> in initializing it.

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:199: const char kHello[] = "hello";
On 2014/12/12 21:39:30, viettrungluu wrote:
> nit: static const char

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter.cc
File mojo/edk/system/async_waiter.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.cc:7: #include "base/logging.h"
On 2014/12/12 21:39:30, viettrungluu wrote:
> Don't need either of these includes.

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter.h
File mojo/edk/system/async_waiter.h (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:10: #include "base/memory/ref_counted.h"
On 2014/12/12 21:39:30, viettrungluu wrote:
> Don't need this include.

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:12: #include "mojo/edk/system/dispatcher.h"
On 2014/12/12 21:39:31, viettrungluu wrote:
> Nor this.

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:13: #include
"mojo/edk/system/handle_signals_state.h"
On 2014/12/12 21:39:31, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:26: AsyncWaiter(AwakeCallback callback);
On 2014/12/12 21:39:31, viettrungluu wrote:
> explicit

Done.

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/async_waiter...
mojo/edk/system/async_waiter.h:27: virtual ~AsyncWaiter();
On 2014/12/12 21:39:31, viettrungluu wrote:
> This doesn't need to be virtual. (Possibly, you should declare this class to
be
> final.)
Our clang plugin complains about this :-/

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/798993003/diff/1/mojo/edk/system/core.cc#newc...
mojo/edk/system/core.cc:109: if (rv != MOJO_RESULT_OK)
On 2014/12/12 21:39:31, viettrungluu wrote:
> Probably:
> 
> if (rv == MOJO_RESULT_OK)
>   ignore_result(waiter.release());
> return rv;
> 
> would be better.
> 
> Also, you should explicitly include "base/macros.h" for ignore_result.

Done.

[viettrungluu, 2014-12-12 23:03:52]

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
File mojo/edk/system/async_waiter.cc (right):

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
mojo/edk/system/async_waiter.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
2014

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
File mojo/edk/system/async_waiter.h (right):

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
mojo/edk/system/async_waiter.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
nit: 2014

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/core_tes...
File mojo/edk/system/core_test_base.cc (left):

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/core_tes...
mojo/edk/system/core_test_base.cc:333: base::AutoLock locker(lock_);
Why was this deleted?

[gmorrita, 2014-12-12 23:08:49]

PTAL?

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
File mojo/edk/system/async_waiter.cc (right):

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
mojo/edk/system/async_waiter.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/12/12 23:03:52, viettrungluu wrote:
> 2014

Done.

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
File mojo/edk/system/async_waiter.h (right):

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/async_wa...
mojo/edk/system/async_waiter.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/12/12 23:03:52, viettrungluu wrote:
> nit: 2014

Done.

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/core_tes...
File mojo/edk/system/core_test_base.cc (left):

https://codereview.chromium.org/798993003/diff/20001/mojo/edk/system/core_tes...
mojo/edk/system/core_test_base.cc:333: base::AutoLock locker(lock_);
On 2014/12/12 23:03:52, viettrungluu wrote:
> Why was this deleted?
Oops. Unintentional..

[viettrungluu, 2014-12-12 23:22:43]

lgtm

[gmorrita, 2014-12-12 23:26:57]

On 2014/12/12 23:22:43, viettrungluu wrote:
> lgtm

Thank you! Landing.

[Hajime Morrita, 2014-12-12 23:30:29]

Committed patchset #3 (id:40001) manually as
7ab59808c6aec184ba1af819abb2a18c49005c33 (presubmit successful).