NaCl: create a separate namespace for Mojo handles.

NaCl: create a separate namespace for Mojo handles.

BUG=401761
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/da897d61e4322370e9768df508434a2b8f0498c7

[Nick Bray (chromium), 2015-04-01 22:56:00]

ncbray@chromium.org changed reviewers:
+ vtl@google.com

[Nick Bray (chromium), 2015-04-01 22:56:01]

Early (ugly) RFC, specifically looking for ideas on API and file naming.

[viettrungluu, 2015-04-01 23:36:17]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2015-04-01 23:36:20]

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
File mojo/edk/embedder/core_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:17: // HACK
delete

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:18: using mojo::system::Core;
These are OK, so you can combine them with the previous list (I suggest sorting
them in alphabetical order).

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:38: Core* fromCorePtr =
static_cast<Core*>(from_core);
nit: no camelCase

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:39: if (fromCorePtr == nullptr) {
nit: For consistency, please omit braces from one-line if blocks.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:58: // HACK this could also be because the
handle does not exist.
Hmmm, this seems bad. Maybe you should make PopDispatcher/GetAndRemoveDispatcher
plumb the correct code through?

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:63: // HACK can we recover in this case?
The answer is no.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:64: DCHECK(createdHandle !=
MOJO_HANDLE_INVALID);
But probably the right thing to do is not to just DCHECK-fail, but to say:

if (created_handle == MOJO_HANDLE_INVALID) {
  LOG(ERROR) << "Oops";
  d->Close();
  return MOJO_RESULT_RESOURCE_EXHAUSTED;
}

(Then you may want to note somewhere that on MOJO_RESULT_RESOURCE_EXHAUSTED,
|handle| will no longer be valid.)

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:66: // TODO MakeUserPointer?
Probably.

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

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core.h#newc...
mojo/edk/system/core.h:55: // Like GetDispatcher, but also removes the handle
from the handle table.
nit: For consistency, "GetDispatcher" -> "|GetDispatcher()|.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core.h#newc...
mojo/edk/system/core.h:56: scoped_refptr<Dispatcher> PopDispatcher(MojoHandle
handle);
For parallelism with HandleTable, call this GetAndRemoveDispatcher.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
File mojo/edk/system/core_unittest.cc (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1314: MojoResult
CreateCoreWithInitialHandle(Core* srcCore,
No camelCased variable names, please.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1343: Core* core0 = new Core(new
embedder::SimplePlatformSupport());
Put this in a scoped_ptr.

Also include base/memory/scoped_ptr.h.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1344: 
Add an ASSERT_TRUE(core0); here if you like.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1347: // uint32_t result_index;
Delete.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1354: EXPECT_NE(MOJO_HANDLE_INVALID, h[0]);
It's unlikely that you'll want to continue the test (it'll just be a cascade of
more errors), so probably ASSERT_NE here.

Also (here and elsewhere), probably write ASSERT_NE(h[0], MOJO_HANDLE_INVALID),
in the natural order. (It's only {EXPECT,ASSERT}_EQ that are broken/weird in
that way.)

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1358: Core* core1 = nullptr;
scoped_ptr here too.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
File mojo/public/platform/native/core_system_api.h (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:10: #include <stddef.h>
nit: You don't need this include in this file.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:14: // This interface is similar
to system_thunks.h, except that it allows Mojo
nit: reformat comment to fill 80 columns

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:17: typedef void* MojoCoreHandle;
To avoid confusion, don't call it a "handle".

... which leads to a naming bikeshed:

* I don't think you should call this "core (system)"; that's confusing.
* Probably the name "private" should appear in the name of the API/thunks
somewhere. (I think I'd just call this file system_impl_private.h.)

I suggest calling these "system impls". Thus:

typedef void* MojoSystemImpl;

MojoGetDefaultSystemImpl()
MojoCreateSystemImpl()

etc.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:19: typedef
MojoCoreHandle(GetDefaultCoreFn)();
Please don't pollute the namespace with all these typedefs. They're unnecessary
(also see below).

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:25: typedef
MojoResult(CloseCoreFn)(MojoCoreHandle core);
In general, it'd be useful to separate the "added" functions from the "standard"
ones.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:110: GetDefaultCoreFn
MojoCoreGetDefaultCore;
Please declare these not as variables, but as actual functions -- they're
semantically slightly different (e.g., "&MojoCoreGetDefaultCore" would mean
different things).

(This is, I believe, strictly speaking incorrect.)

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
File mojo/public/platform/native/core_system_thunks.h (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_thunks.h:20: struct MojoCoreSystemThunks
{
Maybe something like "MojoSystemImplPrivateThunks", or something like that.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_thunks.h:22: // MojoCoreHandle
(*GetDefaultCore)();
?

https://codereview.chromium.org/1052723003/diff/1/nacl_bindings_generator/moj...
File nacl_bindings_generator/mojo_syscall.cc.tmpl (right):

https://codereview.chromium.org/1052723003/diff/1/nacl_bindings_generator/moj...
nacl_bindings_generator/mojo_syscall.cc.tmpl:104: // TODO handle errors?
nit: "TODO(ncbray):"

https://codereview.chromium.org/1052723003/diff/1/nacl_bindings_generator/moj...
nacl_bindings_generator/mojo_syscall.cc.tmpl:105:
MojoCoreTransferHandle(MojoCoreGetDefaultCore(), handle, g_mojo_core,
&g_mojo_handle);
80 cols

[Nick Bray (chromium), 2015-04-04 00:09:10]

Name bikeshedding, round 2!

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
File mojo/edk/embedder/core_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:17: // HACK
On 2015/04/01 23:36:18, viettrungluu wrote:
> delete

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:18: using mojo::system::Core;
On 2015/04/01 23:36:18, viettrungluu wrote:
> These are OK, so you can combine them with the previous list (I suggest
sorting
> them in alphabetical order).

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:38: Core* fromCorePtr =
static_cast<Core*>(from_core);
On 2015/04/01 23:36:18, viettrungluu wrote:
> nit: no camelCase

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:39: if (fromCorePtr == nullptr) {
On 2015/04/01 23:36:18, viettrungluu wrote:
> nit: For consistency, please omit braces from one-line if blocks.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:58: // HACK this could also be because the
handle does not exist.
On 2015/04/01 23:36:18, viettrungluu wrote:
> Hmmm, this seems bad. Maybe you should make
PopDispatcher/GetAndRemoveDispatcher
> plumb the correct code through?

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:63: // HACK can we recover in this case?
On 2015/04/01 23:36:18, viettrungluu wrote:
> The answer is no.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:64: DCHECK(createdHandle !=
MOJO_HANDLE_INVALID);
On 2015/04/01 23:36:18, viettrungluu wrote:
> But probably the right thing to do is not to just DCHECK-fail, but to say:
> 
> if (created_handle == MOJO_HANDLE_INVALID) {
>   LOG(ERROR) << "Oops";
>   d->Close();
>   return MOJO_RESULT_RESOURCE_EXHAUSTED;
> }
> 
> (Then you may want to note somewhere that on MOJO_RESULT_RESOURCE_EXHAUSTED,
> |handle| will no longer be valid.)

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/embedder/core_entr...
mojo/edk/embedder/core_entrypoints.cc:66: // TODO MakeUserPointer?
On 2015/04/01 23:36:18, viettrungluu wrote:
> Probably.

Done.

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

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core.h#newc...
mojo/edk/system/core.h:55: // Like GetDispatcher, but also removes the handle
from the handle table.
On 2015/04/01 23:36:19, viettrungluu wrote:
> nit: For consistency, "GetDispatcher" -> "|GetDispatcher()|.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core.h#newc...
mojo/edk/system/core.h:56: scoped_refptr<Dispatcher> PopDispatcher(MojoHandle
handle);
On 2015/04/01 23:36:19, viettrungluu wrote:
> For parallelism with HandleTable, call this GetAndRemoveDispatcher.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
File mojo/edk/system/core_unittest.cc (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/edk/system/core_unitte...
mojo/edk/system/core_unittest.cc:1314: MojoResult
CreateCoreWithInitialHandle(Core* srcCore,
On 2015/04/01 23:36:19, viettrungluu wrote:
> No camelCased variable names, please.

These changes are an artifact of how I bootstrapped and will not be in the final
CL.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
File mojo/public/platform/native/core_system_api.h (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:10: #include <stddef.h>
On 2015/04/01 23:36:19, viettrungluu wrote:
> nit: You don't need this include in this file.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:14: // This interface is similar
to system_thunks.h, except that it allows Mojo
On 2015/04/01 23:36:19, viettrungluu wrote:
> nit: reformat comment to fill 80 columns

git cl format really wants me to do it this way.  :/

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:17: typedef void* MojoCoreHandle;
On 2015/04/01 23:36:19, viettrungluu wrote:
> To avoid confusion, don't call it a "handle".
> 
> ... which leads to a naming bikeshed:
> 
> * I don't think you should call this "core (system)"; that's confusing.
> * Probably the name "private" should appear in the name of the API/thunks
> somewhere. (I think I'd just call this file system_impl_private.h.)
> 
> I suggest calling these "system impls". Thus:
> 
> typedef void* MojoSystemImpl;
> 
> MojoGetDefaultSystemImpl()
> MojoCreateSystemImpl()
> 
> etc.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:19: typedef
MojoCoreHandle(GetDefaultCoreFn)();
On 2015/04/01 23:36:19, viettrungluu wrote:
> Please don't pollute the namespace with all these typedefs. They're
unnecessary
> (also see below).

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:25: typedef
MojoResult(CloseCoreFn)(MojoCoreHandle core);
On 2015/04/01 23:36:19, viettrungluu wrote:
> In general, it'd be useful to separate the "added" functions from the
"standard"
> ones.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:110: GetDefaultCoreFn
MojoCoreGetDefaultCore;
On 2015/04/01 23:36:19, viettrungluu wrote:
> Please declare these not as variables, but as actual functions -- they're
> semantically slightly different (e.g., "&MojoCoreGetDefaultCore" would mean
> different things).
> 
> (This is, I believe, strictly speaking incorrect.)

I knew this was a little odd, but I've found nothing to indicate this doesn't
result in a plain-old forward declaration of a function.  Somewhat moot when I
get rid of the typedefs, but I'd be interested to know why these are variables.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
File mojo/public/platform/native/core_system_thunks.h (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_thunks.h:20: struct MojoCoreSystemThunks
{
On 2015/04/01 23:36:19, viettrungluu wrote:
> Maybe something like "MojoSystemImplPrivateThunks", or something like that.

Done.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_thunks.h:22: // MojoCoreHandle
(*GetDefaultCore)();
On 2015/04/01 23:36:19, viettrungluu wrote:
> ?

Done.

https://codereview.chromium.org/1052723003/diff/1/nacl_bindings_generator/moj...
File nacl_bindings_generator/mojo_syscall.cc.tmpl (right):

https://codereview.chromium.org/1052723003/diff/1/nacl_bindings_generator/moj...
nacl_bindings_generator/mojo_syscall.cc.tmpl:104: // TODO handle errors?
On 2015/04/01 23:36:19, viettrungluu wrote:
> nit: "TODO(ncbray):"

Done.

https://codereview.chromium.org/1052723003/diff/1/nacl_bindings_generator/moj...
nacl_bindings_generator/mojo_syscall.cc.tmpl:105:
MojoCoreTransferHandle(MojoCoreGetDefaultCore(), handle, g_mojo_core,
&g_mojo_handle);
On 2015/04/01 23:36:19, viettrungluu wrote:
> 80 cols

Done.

[viettrungluu, 2015-04-06 17:39:26]

More bikes in fewer sheds.

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
File mojo/public/platform/native/core_system_api.h (right):

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:14: // This interface is similar
to system_thunks.h, except that it allows Mojo
On 2015/04/04 00:09:09, Nick Bray (chromium) wrote:
> On 2015/04/01 23:36:19, viettrungluu wrote:
> > nit: reformat comment to fill 80 columns
> 
> git cl format really wants me to do it this way.  :/

It's actually correctly filled. I was just confused by rietveld's inability to
render 80 columns correctly (for new files, when there's no side-by-side diff).

https://codereview.chromium.org/1052723003/diff/1/mojo/public/platform/native...
mojo/public/platform/native/core_system_api.h:110: GetDefaultCoreFn
MojoCoreGetDefaultCore;
On 2015/04/04 00:09:09, Nick Bray (chromium) wrote:
> On 2015/04/01 23:36:19, viettrungluu wrote:
> > Please declare these not as variables, but as actual functions -- they're
> > semantically slightly different (e.g., "&MojoCoreGetDefaultCore" would mean
> > different things).
> > 
> > (This is, I believe, strictly speaking incorrect.)
> 
> I knew this was a little odd, but I've found nothing to indicate this doesn't
> result in a plain-old forward declaration of a function.  Somewhat moot when I
> get rid of the typedefs, but I'd be interested to know why these are
variables.

I was just confused. I thought your typedefs were for function pointers, not
functions....

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
File mojo/edk/embedder/core_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
nit: 2015

Also, bikeshed: The name of this file is somewhat confusing. Maybe call it
"impl_entrypoints.cc" (ugh? "system_impl_entrypoints.cc"?
"system_impl_private_entrypoints.cc"?).

It could also use a file-level comment indicating what it contains.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:19: // Definitions of the core system
functions.
Probably this comment should be updated.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:36: Core* from_system_ptr =
static_cast<Core*>(from_system);
nit: Maybe just call this "from_core"

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:43: Core* to_system_ptr =
static_cast<Core*>(to_system);
... and to_core

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:55: // TODO document this case, including
the fact that |handle| will no longer
Yeah, you should probably do this TODO. ;)

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:68: MojoResult
MojoCloseSystemImpl(MojoSystemImpl system) {
Since things aren't actually designed to be shut down, maybe we should just omit
this call?

(And if we really wanted to allow shut down, it should probably be called
"MojoShutdownSystemImpl"[*].)

[*] Or maybe "MojoShutDownSystemImpl".

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:69: mojo::system::Core* core_ptr =
static_cast<mojo::system::Core*>(system);
nit (here and below): Probably just call these variables "core".

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:91: struct MojoHandleSignalsState*
signals_state) {
nit (here and below): since this is a C++ file, no "struct" needed

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/system/core_un...
File mojo/edk/system/core_unittest.cc (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/system/core_un...
mojo/edk/system/core_unittest.cc:1314: MojoResult
CreateCoreWithInitialHandle(Core* srcCore,
(Just reminding myself that the camelCase in this file should be taken care of.)

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/system/core_un...
mojo/edk/system/core_unittest.cc:1321: if (srcCore == nullptr) {
... and probably for consistency, omit braces on one-line if blocks.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
File mojo/public/platform/native/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/BUILD.gn:10:
mojo_sdk_source_set("system_impl_private") {
nit: Probably put this target after the system target? (Maybe we should just
alphabetize all the targets in this file.)

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/BUILD.gn:19: # The GYP target analogous to this one
builds this code into a
Since there's no gyp target analogous to this, you can probably delete this
comment.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_api.h (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_api.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
bikeshed: I think you should just call this file "system_impl_private.h".

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_api.h:7: #ifndef
MOJO_PUBLIC_PLATFORM_NATIVE_CORE_SYSTEM_API_H_
Header guard needs to be updated.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_api.h:18: MojoSystemImpl
MojoGetDefaultSystemImpl();
bikeshed:

Maybe we should be ruthlessly consistent, and always prefix with MojoSystemImpl?

So we'd have MojoSystemImplClose(), MojoSystemImplWait(), etc.

What this and "create" would become is a bit less clear....
MojoSystemImplGetDefault()? MojoSystemImplGetDefaultSystemImpl()? (ha)
MojoSystemImplCreate()? ...?

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_thunks.h (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.h:7: #ifndef
MOJO_PUBLIC_PLATFORM_NATIVE_CORE_SYSTEM_THUNKS_H_
Update header guard.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.h:10: #include <stddef.h>
Don't think you need this include.

[Nick Bray (chromium), 2015-04-06 22:51:00]

The shed looks nearly done.  A few questions inline, plus I need to add tests.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
File mojo/edk/embedder/core_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2015/04/06 17:39:25, viettrungluu wrote:
> nit: 2015
> 
> Also, bikeshed: The name of this file is somewhat confusing. Maybe call it
> "impl_entrypoints.cc" (ugh? "system_impl_entrypoints.cc"?
> "system_impl_private_entrypoints.cc"?).
> 
> It could also use a file-level comment indicating what it contains.

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:19: // Definitions of the core system
functions.
On 2015/04/06 17:39:25, viettrungluu wrote:
> Probably this comment should be updated.

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:36: Core* from_system_ptr =
static_cast<Core*>(from_system);
On 2015/04/06 17:39:25, viettrungluu wrote:
> nit: Maybe just call this "from_core"

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:43: Core* to_system_ptr =
static_cast<Core*>(to_system);
On 2015/04/06 17:39:25, viettrungluu wrote:
> ... and to_core

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:55: // TODO document this case, including
the fact that |handle| will no longer
On 2015/04/06 17:39:25, viettrungluu wrote:
> Yeah, you should probably do this TODO. ;)

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:68: MojoResult
MojoCloseSystemImpl(MojoSystemImpl system) {
On 2015/04/06 17:39:25, viettrungluu wrote:
> Since things aren't actually designed to be shut down, maybe we should just
omit
> this call?
> 
> (And if we really wanted to allow shut down, it should probably be called
> "MojoShutdownSystemImpl"[*].)
> 
> [*] Or maybe "MojoShutDownSystemImpl".

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:69: mojo::system::Core* core_ptr =
static_cast<mojo::system::Core*>(system);
On 2015/04/06 17:39:25, viettrungluu wrote:
> nit (here and below): Probably just call these variables "core".

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/edk/embedder/core_...
mojo/edk/embedder/core_entrypoints.cc:91: struct MojoHandleSignalsState*
signals_state) {
On 2015/04/06 17:39:25, viettrungluu wrote:
> nit (here and below): since this is a C++ file, no "struct" needed

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
File mojo/public/platform/native/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/BUILD.gn:10:
mojo_sdk_source_set("system_impl_private") {
On 2015/04/06 17:39:26, viettrungluu wrote:
> nit: Probably put this target after the system target? (Maybe we should just
> alphabetize all the targets in this file.)

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/BUILD.gn:19: # The GYP target analogous to this one
builds this code into a
On 2015/04/06 17:39:25, viettrungluu wrote:
> Since there's no gyp target analogous to this, you can probably delete this
> comment.

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_api.h (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_api.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/04/06 17:39:26, viettrungluu wrote:
> bikeshed: I think you should just call this file "system_impl_private.h".

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_api.h:7: #ifndef
MOJO_PUBLIC_PLATFORM_NATIVE_CORE_SYSTEM_API_H_
On 2015/04/06 17:39:26, viettrungluu wrote:
> Header guard needs to be updated.

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_api.h:18: MojoSystemImpl
MojoGetDefaultSystemImpl();
On 2015/04/06 17:39:26, viettrungluu wrote:
> bikeshed:
> 
> Maybe we should be ruthlessly consistent, and always prefix with
MojoSystemImpl?
> 
> So we'd have MojoSystemImplClose(), MojoSystemImplWait(), etc.
> 
> What this and "create" would become is a bit less clear....
> MojoSystemImplGetDefault()? MojoSystemImplGetDefaultSystemImpl()? (ha)
> MojoSystemImplCreate()? ...?

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_thunks.h (right):

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.h:7: #ifndef
MOJO_PUBLIC_PLATFORM_NATIVE_CORE_SYSTEM_THUNKS_H_
On 2015/04/06 17:39:26, viettrungluu wrote:
> Update header guard.

Done.

https://codereview.chromium.org/1052723003/diff/20001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.h:10: #include <stddef.h>
On 2015/04/06 17:39:26, viettrungluu wrote:
> Don't think you need this include.

Done.

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/BUILD.gn
File mojo/edk/embedder/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/BUILD...
mojo/edk/embedder/BUILD.gn:13:
"../../public/platform/native/system_impl_private.h",  # HACK
Where should this header live?

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:33: // If
|MOJO_RESULT_RESOURCE_EXHAUSTED| is returned, the |handle| will have been
Technically this corner case could be eliminated by fusing creation and transfer
into one operation.  (A newly created core should have space for at least one
handle?  Or we could hold the lock on the newly created core (which could not
deadlock), check there was enough space, and only remove the handle from the
source once we were sure there was space in the destination.)  Worth it?  It's
unclear that we won't need separate creation and transfer operations at some
point... but I am just speculating.

https://codereview.chromium.org/1052723003/diff/40001/nacl_bindings_generator...
File nacl_bindings_generator/mojo_syscall.cc.tmpl (right):

https://codereview.chromium.org/1052723003/diff/40001/nacl_bindings_generator...
nacl_bindings_generator/mojo_syscall.cc.tmpl:105:
MojoSystemImplTransferHandle(MojoSystemImplGetDefaultImpl(), handle,
g_mojo_system,
Line length, I know, will fix in next upload.

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
File shell/native_application_support.cc (right):

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
shell/native_application_support.cc:71:
SetThunks(&MojoMakeSystemImplThunksPrivate, "MojoSetSystemImplThunksPrivate",
I hate these names, but  coming up with names for two similar but distinct
interfaces is difficult.  Thoughts?

[viettrungluu, 2015-04-06 23:21:02]

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/BUILD.gn
File mojo/edk/embedder/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/BUILD...
mojo/edk/embedder/BUILD.gn:13:
"../../public/platform/native/system_impl_private.h",  # HACK
On 2015/04/06 22:50:59, Nick Bray (chromium) wrote:
> Where should this header live?

I think it's fine where it is.

Technically, I don't think you even need to include it from
system_impl_private_entrypoints.cc, but it's probably good to to ensure
consistency (namely, that the declarations match the definitions).

Anyhow, I think what you really want is a suitable target/mojo_sdk_source_set
for it, and put that in mojo_sdk_public_deps[*] below, or something like that.

[*] Or maybe add a non-public mojo_sdk_deps (haven't thought about it too much).

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/syste...
File mojo/edk/embedder/system_impl_private_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_private_entrypoints.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Maybe you should add a test file for these entrypoints?

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_private_entrypoints.cc:28: // TODO before
commiting: take core as parameter rather than using g_core?
Maybe just make this a generic "TODO(ncbray): ...", maybe.

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:33: // If
|MOJO_RESULT_RESOURCE_EXHAUSTED| is returned, the |handle| will have been
On 2015/04/06 22:51:00, Nick Bray (chromium) wrote:
> Technically this corner case could be eliminated by fusing creation and
transfer
> into one operation.  (A newly created core should have space for at least one
> handle?  Or we could hold the lock on the newly created core (which could not
> deadlock), check there was enough space, and only remove the handle from the
> source once we were sure there was space in the destination.)  Worth it?  It's
> unclear that we won't need separate creation and transfer operations at some
> point... but I am just speculating.

Probably it's not worth it, and the extra generality of the separate interfaces
is better.

In code that actually uses them together, you could do:

MojoSystemImpl new_system = MojoSystemImplCreateImpl();
MojoHandle result_handle = MOJO_HANDLE_INVALID;
CHECK_EQ(MojoSystemImplTransferHandle(from_system, old_handle, new_system,
&result_handle), MOJO_RESULT_OK);

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:126: }
nit: could use a |// extern "C"| comment

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_thunks.h (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.h:114: inline
MojoSystemImplThunksPrivate MojoMakeSystemImplThunksPrivate() {
Do you really need two thunk tables?

https://codereview.chromium.org/1052723003/diff/40001/nacl_bindings_generator...
File nacl_bindings_generator/mojo_syscall.cc.tmpl (right):

https://codereview.chromium.org/1052723003/diff/40001/nacl_bindings_generator...
nacl_bindings_generator/mojo_syscall.cc.tmpl:104: // TODO(ncbray): handle
errors?
I'd just CHECK (or equivalent -- I don't know what dependencies this file can
have) that it didn't fail.

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
File shell/native_application_support.cc (right):

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
shell/native_application_support.cc:71:
SetThunks(&MojoMakeSystemImplThunksPrivate, "MojoSetSystemImplThunksPrivate",
On 2015/04/06 22:51:00, Nick Bray (chromium) wrote:
> I hate these names, but  coming up with names for two similar but distinct
> interfaces is difficult.  Thoughts?

(I asked this elsewhere....) Do you really need two thunk tables?

[viettrungluu, 2015-04-06 23:49:33]

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
File shell/native_application_support.cc (right):

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
shell/native_application_support.cc:71:
SetThunks(&MojoMakeSystemImplThunksPrivate, "MojoSetSystemImplThunksPrivate",
On 2015/04/06 23:21:02, viettrungluu wrote:
> On 2015/04/06 22:51:00, Nick Bray (chromium) wrote:
> > I hate these names, but  coming up with names for two similar but distinct
> > interfaces is difficult.  Thoughts?
> 
> (I asked this elsewhere....) Do you really need two thunk tables?

Also, if you really need two thunk tables (and I understand the appeal), I'd
call the first one "SystemImplControlThunks" and the latter "SystemImplThunks".
Maybe you can think of a better word than "control".

[Nick Bray (chromium), 2015-04-07 23:51:06]

OK, I think we've chosen the paint color.  PTAL.

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/BUILD.gn
File mojo/edk/embedder/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/BUILD...
mojo/edk/embedder/BUILD.gn:13:
"../../public/platform/native/system_impl_private.h",  # HACK
On 2015/04/06 23:21:02, viettrungluu wrote:
> On 2015/04/06 22:50:59, Nick Bray (chromium) wrote:
> > Where should this header live?
> 
> I think it's fine where it is.
> 
> Technically, I don't think you even need to include it from
> system_impl_private_entrypoints.cc, but it's probably good to to ensure
> consistency (namely, that the declarations match the definitions).
> 
> Anyhow, I think what you really want is a suitable target/mojo_sdk_source_set
> for it, and put that in mojo_sdk_public_deps[*] below, or something like that.
> 
> [*] Or maybe add a non-public mojo_sdk_deps (haven't thought about it too
much).

Done.

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/syste...
File mojo/edk/embedder/system_impl_private_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_private_entrypoints.cc:28: // TODO before
commiting: take core as parameter rather than using g_core?
On 2015/04/06 23:21:02, viettrungluu wrote:
> Maybe just make this a generic "TODO(ncbray): ...", maybe.

Done.

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/40001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:126: }
On 2015/04/06 23:21:02, viettrungluu wrote:
> nit: could use a |// extern "C"| comment

Done.

https://codereview.chromium.org/1052723003/diff/40001/nacl_bindings_generator...
File nacl_bindings_generator/mojo_syscall.cc.tmpl (right):

https://codereview.chromium.org/1052723003/diff/40001/nacl_bindings_generator...
nacl_bindings_generator/mojo_syscall.cc.tmpl:104: // TODO(ncbray): handle
errors?
On 2015/04/06 23:21:02, viettrungluu wrote:
> I'd just CHECK (or equivalent -- I don't know what dependencies this file can
> have) that it didn't fail.

Done.

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
File shell/native_application_support.cc (right):

https://codereview.chromium.org/1052723003/diff/40001/shell/native_applicatio...
shell/native_application_support.cc:71:
SetThunks(&MojoMakeSystemImplThunksPrivate, "MojoSetSystemImplThunksPrivate",
On 2015/04/06 23:21:02, viettrungluu wrote:
> On 2015/04/06 22:51:00, Nick Bray (chromium) wrote:
> > I hate these names, but  coming up with names for two similar but distinct
> > interfaces is difficult.  Thoughts?
> 
> (I asked this elsewhere....) Do you really need two thunk tables?

Used the naming scheme you suggested.

No "need" strictly speaking, but it would make it simpler to evolve interfaces
in the future.

[viettrungluu, 2015-04-08 00:06:32]

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
File mojo/edk/embedder/system_impl_private_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_private_entrypoints.cc:56: // TODO document this
case, including the fact that |handle| will no longer
I think you did this already?

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
File mojo/edk/embedder/system_impl_unittest.cc (right):

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_unittest.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
nit: Maybe call this (the excessively-long) system_impl_private_unittest.cc?

Also, my feeling is that this test should be located in
mojo/public/platform/native.

The reason is that eventually we'll want to (also) run this as an apptest, which
will actually use the thunks....

(Probably mojo/public/platform/native should be reorganized a bit, but we can
worry about that later.)

https://codereview.chromium.org/1052723003/diff/60001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/60001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:24: MojoSystemImpl
MojoSystemImplGetDefaultImpl();
It occurs to me that (since this part of the EDK), you probably want the same
export macros as in mojo/public/c/system (MOJO_SYSTEM_EXPORT here).

[Nick Bray (chromium), 2015-04-08 20:43:03]

PTAL

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
File mojo/edk/embedder/system_impl_private_entrypoints.cc (right):

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_private_entrypoints.cc:56: // TODO document this
case, including the fact that |handle| will no longer
On 2015/04/08 00:06:32, viettrungluu wrote:
> I think you did this already?

Done, changed this into an explanation of the design forces.

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
File mojo/edk/embedder/system_impl_unittest.cc (right):

https://codereview.chromium.org/1052723003/diff/60001/mojo/edk/embedder/syste...
mojo/edk/embedder/system_impl_unittest.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/04/08 00:06:32, viettrungluu wrote:
> nit: Maybe call this (the excessively-long) system_impl_private_unittest.cc?
> 
> Also, my feeling is that this test should be located in
> mojo/public/platform/native.
> 
> The reason is that eventually we'll want to (also) run this as an apptest,
which
> will actually use the thunks....
> 
> (Probably mojo/public/platform/native should be reorganized a bit, but we can
> worry about that later.)

Done.

https://codereview.chromium.org/1052723003/diff/60001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/60001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:24: MojoSystemImpl
MojoSystemImplGetDefaultImpl();
On 2015/04/08 00:06:32, viettrungluu wrote:
> It occurs to me that (since this part of the EDK), you probably want the same
> export macros as in mojo/public/c/system (MOJO_SYSTEM_EXPORT here).

Done.

[viettrungluu, 2015-04-08 21:00:17]

LGTM w/a couple of nits and a note.

https://codereview.chromium.org/1052723003/diff/80001/mojo/edk/test/BUILD.gn
File mojo/edk/test/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/80001/mojo/edk/test/BUILD.gn#...
mojo/edk/test/BUILD.gn:128:
"../../public/platform/native:system_impl_private_tests",
You may ponder creating a new test binary (called, say,
mojo_public_system_impl_private_tests), and adding it in various places (the
public_tests group above and mojo/tools/data/unittests[*]).

[*] Maybe elsewhere too, but I don't remember offhand.

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:11: 
Probably you should explicitly include "mojo/public/c/system/system_export.h".

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_thunks.cc (right):

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
nit: This file is "new" (insofar as anything is ever new), so 2015.

[Nick Bray (chromium), 2015-04-08 21:49:51]

https://codereview.chromium.org/1052723003/diff/80001/mojo/edk/test/BUILD.gn
File mojo/edk/test/BUILD.gn (right):

https://codereview.chromium.org/1052723003/diff/80001/mojo/edk/test/BUILD.gn#...
mojo/edk/test/BUILD.gn:128:
"../../public/platform/native:system_impl_private_tests",
On 2015/04/08 21:00:17, viettrungluu wrote:
> You may ponder creating a new test binary (called, say,
> mojo_public_system_impl_private_tests), and adding it in various places (the
> public_tests group above and mojo/tools/data/unittests[*]).
> 
> [*] Maybe elsewhere too, but I don't remember offhand.

Done.

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private.h (right):

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private.h:11: 
On 2015/04/08 21:00:17, viettrungluu wrote:
> Probably you should explicitly include "mojo/public/c/system/system_export.h".

Done.

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
File mojo/public/platform/native/system_impl_private_thunks.cc (right):

https://codereview.chromium.org/1052723003/diff/80001/mojo/public/platform/na...
mojo/public/platform/native/system_impl_private_thunks.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2015/04/08 21:00:17, viettrungluu wrote:
> nit: This file is "new" (insofar as anything is ever new), so 2015.

Done.

[viettrungluu, 2015-04-08 23:01:38]

(Still LGTM[*], in case you you were waiting.)

[*] Maybe that's now Looks Gooder To Me.

[Nick Bray (chromium), 2015-04-08 23:29:00]

Committed patchset #6 (id:100001) manually as
da897d61e4322370e9768df508434a2b8f0498c7 (presubmit successful).