Added MojoNewWait and MojoNewWaitMany, along with unit tests.

Added MojoNewWait and MojoNewWaitMany, along with unit tests.

R=viettrungluu@chromium.org
BUG=365436
Committed: https://chromium.googlesource.com/external/mojo/+/4bd89c6adeed4ae33dfe57bdab934d609b15e979

[viettrungluu, 2014-12-02 17:24:56]

Mostly nits.

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

https://codereview.chromium.org/774673003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
The same comments as for core_unittest.cc ("below" -- I reviewed this change out
of order) apply to this file....

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:105: //   - a signal indicated by |signals| is
satisfied.
nit: Capitalize 'a'. (I suppose this is a list of mostly sentence-fragments not
sentences, so the entries should not be capitalized and should be separated by
semicolons not periods, but that would require editing everything else.)

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:106: //   - it becomes known that no signal
indicated by |signals| will ever be
Ditto. Also, the terminating period should be outside the closing parenthesis.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:109: //   - until |deadline| has passed.
" (etc.)

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:117: // |signals_state| (optional) See
documentation for |MojoHandleSignalsState|.
nit: colon after "(optional)"

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:142: //   - (at least) one handle satisfies a
signal indicated in its respective
Needs punctuation, and possibly capitalization.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:155: //     that caused the call to return. For
example, the index |i| (from 0 to
nit: "is returned" -> "is used to return"

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:157: //     must manually initialize this to -1
before you make this call because
"-1" -> "a suitable sentinel value" (also add "(e.g., -1)" if you'd like)

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:171: //   |MOJO_RESULT_RESOURCE_EXHAUSTED| if
there are too many handles.  The
nit: here and elsewhere: one space after '.'

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:174: //       (e.g., if it is NULL or if it has
already been closed).  The
"NULL" here is incorrect.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
File mojo/public/c/system/tests/core_unittest.cc (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:305: const MojoHandleSignals
MOJO_HANDLE_SIGNAL_READWRITABLE =
Use C++ style for constants (kSignalReadadableWritable, or whatever).

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:308: const MojoHandleSignals
MOJO_HANDLE_SIGNAL_ALL = MOJO_HANDLE_SIGNAL_READABLE |
"

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:312: TEST(NewCoreTest,
GetTimeTicksNow) {
Don't duplicate tests that you don't change.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:320: TEST(NewCoreTest,
InvalidHandle) {
Just add this as |TEST(CoreTest, NewInvalidHandle)|.

Better yet, make the conversion to MojoNewWait() in the existing test, unless
the new version of the test is more new stuff than old. (The way this is now, I
can't see a reasonable diff.)

We can deal with the momentary loss in test coverage, which isn't really
significant anyway, since the old MojoWait()/MojoWaitMany() are just shims onto
the same functions, with strictly less functionality.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
File mojo/public/c/system/tests/core_unittest_pure_c.c (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest_pure_c.c:93: // TODO(jimbe) Remove code
above and rename calls to wait below.
You may as well change the code above now, so I can see a diff.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h...
mojo/public/c/system/types.h:170: //   - |satisfied signals| Bitmask of signals
that were satisfied at some time
nit: colon after "|satisfied_signals|"

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h...
mojo/public/c/system/types.h:172: //         listed in |signals|.
Dangling reference to |signals|.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h...
mojo/public/c/system/types.h:173: //   - |satisfiable signals| These are the
signals that are possible to satisfy.
same nit as above

[jimbe, 2014-12-02 19:21:18]

PTAL.

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

https://codereview.chromium.org/774673003/diff/1/mojo/edk/embedder/embedder_u...
mojo/edk/embedder/embedder_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/12/02 17:24:55, viettrungluu wrote:
> The same comments as for core_unittest.cc ("below" -- I reviewed this change
out
> of order) apply to this file....

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:105: //   - a signal indicated by |signals| is
satisfied.
On 2014/12/02 17:24:55, viettrungluu wrote:
> nit: Capitalize 'a'. (I suppose this is a list of mostly sentence-fragments
not
> sentences, so the entries should not be capitalized and should be separated by
> semicolons not periods, but that would require editing everything else.)

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:106: //   - it becomes known that no signal
indicated by |signals| will ever be
On 2014/12/02 17:24:56, viettrungluu wrote:
> Ditto. Also, the terminating period should be outside the closing parenthesis.

Done. It's now an independent clause, so period stays inside parens.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:109: //   - until |deadline| has passed.
On 2014/12/02 17:24:55, viettrungluu wrote:
> " (etc.)

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:117: // |signals_state| (optional) See
documentation for |MojoHandleSignalsState|.
On 2014/12/02 17:24:55, viettrungluu wrote:
> nit: colon after "(optional)"

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:142: //   - (at least) one handle satisfies a
signal indicated in its respective
On 2014/12/02 17:24:55, viettrungluu wrote:
> Needs punctuation, and possibly capitalization.

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:155: //     that caused the call to return. For
example, the index |i| (from 0 to
On 2014/12/02 17:24:55, viettrungluu wrote:
> nit: "is returned" -> "is used to return"

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:157: //     must manually initialize this to -1
before you make this call because
On 2014/12/02 17:24:55, viettrungluu wrote:
> "-1" -> "a suitable sentinel value" (also add "(e.g., -1)" if you'd like)

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:171: //   |MOJO_RESULT_RESOURCE_EXHAUSTED| if
there are too many handles.  The
On 2014/12/02 17:24:55, viettrungluu wrote:
> nit: here and elsewhere: one space after '.'

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/functio...
mojo/public/c/system/functions.h:174: //       (e.g., if it is NULL or if it has
already been closed).  The
On 2014/12/02 17:24:55, viettrungluu wrote:
> "NULL" here is incorrect.

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
File mojo/public/c/system/tests/core_unittest.cc (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:305: const MojoHandleSignals
MOJO_HANDLE_SIGNAL_READWRITABLE =
On 2014/12/02 17:24:56, viettrungluu wrote:
> Use C++ style for constants (kSignalReadadableWritable, or whatever).

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:308: const MojoHandleSignals
MOJO_HANDLE_SIGNAL_ALL = MOJO_HANDLE_SIGNAL_READABLE |
On 2014/12/02 17:24:56, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:312: TEST(NewCoreTest,
GetTimeTicksNow) {
On 2014/12/02 17:24:56, viettrungluu wrote:
> Don't duplicate tests that you don't change.

As currently implemented, I can safely delete the entire upper namespace with no
additional editing and no danger of inadvertently adding or removing anything.
If I pick and choose one by one, then there is added time and complexity for
essentially no benefit since these duplicated functions will disappear in a few
days.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest.cc:320: TEST(NewCoreTest,
InvalidHandle) {
On 2014/12/02 17:24:56, viettrungluu wrote:
> Just add this as |TEST(CoreTest, NewInvalidHandle)|.
> 
> Better yet, make the conversion to MojoNewWait() in the existing test, unless
> the new version of the test is more new stuff than old. (The way this is now,
I
> can't see a reasonable diff.)
> 
> We can deal with the momentary loss in test coverage, which isn't really
> significant anyway, since the old MojoWait()/MojoWaitMany() are just shims
onto
> the same functions, with strictly less functionality.

Same comment as above.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
File mojo/public/c/system/tests/core_unittest_pure_c.c (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/tests/c...
mojo/public/c/system/tests/core_unittest_pure_c.c:93: // TODO(jimbe) Remove code
above and rename calls to wait below.
On 2014/12/02 17:24:56, viettrungluu wrote:
> You may as well change the code above now, so I can see a diff.

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h...
mojo/public/c/system/types.h:170: //   - |satisfied signals| Bitmask of signals
that were satisfied at some time
On 2014/12/02 17:24:56, viettrungluu wrote:
> nit: colon after "|satisfied_signals|"

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h...
mojo/public/c/system/types.h:172: //         listed in |signals|.
On 2014/12/02 17:24:56, viettrungluu wrote:
> Dangling reference to |signals|.

Done.

https://codereview.chromium.org/774673003/diff/1/mojo/public/c/system/types.h...
mojo/public/c/system/types.h:173: //   - |satisfiable signals| These are the
signals that are possible to satisfy.
On 2014/12/02 17:24:56, viettrungluu wrote:
> same nit as above

Done.

[jimbe, 2014-12-02 23:22:07]

Patchset #2 (id:20001) has been deleted

[jimbe, 2014-12-02 23:25:41]

Patchset #2 (id:40001) has been deleted

[jimbe, 2014-12-02 23:28:19]

Patchset #2 (id:60001) has been deleted

[jimbe, 2014-12-02 23:36:40]

Patchset #2 (id:80001) has been deleted

[jimbe, 2014-12-02 23:44:02]

Patchset #2 (id:100001) has been deleted

[viettrungluu, 2014-12-02 23:57:34]

LGTM w/nits

https://codereview.chromium.org/774673003/diff/120001/mojo/public/c/system/te...
File mojo/public/c/system/tests/core_unittest.cc (right):

https://codereview.chromium.org/774673003/diff/120001/mojo/public/c/system/te...
mojo/public/c/system/tests/core_unittest.cc:126: uint32_t result_index;
nit: initialize (to detect that it's set correctly)

https://codereview.chromium.org/774673003/diff/120001/mojo/public/c/system/te...
mojo/public/c/system/tests/core_unittest.cc:225: uint32_t result_index;
"

https://codereview.chromium.org/774673003/diff/120001/mojo/public/c/system/te...
File mojo/public/c/system/tests/core_unittest_pure_c.c (right):

https://codereview.chromium.org/774673003/diff/120001/mojo/public/c/system/te...
mojo/public/c/system/tests/core_unittest_pure_c.c:64: uint32_t result_index;
"

[jimbe, 2014-12-03 16:45:28]

Committed patchset #2 (id:120001) manually as
4bd89c6adeed4ae33dfe57bdab934d609b15e979 (presubmit successful).