Initial in-process implementation of some Mojo primitives.

Initial in-process implementation of some Mojo primitives.

This has an initial in-process implementation of the most basic Mojo primitives:
  - MojoClose()
  - MojoWait()
  - MojoWaitMany()
  - MojoCreateMessagePipe()
  - MojoWriteMessage()
  - MojoReadMessage()

R=darin@chromium.org

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

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

[viettrungluu, 2013-09-24 20:44:08]

Sorry for the size of the change (though it's 2/3 tests and a good chunk of
comments).

There are still some more tests to write, but coverage should be very good
already.

[darin (slow to review), 2013-09-24 22:48:19]

Initial comments, just on the first two files.

https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp#newcode18
mojo/mojo.gyp:18: 'mojo_core_unittests',
It might be nice to only have mojo_unittests instead of mojo_{foo}_unittests for
various values of foo.

https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp#newcode24
mojo/mojo.gyp:24: 'target_name': 'mojo_core',
should this be mojo_system instead to match the mojo/system directory?

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h
File mojo/public/system/core.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:10: #include <stdint.h>
This probably goes without saying:
I think on Windows, this works assuming VS2010. We can provide a dummy stdint.h
for older versions of the compiler as needed.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:15: // (typically they'll be ignored), not necessarily
an error.
I guess this is primarily an issue for newer clients running on older versions
of the system, where new flags have since been introduced. Hmm...

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE = 0;
since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you get a
similar result (type safety wise) with using an enum here? the benefit is that
an enum will work fine in C as well as C++. then, you don't have to duplicate
code. Ditto for the MOJO_RESULT_* values.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:140: // TODO(vtl): flags? other params (e.g., queue
sizes, max message sizes?)
Maybe we should have an attributes struct like pthreads.

  struct MessagePipeAttributes {
    int flags;
    ...
  };

Based on bits set in flags, we'd dereference fields on MessagePipeAttributes.

This way it is possible to add attributes to configure queue_sizes, etc. later.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:174: // TODO(vtl): C++ wrapper for |WaitMany()|.
We probably want to ensure that Handle is packed so that Handle* can be
reinterpret_cast'd to MojoHandle*.

[darin (slow to review), 2013-09-25 06:06:07]

More comments. This looks really great. I haven't gotten through the entire CL
in detail yet.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h
File mojo/public/system/core.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:34: const MojoHandle MOJO_HANDLE_INVALID = 0;
yay, sanity!  i'm so glad you chose a value other than -1 for this ;-)

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:53: const MojoResult MOJO_RESULT_ABORTED = -10;
what is the difference between ABORTED and CANCELLED?

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:56: const MojoResult MOJO_RESULT_INTERNAL = -13;
what does the INTERNAL error code refer too? my experience with the net:: error
codes is that it really helps to have individual error codes for each error
source. MOJO_RESULT_UNKNOWN would be quite a sad error code since it leaves you
with little information to help investigate a cause. we should feel free to
append new error codes as needed during implementation.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE = 0;
On 2013/09/24 22:48:19, darin wrote:
> since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you get a
> similar result (type safety wise) with using an enum here? the benefit is that
> an enum will work fine in C as well as C++. then, you don't have to duplicate
> code. Ditto for the MOJO_RESULT_* values.

Not having to duplicate these values would be nice as it would suck for these to
be out-of-sync. Plus, even in C code, it is nice to avoid macros :-P

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:115: const MojoReadMessageFlags
MOJO_READ_MESSAGE_FLAG_MAY_DISCARD = 1 << 0;
nit: add a comment explaining what MAY_DISCARD means.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:148: MojoResult MojoReadMessage(MojoHandle handle,
nit: it'd be good to include some documentation for these functions. what error
code gets returned when *num_bytes is too small?

btw, do we expect MojoReadMessage to always return exactly one complete message?
I think we probably should, but hmm...

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.cc
File mojo/system/core_impl.cc (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.cc#...
mojo/system/core_impl.cc:80: return MOJO_RESULT_INVALID_ARGUMENT;
nit: consider a more specific error code of MOJO_RESULT_INVALID_HANDLE

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.cc#...
mojo/system/core_impl.cc:212: if (next_handle_ == MOJO_HANDLE_INVALID)
it seems like there are some good test cases to write for these overflow cases.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h#n...
mojo/system/core_impl.h:60: friend class test::CoreTestBase;
note: Are you sure you want to use all of the nested namespaces? They have a way
of making things confusing. For example, if we ever have a top-level test
namespace, then you may end up needing to distinguish between ::test::Foo and
test::Foo. Maybe just prefixing test-specific classes with "Test" would work
better? TestCoreBase?

Also, the nested namespaces result in class names that are not as easy to grep
for because you get conflicts with classes of the same name defined in other
namespaces. /shrug... just food for thought.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h#n...
mojo/system/core_impl.h:86: static CoreImpl* mojo_system_singleton_;
nit: maybe just call it singleton_ since "mojo" and "system" are already fairly
implicit.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h#n...
mojo/system/core_impl.h:94: MojoHandle next_handle_;  // Invariant: never
|MOJO_HANDLE_INVALID|.
nit: next_handle_ -> next_unused_handle_? I had to study the implementation to
realize what this variable was for.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/dispatcher.cc
File mojo/system/dispatcher.cc (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/dispatcher.cc...
mojo/system/dispatcher.cc:15: return MOJO_RESULT_INVALID_ARGUMENT;
Hmm, how do we ever reach this branch? it seems like CoreImpl::Close ensures
that Dispatcher::Close() can only ever be called once per Dispatcher. If so,
then we can just assert that is_closed_ is false on entry to this function,
right?

https://codereview.chromium.org/23621056/diff/11001/mojo/system/limits.h
File mojo/system/limits.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/limits.h#newc...
mojo/system/limits.h:16: const size_t kMaxWaitManyNumHandles = 256;
In practice, I think we will quickly exceed this limit. We exceeded
WaitForMultipleObjects' limit of 64 very easily. It led us to build a very
complicated coping mechanism, relying on Win32 worker pools, etc. until we
finally switched over to IO completion ports, which don't have this low of a
limit.

Plus, given that we imagine creating a lot of message pipes, it seems like the
limit here should be much higher and have more to do with address space
limitations.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/limits.h#newc...
mojo/system/limits.h:20: const size_t kMaxMessageNumHandles = 256;
i'd be tempted to also allow this to be larger unless it somehow hurts to have
it be larger.

[viettrungluu, 2013-09-25 18:22:16]

Thanks for your comments. I'll consider/take care of the things that require
code changes separately.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h
File mojo/public/system/core.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:53: const MojoResult MOJO_RESULT_ABORTED = -10;
On 2013/09/25 06:06:07, darin wrote:
> what is the difference between ABORTED and CANCELLED?

(I plan on adding/copying documentation, but for now you can look at
http://cs/#piper///depot/google3/util/task/codes.proto .)

Cancelled is due to caller action, whereas aborted is a system action.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:56: const MojoResult MOJO_RESULT_INTERNAL = -13;
On 2013/09/25 06:06:07, darin wrote:
> what does the INTERNAL error code refer too? my experience with the net::
error
> codes is that it really helps to have individual error codes for each error
> source. MOJO_RESULT_UNKNOWN would be quite a sad error code since it leaves
you
> with little information to help investigate a cause. we should feel free to
> append new error codes as needed during implementation.

"Internal" should basically never happen -- it indicates a bug in the system
itself. I agree that we should feel free to append new error codes necessary,
but at the same time try to resist error reporting that's too detailed.

(A litmus test for whether you need separate error codes might be if the caller
should do different things in response. For debugging purposes, suitable logging
may be better....)

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE = 0;
On 2013/09/24 22:48:19, darin wrote:
> since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you get a
> similar result (type safety wise) with using an enum here? the benefit is that
> an enum will work fine in C as well as C++. then, you don't have to duplicate
> code. Ditto for the MOJO_RESULT_* values.

Sigh. Maybe. enums are of unspecified width and signedness, so you could
conceivably get type-conversion or sign-conversion warnings....

Possibly I should make a macro to do the right thing and avoid the duplication.

Hrm.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:148: MojoResult MojoReadMessage(MojoHandle handle,
On 2013/09/25 06:06:07, darin wrote:
> nit: it'd be good to include some documentation for these functions. what
error
> code gets returned when *num_bytes is too small?

Yes, I still have to write documentation. :)

> btw, do we expect MojoReadMessage to always return exactly one complete
message?
> I think we probably should, but hmm...

I expect MojoReadMessage to return only complete messages.

If the message is too large, it'll return the required size in
|*num_bytes|/|*num_handles|, unless it's in "may discard" mode in which case
it'll discard too-large messages. I pondered letting it do partial reads, but
decided that added more complexity than it's worth.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h#n...
mojo/system/core_impl.h:60: friend class test::CoreTestBase;
On 2013/09/25 06:06:07, darin wrote:
> note: Are you sure you want to use all of the nested namespaces? They have a
way
> of making things confusing. For example, if we ever have a top-level test
> namespace, then you may end up needing to distinguish between ::test::Foo and
> test::Foo. Maybe just prefixing test-specific classes with "Test" would work
> better? TestCoreBase?

That's a good question. The advantage of putting test-only things in a uniform
test namespace is that then (theoretically) it'd be easier to check that no
test-only things are leaking into non-tests.

> Also, the nested namespaces result in class names that are not as easy to grep
> for because you get conflicts with classes of the same name defined in other
> namespaces. /shrug... just food for thought.

I'm generally averse to nested namespaces too. The reason I put this stuff into
a system namespace (under mojo) is that I expect Mojo things (shell, services)
to be implemented in the mojo namespace and using in-process/native
implementations of the primitives (but should never use anything in the
mojo::system namespace directly).

https://codereview.chromium.org/23621056/diff/11001/mojo/system/dispatcher.cc
File mojo/system/dispatcher.cc (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/dispatcher.cc...
mojo/system/dispatcher.cc:15: return MOJO_RESULT_INVALID_ARGUMENT;
On 2013/09/25 06:06:07, darin wrote:
> Hmm, how do we ever reach this branch? it seems like CoreImpl::Close ensures
> that Dispatcher::Close() can only ever be called once per Dispatcher. If so,
> then we can just assert that is_closed_ is false on entry to this function,
> right?

You're right, thanks (Close() can't race with itself, but it can race with other
calls).

https://codereview.chromium.org/23621056/diff/11001/mojo/system/limits.h
File mojo/system/limits.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/limits.h#newc...
mojo/system/limits.h:16: const size_t kMaxWaitManyNumHandles = 256;
On 2013/09/25 06:06:07, darin wrote:
> In practice, I think we will quickly exceed this limit. We exceeded
> WaitForMultipleObjects' limit of 64 very easily. It led us to build a very
> complicated coping mechanism, relying on Win32 worker pools, etc. until we
> finally switched over to IO completion ports, which don't have this low of a
> limit.
> 
> Plus, given that we imagine creating a lot of message pipes, it seems like the
> limit here should be much higher and have more to do with address space
> limitations.

Okay. (256 was a low-ish number that I chose arbitrarily. We can adjust "low"
upwards some.)

https://codereview.chromium.org/23621056/diff/11001/mojo/system/limits.h#newc...
mojo/system/limits.h:20: const size_t kMaxMessageNumHandles = 256;
On 2013/09/25 06:06:07, darin wrote:
> i'd be tempted to also allow this to be larger unless it somehow hurts to have
> it be larger.

Ditto.

What do you think of 1024 as the limit for these?

(They aren't without cost, which is why I don't want to give the impression
that, say, sending 100000 handles is a good idea.)

[viettrungluu, 2013-09-25 19:47:59]

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h
File mojo/public/system/core.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE = 0;
On 2013/09/25 18:22:16, viettrungluu wrote:
> On 2013/09/24 22:48:19, darin wrote:
> > since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you get a
> > similar result (type safety wise) with using an enum here? the benefit is
that
> > an enum will work fine in C as well as C++. then, you don't have to
duplicate
> > code. Ditto for the MOJO_RESULT_* values.
> 
> Sigh. Maybe. enums are of unspecified width and signedness, so you could
> conceivably get type-conversion or sign-conversion warnings....
> 
> Possibly I should make a macro to do the right thing and avoid the
duplication.

Blech, of course you can't have a macro to define a macro. :(

[darin (slow to review), 2013-09-26 05:29:19]

The enum approach hasn't caused us problems with Pepper has it? Another
approach would be some kind of codegen for core.h, but that sounds like
overkill too.

-Darin


On Wed, Sep 25, 2013 at 12:48 PM, <viettrungluu@chromium.org> wrote:

>
> https://codereview.chromium.**org/23621056/diff/11001/mojo/**
>
public/system/core.h<https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h>
> File mojo/public/system/core.h (right):
>
> https://codereview.chromium.**org/23621056/diff/11001/mojo/**
>
public/system/core.h#newcode83<https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h#newcode83>
> mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE =
> 0;
> On 2013/09/25 18:22:16, viettrungluu wrote:
>
>> On 2013/09/24 22:48:19, darin wrote:
>> > since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you
>>
> get a
>
>> > similar result (type safety wise) with using an enum here? the
>>
> benefit is that
>
>> > an enum will work fine in C as well as C++. then, you don't have to
>>
> duplicate
>
>> > code. Ditto for the MOJO_RESULT_* values.
>>
>
>  Sigh. Maybe. enums are of unspecified width and signedness, so you
>>
> could
>
>> conceivably get type-conversion or sign-conversion warnings....
>>
>
>  Possibly I should make a macro to do the right thing and avoid the
>>
> duplication.
>
> Blech, of course you can't have a macro to define a macro. :(
>
>
https://codereview.chromium.**org/23621056/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-09-26 05:35:22]

According to some random person on stack overflow:

" The C standard also specifies that each enumeration constant has type
int. Howeve, the term “enumeration constant” refers to the value constants
declared inside the enum { } block. A variable with an enum type may have
any integer type in C, e.g. it might be a shorter type than int if one can
represent all the values. (GCC, for example, has an option -fshort-enums
for doing precisely this.)"
On Sep 25, 2013 10:29 PM, "Darin Fisher" <darin@chromium.org> wrote:

> The enum approach hasn't caused us problems with Pepper has it? Another
> approach would be some kind of codegen for core.h, but that sounds like
> overkill too.
>
> -Darin
>
>
> On Wed, Sep 25, 2013 at 12:48 PM, <viettrungluu@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/23621056/diff/11001/mojo/**
>>
public/system/core.h<https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h>
>> File mojo/public/system/core.h (right):
>>
>> https://codereview.chromium.**org/23621056/diff/11001/mojo/**
>>
public/system/core.h#newcode83<https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h#newcode83>
>> mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE =
>> 0;
>> On 2013/09/25 18:22:16, viettrungluu wrote:
>>
>>> On 2013/09/24 22:48:19, darin wrote:
>>> > since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you
>>>
>> get a
>>
>>> > similar result (type safety wise) with using an enum here? the
>>>
>> benefit is that
>>
>>> > an enum will work fine in C as well as C++. then, you don't have to
>>>
>> duplicate
>>
>>> > code. Ditto for the MOJO_RESULT_* values.
>>>
>>
>>  Sigh. Maybe. enums are of unspecified width and signedness, so you
>>>
>> could
>>
>>> conceivably get type-conversion or sign-conversion warnings....
>>>
>>
>>  Possibly I should make a macro to do the right thing and avoid the
>>>
>> duplication.
>>
>> Blech, of course you can't have a macro to define a macro. :(
>>
>>
https://codereview.chromium.**org/23621056/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin1, 2013-09-26 05:35:52]

I meant to include the link:
http://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-g...
On Sep 25, 2013 10:35 PM, "Darin Fisher" <darin@chromium.org> wrote:

> According to some random person on stack overflow:
>
> " The C standard also specifies that each enumeration constant has type
> int. Howeve, the term “enumeration constant” refers to the value constants
> declared inside the enum { } block. A variable with an enum type may have
> any integer type in C, e.g. it might be a shorter type than int if one can
> represent all the values. (GCC, for example, has an option -fshort-enums
> for doing precisely this.)"
> On Sep 25, 2013 10:29 PM, "Darin Fisher" <darin@chromium.org> wrote:
>
>> The enum approach hasn't caused us problems with Pepper has it? Another
>> approach would be some kind of codegen for core.h, but that sounds like
>> overkill too.
>>
>> -Darin
>>
>>
>> On Wed, Sep 25, 2013 at 12:48 PM, <viettrungluu@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.**org/23621056/diff/11001/mojo/**
>>>
public/system/core.h<https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h>
>>> File mojo/public/system/core.h (right):
>>>
>>> https://codereview.chromium.**org/23621056/diff/11001/mojo/**
>>>
public/system/core.h#newcode83<https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h#newcode83>
>>> mojo/public/system/core.h:83: const MojoWaitFlags MOJO_WAIT_FLAG_NONE =
>>> 0;
>>> On 2013/09/25 18:22:16, viettrungluu wrote:
>>>
>>>> On 2013/09/24 22:48:19, darin wrote:
>>>> > since MojoWaitFlags is just a typedef for an uint32_t, wouldn't you
>>>>
>>> get a
>>>
>>>> > similar result (type safety wise) with using an enum here? the
>>>>
>>> benefit is that
>>>
>>>> > an enum will work fine in C as well as C++. then, you don't have to
>>>>
>>> duplicate
>>>
>>>> > code. Ditto for the MOJO_RESULT_* values.
>>>>
>>>
>>>  Sigh. Maybe. enums are of unspecified width and signedness, so you
>>>>
>>> could
>>>
>>>> conceivably get type-conversion or sign-conversion warnings....
>>>>
>>>
>>>  Possibly I should make a macro to do the right thing and avoid the
>>>>
>>> duplication.
>>>
>>> Blech, of course you can't have a macro to define a macro. :(
>>>
>>>
https://codereview.chromium.**org/23621056/<https://codereview.chromium.org/2...
>>>
>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2013-09-26 17:59:14]

https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp#newcode18
mojo/mojo.gyp:18: 'mojo_core_unittests',
On 2013/09/24 22:48:19, darin wrote:
> It might be nice to only have mojo_unittests instead of mojo_{foo}_unittests
for
> various values of foo.

Pros of having more unit test binaries:
- smaller, faster to build/link
- easier to execute relevant tests without using a filter
- inherent enforcement of unit-ness of unit tests

Cons:
- more test binaries to build and run
- harder to build/execute all tests (but maybe we should have a mojo_unittests
target that depends on the all the mojo_{foo}_unittests targets, and teach our
tooling to run such targets)
- harder to add tests to the bots (though this is also a tooling issue)

https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp#newcode24
mojo/mojo.gyp:24: 'target_name': 'mojo_core',
On 2013/09/24 22:48:19, darin wrote:
> should this be mojo_system instead to match the mojo/system directory?

Done.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h
File mojo/public/system/core.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:15: // (typically they'll be ignored), not necessarily
an error.
On 2013/09/24 22:48:19, darin wrote:
> I guess this is primarily an issue for newer clients running on older versions
> of the system, where new flags have since been introduced. Hmm...

Indeed. The reason I want to record this somewhere (probably not here) is that
this is a compatibility issue, but I don't want to incur the cost of validating
flags (even if it's admittedly a small cost).

(Possibly, it'd be safer to validate flags, though then that'd preclude, e.g.,
"waiting on everything".)

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:115: const MojoReadMessageFlags
MOJO_READ_MESSAGE_FLAG_MAY_DISCARD = 1 << 0;
On 2013/09/25 06:06:07, darin wrote:
> nit: add a comment explaining what MAY_DISCARD means.

Done.

https://codereview.chromium.org/23621056/diff/11001/mojo/public/system/core.h...
mojo/public/system/core.h:174: // TODO(vtl): C++ wrapper for |WaitMany()|.
On 2013/09/24 22:48:19, darin wrote:
> We probably want to ensure that Handle is packed so that Handle* can be
> reinterpret_cast'd to MojoHandle*.

Done.

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/23621056/diff/11001/mojo/system/core_impl.h#n...
mojo/system/core_impl.h:86: static CoreImpl* mojo_system_singleton_;
On 2013/09/25 06:06:07, darin wrote:
> nit: maybe just call it singleton_ since "mojo" and "system" are already
fairly
> implicit.

Done.

[darin (slow to review), 2013-09-27 15:09:54]

On 2013/09/26 17:59:14, viettrungluu wrote:
> https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp
> File mojo/mojo.gyp (right):
> 
> https://codereview.chromium.org/23621056/diff/11001/mojo/mojo.gyp#newcode18
> mojo/mojo.gyp:18: 'mojo_core_unittests',
> On 2013/09/24 22:48:19, darin wrote:
> > It might be nice to only have mojo_unittests instead of mojo_{foo}_unittests
> for
> > various values of foo.
> 
> Pros of having more unit test binaries:
> - smaller, faster to build/link
> - easier to execute relevant tests without using a filter
> - inherent enforcement of unit-ness of unit tests
> 
> Cons:
> - more test binaries to build and run
> - harder to build/execute all tests (but maybe we should have a mojo_unittests
> target that depends on the all the mojo_{foo}_unittests targets, and teach our
> tooling to run such targets)
> - harder to add tests to the bots (though this is also a tooling issue)

I was primarily worried about the last bullet there. I agree that is just
a tooling issue. Happy to see a better solution to that then mashing all
of our tests together into one executable.

[darin (slow to review), 2013-09-27 15:34:47]

LGTM

https://codereview.chromium.org/23621056/diff/29001/mojo/system/core_impl.cc
File mojo/system/core_impl.cc (right):

https://codereview.chromium.org/23621056/diff/29001/mojo/system/core_impl.cc#...
mojo/system/core_impl.cc:238: dispatchers.push_back(d);
Do we want to check for the same handle being specified multiple times in the
call to WaitMany? Or, perhaps the logic below will be resilient to that case.

https://codereview.chromium.org/23621056/diff/29001/mojo/system/limits.h
File mojo/system/limits.h (right):

https://codereview.chromium.org/23621056/diff/29001/mojo/system/limits.h#newc...
mojo/system/limits.h:16: const size_t kMaxWaitManyNumHandles = 256;
I'm certain this number needs to be *much* larger. We want to allow clients to
create as many handles as they want. They shouldn't be limited on how many
handles they can wait on from any given thread.

Perhaps kMaxWaitManyNumHandles should just be set to kMaxHandleTableSize.

https://codereview.chromium.org/23621056/diff/29001/mojo/system/waiter.cc
File mojo/system/waiter.cc (right):

https://codereview.chromium.org/23621056/diff/29001/mojo/system/waiter.cc#new...
mojo/system/waiter.cc:75: cv_.Signal();
You probably had this in mind at the time, but just after this call to
cv_.Signal(), |this| might become a junk pointer. The WaitManyInternal
implementation places the Waiter on the stack.

[viettrungluu, 2013-09-27 17:58:00]

Thanks.

https://codereview.chromium.org/23621056/diff/29001/mojo/system/core_impl.cc
File mojo/system/core_impl.cc (right):

https://codereview.chromium.org/23621056/diff/29001/mojo/system/core_impl.cc#...
mojo/system/core_impl.cc:238: dispatchers.push_back(d);
On 2013/09/27 15:34:47, darin wrote:
> Do we want to check for the same handle being specified multiple times in the
> call to WaitMany? Or, perhaps the logic below will be resilient to that case.

It's intended to be resilient to that case, and there's a use-case for a handle
being specified multiple times -- e.g., if someone wants to wait for a handle to
become readable and writable separately (and be told which it's being woken up
for).

I should add a test for this....

https://codereview.chromium.org/23621056/diff/29001/mojo/system/core_impl.cc#...
mojo/system/core_impl.cc:262: dispatchers[i]->RemoveWaiter(&waiter);
I'm not sure if I was smart when I wrote this or just lucky, but this ensures
that there's no use-after-free of |waiter|.

(Even if it were in TLS instead, we'd still want to do this, to avoid a race
condition if a thread did two |WaitMany()|s in a row.)

Since I'm obviously not smart enough to understand this this morning, I guess I
should comment it more thoroughly. :)

https://codereview.chromium.org/23621056/diff/29001/mojo/system/limits.h
File mojo/system/limits.h (right):

https://codereview.chromium.org/23621056/diff/29001/mojo/system/limits.h#newc...
mojo/system/limits.h:16: const size_t kMaxWaitManyNumHandles = 256;
On 2013/09/27 15:34:47, darin wrote:
> I'm certain this number needs to be *much* larger. We want to allow clients to
> create as many handles as they want. They shouldn't be limited on how many
> handles they can wait on from any given thread.
> 
> Perhaps kMaxWaitManyNumHandles should just be set to kMaxHandleTableSize.

Done.

https://codereview.chromium.org/23621056/diff/29001/mojo/system/waiter.cc
File mojo/system/waiter.cc (right):

https://codereview.chromium.org/23621056/diff/29001/mojo/system/waiter.cc#new...
mojo/system/waiter.cc:75: cv_.Signal();
On 2013/09/27 15:34:47, darin wrote:
> You probably had this in mind at the time, but just after this call to
> cv_.Signal(), |this| might become a junk pointer. The WaitManyInternal
> implementation places the Waiter on the stack.

Actually, |cv_.Wait()| only returns after |lock_| is released.

(See above comments about why all this is safe. At least I think it's safe.)

[commit-bot: I haz the power, 2013-09-27 20:33:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/23621056/69001

[commit-bot: I haz the power, 2013-09-27 21:28:38]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-27 21:33:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/23621056/41002

[viettrungluu, 2013-09-27 22:37:39]

Committed patchset #18 manually as r225801 (presubmit successful).

[viettrungluu, 2013-09-28 00:30:10]

Committed patchset #19 manually as r225821 (presubmit successful).

[tfarina, 2013-10-01 02:21:13]

https://codereview.chromium.org/23621056/diff/86001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/23621056/diff/86001/mojo/mojo.gyp#newcode66
mojo/mojo.gyp:66: # TODO(vtl): This should probably be '<(component)'; make it
work.
shouldn't be hard.

Add:
defines: [ 'MOJO_IMPLEMENTATION' ]

mojo_export.h # copy the contents from other _export.h (say content_export.h)

and do class MOJO_EXPORT Foo { };

while there aren't to many files to update.