Extract Awakable from Waiter

Extract Awakable from Waiter

This CL extracts simple Awakable interface from Waiter and
rename WaiterList to AwakableList.

This is a preparation for introducing system-integrated
AsyncWaiter.

R=viettrungluu@chromium.org
BUG=377980
Committed: https://chromium.googlesource.com/external/mojo/+/096decd183e3679cbf548495eee5ee8b8919c317

[viettrungluu, 2014-12-03 21:18:13]

Pass #1.

Please update all comments and properly re-wrap them (80-columns, only 1 space
after periods).

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:15: // Awakable is a callback that is called by
AwakableList.
nit: For consistency with the rest of the directory, enclose "code" in between
|...|: e.g., "|Awakable|" and "|AwakableList|".

Also, "callback" isn't quite right. I'd say: "|Awakable| is an interface for
things that may be waited on (from some separate thread).".

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:16: // The Awake() callback must satisfy following
contract:
Put this comment above |Awake()|. You can rewrite it to say: "|Awake()| must
satisfy the following contract:".

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:18: // * As it is called from any thread, the
callback must be thread-safe.
"it" -> "this", "the callback" -> "this"

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:19: // * As it is called inside a lock, the callback
must not call API that
"it" -> "this", "the callback" -> "this"

More significantly, this is not right. say '... this must not call anything that
takes "non-terminal" locks, i.e., those which are always safe to take.'.

(E.g., |Waiter|'s lock is safe to take. E.g., if one has a refcounted class
whose refcount is protected by a lock -- which protects nothing else -- taking
it would also be safe.)

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable_lis...
mojo/edk/system/awakable_list.h:24: // can
Please rewrap to 80 columns correctly.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable_lis...
mojo/edk/system/awakable_list.h:42: void Add(Awakable* awakable,
MojoHandleSignals signals, uint32_t context) {
Just rename AddWaiter and RemoveWaiter.

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/core.cc#newc...
mojo/edk/system/core.cc:51: // owned
rewrap

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/dispatcher.h...
mojo/edk/system/dispatcher.h:131: // Adds an awakable to this dispatcher. The
waiter will be woken up when this
"... dispatcher. The waiter ..." -> "... dispatcher, which will"

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/dispatcher.h...
mojo/edk/system/dispatcher.h:150: // multiple
please properly rewrap everywhere

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/dispatcher.h...
mojo/edk/system/dispatcher.h:151: // times for the same |waiter| on the same
object, so long as |AddWaiter()|
waiter -> awakable

Update AddWaiter.

[Hajime Morrita, 2014-12-03 21:44:09]

Thanks for the reivew Trung! PTAL?
I hope git cl format re-wrap comments adequately :-/

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:15: // Awakable is a callback that is called by
AwakableList.
On 2014/12/03 21:18:13, viettrungluu wrote:
> nit: For consistency with the rest of the directory, enclose "code" in between
> |...|: e.g., "|Awakable|" and "|AwakableList|".
> 
> Also, "callback" isn't quite right. I'd say: "|Awakable| is an interface for
> things that may be waited on (from some separate thread).".

Done.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:16: // The Awake() callback must satisfy following
contract:
On 2014/12/03 21:18:13, viettrungluu wrote:
> Put this comment above |Awake()|. You can rewrite it to say: "|Awake()| must
> satisfy the following contract:".

Done.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:18: // * As it is called from any thread, the
callback must be thread-safe.
On 2014/12/03 21:18:13, viettrungluu wrote:
> "it" -> "this", "the callback" -> "this"

Done.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable.h#n...
mojo/edk/system/awakable.h:19: // * As it is called inside a lock, the callback
must not call API that
On 2014/12/03 21:18:13, viettrungluu wrote:
> "it" -> "this", "the callback" -> "this"
> 
> More significantly, this is not right. say '... this must not call anything
that
> takes "non-terminal" locks, i.e., those which are always safe to take.'.
> 
> (E.g., |Waiter|'s lock is safe to take. E.g., if one has a refcounted class
> whose refcount is protected by a lock -- which protects nothing else -- taking
> it would also be safe.)

That is right. "any lock" is unnecessarily restrictive.

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable_lis...
mojo/edk/system/awakable_list.h:24: // can
On 2014/12/03 21:18:13, viettrungluu wrote:
> Please rewrap to 80 columns correctly.

Done.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/awakable_lis...
mojo/edk/system/awakable_list.h:42: void Add(Awakable* awakable,
MojoHandleSignals signals, uint32_t context) {
On 2014/12/03 21:18:13, viettrungluu wrote:
> Just rename AddWaiter and RemoveWaiter.
Oops. This should've been cleaned up :-(

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/core.cc#newc...
mojo/edk/system/core.cc:51: // owned
On 2014/12/03 21:18:13, viettrungluu wrote:
> rewrap

Done.

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

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/dispatcher.h...
mojo/edk/system/dispatcher.h:131: // Adds an awakable to this dispatcher. The
waiter will be woken up when this
On 2014/12/03 21:18:13, viettrungluu wrote:
> "... dispatcher. The waiter ..." -> "... dispatcher, which will"

Done.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/dispatcher.h...
mojo/edk/system/dispatcher.h:150: // multiple
On 2014/12/03 21:18:13, viettrungluu wrote:
> please properly rewrap everywhere

Done.

https://codereview.chromium.org/779503003/diff/1/mojo/edk/system/dispatcher.h...
mojo/edk/system/dispatcher.h:151: // times for the same |waiter| on the same
object, so long as |AddWaiter()|
On 2014/12/03 21:18:13, viettrungluu wrote:
> waiter -> awakable
> 
> Update AddWaiter.

Done.

[viettrungluu, 2014-12-03 22:31:27]

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable.h
File mojo/edk/system/awakable.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
mojo/edk/system/awakable.h:15: // |Awakable| is an interface that may be waited
on |AwakableList|.
remove |AwakableList|

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
mojo/edk/system/awakable.h:21: //   "non-terminal" locks, i.e., those which are
always safe to take.'
remove trailing '

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
File mojo/edk/system/awakable_list.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
mojo/edk/system/awakable_list.h:36: void AddWaiter(Awakable* awakable,
Could you just rename this to "Add"? (And "RemoveWaiter" to "Remove"?) Or
AddAwakable/RemoveAwakable would be OK too.

Sorry for the confusion.

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

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/core.cc#...
mojo/edk/system/core.cc:50: // The |Dispatcher| then adds the |Waiter| to a
|AwakableList| that's either
"a" -> "an"

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/message_...
File mojo/edk/system/message_pipe_endpoint.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/message_...
mojo/edk/system/message_pipe_endpoint.h:26: class Awakable;
reorder

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/waiter.h
File mojo/edk/system/waiter.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/waiter.h...
mojo/edk/system/waiter.h:27: ~Waiter();
Maybe this should be declared virtual.

(It's not really needed, since no one should delete it from an Awakable*. Maybe
that means that Awakable should have a protected, non-virtual destructor
declared.)

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/waiter.h...
mojo/edk/system/waiter.h:71: // This is a |uint32_t| because we really only need
to store an index (for
Remove this comment.

[Hajime Morrita, 2014-12-03 22:47:11]

PTAL?

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable.h
File mojo/edk/system/awakable.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
mojo/edk/system/awakable.h:15: // |Awakable| is an interface that may be waited
on |AwakableList|.
On 2014/12/03 22:31:26, viettrungluu wrote:
> remove |AwakableList|

Done.

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
mojo/edk/system/awakable.h:21: //   "non-terminal" locks, i.e., those which are
always safe to take.'
On 2014/12/03 22:31:26, viettrungluu wrote:
> remove trailing '

Oops.

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
File mojo/edk/system/awakable_list.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/awakable...
mojo/edk/system/awakable_list.h:36: void AddWaiter(Awakable* awakable,
On 2014/12/03 22:31:26, viettrungluu wrote:
> Could you just rename this to "Add"? (And "RemoveWaiter" to "Remove"?) Or
> AddAwakable/RemoveAwakable would be OK too.
> 
> Sorry for the confusion.

Done.

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

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/core.cc#...
mojo/edk/system/core.cc:50: // The |Dispatcher| then adds the |Waiter| to a
|AwakableList| that's either
On 2014/12/03 22:31:27, viettrungluu wrote:
> "a" -> "an"

Done.

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/message_...
File mojo/edk/system/message_pipe_endpoint.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/message_...
mojo/edk/system/message_pipe_endpoint.h:26: class Awakable;
On 2014/12/03 22:31:27, viettrungluu wrote:
> reorder

Done.

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/waiter.h
File mojo/edk/system/waiter.h (right):

https://codereview.chromium.org/779503003/diff/20001/mojo/edk/system/waiter.h...
mojo/edk/system/waiter.h:27: ~Waiter();
On 2014/12/03 22:31:27, viettrungluu wrote:
> Maybe this should be declared virtual.
> 
> (It's not really needed, since no one should delete it from an Awakable*.
Maybe
> that means that Awakable should have a protected, non-virtual destructor
> declared.)
Added protected ~Awakable() to make it clear that Awakable won't be deleted.

[viettrungluu, 2014-12-03 23:07:47]

Thanks, LGTM w/one last nit.

https://codereview.chromium.org/779503003/diff/40001/mojo/edk/system/waiter_t...
File mojo/edk/system/waiter_test_utils.h (right):

https://codereview.chromium.org/779503003/diff/40001/mojo/edk/system/waiter_t...
mojo/edk/system/waiter_test_utils.h:44: // code).  (We accept this unrealism for
simplicity, since |AwakableList| is
nit: one space after period, please :)

[Hajime Morrita, 2014-12-04 00:17:56]

Committed patchset #3 (id:40001) manually as
096decd183e3679cbf548495eee5ee8b8919c317 (presubmit successful).