Make mojo_system static and mojo_system_impl a component, never use both

Make mojo_system static and mojo_system_impl a component, never use both

BUG=
R=viettrungluu@chromium.org

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

[DaveMoore, 2014-04-09 19:59:08]

Cleanup

[viettrungluu, 2014-04-09 20:26:52]

https://codereview.chromium.org/231353002/diff/20001/mojo/embedder/embedder.cc
File mojo/embedder/embedder.cc (left):

https://codereview.chromium.org/231353002/diff/20001/mojo/embedder/embedder.c...
mojo/embedder/embedder.cc:57: void Init() {
Please don't do this.

https://codereview.chromium.org/231353002/diff/20001/mojo/shell/in_process_dy...
File mojo/shell/in_process_dynamic_service_runner.cc (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/shell/in_process_dy...
mojo/shell/in_process_dynamic_service_runner.cc:68: SetMojoCoreFn
set_mojo_core_fn = reinterpret_cast<SetMojoCoreFn>(
static_cast

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

https://codereview.chromium.org/231353002/diff/20001/mojo/system/core.cc#newc...
mojo/system/core.cc:26: static base::LazyInstance<mojo::system::Core> g_core =
Please don't do this. Core should be explicitly created and torn down. This was
a conscious decision.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/core.cc#newc...
mojo/system/core.cc:591: // Definitions of the MojoCore functions.
Please put these in a separate file. This file is already too big.

(I wouldn't have minded keeping it as "CoreImpl", and just having the extern "C"
bits in core.cc.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/handle_table.h
File mojo/system/handle_table.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/system/handle_table...
mojo/system/handle_table.h:30: // This class provides the (global) handle table
(owned by |Core|), which
Nit: Please re-wrap this paragraph, and the next one.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/handle_table...
mojo/system/handle_table.h:47: // use the result outside |Core|'s lock, you MUST
take a reference (e.g.,
"

https://codereview.chromium.org/231353002/diff/20001/mojo/system/mapping_table.h
File mojo/system/mapping_table.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/system/mapping_tabl...
mojo/system/mapping_table.h:30: // This class provides the (global) table of
memory mappings (owned by
Nit: Re-wrap.

[viettrungluu, 2014-04-09 20:39:10]

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
File mojo/public/platform/native/mojo_core.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.h:120: extern "C" MOJO_SYSTEM_EXPORT
MojoResult SetMojoCore(MojoCore* mojo_core);
This probably shouldn't be in the header file. (It's mis-representing to the
embedder that the function is actually directly available to it.)

You may note its existence (in DSOs) in a comment though.

[viettrungluu, 2014-04-09 20:52:22]

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
File mojo/public/platform/native/mojo_core.cc (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.cc:156: mojo_core->size = sizeof(g_core);
This seems like a bad idea. I think it should take a const MojoCore* argument,
and report the size separately.

(Otherwise, the caller can't have a common (possibly global const) copy of the
MojoCore struct, and may inadvertently be left with one which is actually
invalid -- i.e., |size| is bigger than its actual size.)

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
File mojo/public/platform/native/mojo_core.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.h:27: // the embedder.
You should probably have a comment indicating that the order is important (and
to only append).

Also, I wonder if there shouldn't be a suitable #pragma pack (or #pragma
push(pack, ...)/...pop), given that it's ABI.

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.h:29: size_t size;
Comment indicating that this should be set to sizeof(MojoCore)?

[DaveMoore, 2014-04-09 22:35:56]

Deal with review comments

[DaveMoore, 2014-04-09 22:37:19]

Fix accidental change to .gitmodules

[DaveMoore, 2014-04-09 22:41:36]

Fix accidental change to .gitmodules

[DaveMoore, 2014-04-09 22:49:07]

https://codereview.chromium.org/231353002/diff/20001/mojo/embedder/embedder.cc
File mojo/embedder/embedder.cc (left):

https://codereview.chromium.org/231353002/diff/20001/mojo/embedder/embedder.c...
mojo/embedder/embedder.cc:57: void Init() {
On 2014/04/09 20:26:52, viettrungluu wrote:
> Please don't do this.

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
File mojo/public/platform/native/mojo_core.cc (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.cc:156: mojo_core->size = sizeof(g_core);
On 2014/04/09 20:52:23, viettrungluu wrote:
> This seems like a bad idea. I think it should take a const MojoCore* argument,
> and report the size separately.
> 
> (Otherwise, the caller can't have a common (possibly global const) copy of the
> MojoCore struct, and may inadvertently be left with one which is actually
> invalid -- i.e., |size| is bigger than its actual size.)

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
File mojo/public/platform/native/mojo_core.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.h:27: // the embedder.
On 2014/04/09 20:52:23, viettrungluu wrote:
> You should probably have a comment indicating that the order is important (and
> to only append).
> 
> Also, I wonder if there shouldn't be a suitable #pragma pack (or #pragma
> push(pack, ...)/...pop), given that it's ABI.

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/public/platform/nat...
mojo/public/platform/native/mojo_core.h:29: size_t size;
On 2014/04/09 20:52:23, viettrungluu wrote:
> Comment indicating that this should be set to sizeof(MojoCore)?

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/shell/in_process_dy...
File mojo/shell/in_process_dynamic_service_runner.cc (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/shell/in_process_dy...
mojo/shell/in_process_dynamic_service_runner.cc:68: SetMojoCoreFn
set_mojo_core_fn = reinterpret_cast<SetMojoCoreFn>(
static_casts aren't allowed on things like this:

../../mojo/shell/in_process_dynamic_service_runner.cc:68:9: error: static_cast
from 'void *' to 'MojoSetSystemThunksFn' (aka 'size_t (*)(const MojoSystemThunks
*)') is not allowed
        static_cast<MojoSetSystemThunksFn>(app_library.GetFunctionPointer(

On 2014/04/09 20:26:52, viettrungluu wrote:
> static_cast

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

https://codereview.chromium.org/231353002/diff/20001/mojo/system/core.cc#newc...
mojo/system/core.cc:26: static base::LazyInstance<mojo::system::Core> g_core =
On 2014/04/09 20:26:52, viettrungluu wrote:
> Please don't do this. Core should be explicitly created and torn down. This
was
> a conscious decision.

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/core.cc#newc...
mojo/system/core.cc:591: // Definitions of the MojoCore functions.
On 2014/04/09 20:26:52, viettrungluu wrote:
> Please put these in a separate file. This file is already too big.
> 
> (I wouldn't have minded keeping it as "CoreImpl", and just having the extern
"C"
> bits in core.cc.

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/handle_table.h
File mojo/system/handle_table.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/system/handle_table...
mojo/system/handle_table.h:30: // This class provides the (global) handle table
(owned by |Core|), which
On 2014/04/09 20:26:52, viettrungluu wrote:
> Nit: Please re-wrap this paragraph, and the next one.

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/handle_table...
mojo/system/handle_table.h:47: // use the result outside |Core|'s lock, you MUST
take a reference (e.g.,
On 2014/04/09 20:26:52, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/231353002/diff/20001/mojo/system/mapping_table.h
File mojo/system/mapping_table.h (right):

https://codereview.chromium.org/231353002/diff/20001/mojo/system/mapping_tabl...
mojo/system/mapping_table.h:30: // This class provides the (global) table of
memory mappings (owned by
On 2014/04/09 20:26:52, viettrungluu wrote:
> Nit: Re-wrap.

Done.

[viettrungluu, 2014-04-09 23:06:44]

https://codereview.chromium.org/231353002/diff/80001/content/app/mojo/mojo_in...
File content/app/mojo/mojo_init.cc (left):

https://codereview.chromium.org/231353002/diff/80001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:23: mojo::embedder::Init();
Presumably, you need to restore this line?

https://codereview.chromium.org/231353002/diff/80001/mojo/common/test/run_all...
File mojo/common/test/run_all_unittests.cc (left):

https://codereview.chromium.org/231353002/diff/80001/mojo/common/test/run_all...
mojo/common/test/run_all_unittests.cc:20: mojo::embedder::Init();
"

https://codereview.chromium.org/231353002/diff/80001/mojo/embedder/test_embed...
File mojo/embedder/test_embedder.cc (left):

https://codereview.chromium.org/231353002/diff/80001/mojo/embedder/test_embed...
mojo/embedder/test_embedder.cc:43: Core::Reset();
This line is important.

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
File mojo/public/platform/native/system_thunks.h (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:7: 
nit: Should also include <stddef.h> for size_t.

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:35: MojoResult (*Close)(MojoHandle
handle);
Comment: It's unnecessary to include the parameter names, and since we're
probably not trying to document anything for anyone here you may as well delete
them.

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:97: // Intended to be called from
the embedder. Returns a |MojoCore| initialized
MojoCore -> MojoSystemThunks

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:98: // to contain pointers to each
of the embedder's MojoCore functions.
"

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:99: static inline const
MojoSystemThunks MojoMakeSystemThunks() {
const is unnecessary

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:126: // Called to set the MojoCore
functionality of a DSO. Returns the expected size.
MojoCore

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:127: extern "C" MOJO_SYSTEM_EXPORT
size_t MojoSetSystemThunks(
Shouldn't be in this header, per my 2nd batch of comments.

https://codereview.chromium.org/231353002/diff/80001/mojo/shell/in_process_dy...
File mojo/shell/in_process_dynamic_service_runner.cc (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/shell/in_process_dy...
mojo/shell/in_process_dynamic_service_runner.cc:80: }
break;

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc
File mojo/system/core.cc (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:25: static mojo::system::Core* g_core = NULL;
Move this down a few lines, and get rid of the mojo::system.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:92: 
nit:
// static

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:93: Core* Core::GetInstance() {
nit: ordering doesn't match the .h file

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:95: g_core = new Core;
Please don't do this. This masks initialization errors.

Probably there should be explicit CreateInstance()/ResetInstance()/GetInstance()
static methods.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:590: 
nit: Please don't add trailing blank lines.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.h
File mojo/system/core.h (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.h#newco...
mojo/system/core.h:29: // These methods are only to be used by via the embedder
API.
You mangled the comments in this file:

This comment is now in the wrong place, and you deleted a subsequent (crucial)
comment.

[DaveMoore, 2014-04-10 17:26:50]

Address review concerns, move static Core* to entrypoints

[DaveMoore, 2014-04-10 18:16:12]

Export MojoSetSystemThunks() regardless of component build

[DaveMoore, 2014-04-10 18:53:58]

Get content builds to work

[DaveMoore, 2014-04-10 18:58:58]

https://codereview.chromium.org/231353002/diff/80001/content/app/mojo/mojo_in...
File content/app/mojo/mojo_init.cc (left):

https://codereview.chromium.org/231353002/diff/80001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:23: mojo::embedder::Init();
On 2014/04/09 23:06:44, viettrungluu wrote:
> Presumably, you need to restore this line?

Done.

https://codereview.chromium.org/231353002/diff/80001/mojo/common/test/run_all...
File mojo/common/test/run_all_unittests.cc (left):

https://codereview.chromium.org/231353002/diff/80001/mojo/common/test/run_all...
mojo/common/test/run_all_unittests.cc:20: mojo::embedder::Init();
On 2014/04/09 23:06:44, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/231353002/diff/80001/mojo/embedder/test_embed...
File mojo/embedder/test_embedder.cc (left):

https://codereview.chromium.org/231353002/diff/80001/mojo/embedder/test_embed...
mojo/embedder/test_embedder.cc:43: Core::Reset();
Modified it to use the new entrypoints::Get/SetCore
On 2014/04/09 23:06:44, viettrungluu wrote:
> This line is important.

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
File mojo/public/platform/native/system_thunks.h (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/public/platform/nat...
mojo/public/platform/native/system_thunks.h:7: 
On 2014/04/09 23:06:44, viettrungluu wrote:
> nit: Should also include <stddef.h> for size_t.

Done.

https://codereview.chromium.org/231353002/diff/80001/mojo/shell/in_process_dy...
File mojo/shell/in_process_dynamic_service_runner.cc (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/shell/in_process_dy...
mojo/shell/in_process_dynamic_service_runner.cc:80: }
On 2014/04/09 23:06:44, viettrungluu wrote:
> break;

Done.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc
File mojo/system/core.cc (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:25: static mojo::system::Core* g_core = NULL;
On 2014/04/09 23:06:44, viettrungluu wrote:
> Move this down a few lines, and get rid of the mojo::system.

The static has moved to entrypoints.cc, with Set and Get functions.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:92: 
Gone
On 2014/04/09 23:06:44, viettrungluu wrote:
> nit:
> // static

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:93: Core* Core::GetInstance() {
Gone
On 2014/04/09 23:06:44, viettrungluu wrote:
> nit: ordering doesn't match the .h file

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:95: g_core = new Core;
Gone
On 2014/04/09 23:06:44, viettrungluu wrote:
> Please don't do this. This masks initialization errors.
> 
> Probably there should be explicit
CreateInstance()/ResetInstance()/GetInstance()
> static methods.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.cc#newc...
mojo/system/core.cc:590: 
On 2014/04/09 23:06:44, viettrungluu wrote:
> nit: Please don't add trailing blank lines.

Done.

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.h
File mojo/system/core.h (right):

https://codereview.chromium.org/231353002/diff/80001/mojo/system/core.h#newco...
mojo/system/core.h:29: // These methods are only to be used by via the embedder
API.
On 2014/04/09 23:06:44, viettrungluu wrote:
> You mangled the comments in this file:
> 
> This comment is now in the wrong place, and you deleted a subsequent (crucial)
> comment.

Done.

[viettrungluu, 2014-04-10 19:38:06]

LGTM with a few small issues. You break the Winuxoid Component Frobinator build,
you buy it.

https://codereview.chromium.org/231353002/diff/140001/mojo/mojo_public.gypi
File mojo/mojo_public.gypi (left):

https://codereview.chromium.org/231353002/diff/140001/mojo/mojo_public.gypi#o...
mojo/mojo_public.gypi:235: 'mojo_system',
(I suspect yes, if the perftests build without the system_impl dependency.)

https://codereview.chromium.org/231353002/diff/140001/mojo/mojo_public.gypi
File mojo/mojo_public.gypi (right):

https://codereview.chromium.org/231353002/diff/140001/mojo/mojo_public.gypi#n...
mojo/mojo_public.gypi:141: 'mojo_system_impl',
Can we pick up this dependency via mojo_run_all_unittests, rather than listing
it here?

https://codereview.chromium.org/231353002/diff/140001/mojo/mojo_public.gypi#n...
mojo/mojo_public.gypi:188: 'mojo_system_impl',
"

https://codereview.chromium.org/231353002/diff/140001/mojo/public/platform/na...
File mojo/public/platform/native/system_thunks.cc (right):

https://codereview.chromium.org/231353002/diff/140001/mojo/public/platform/na...
mojo/public/platform/native/system_thunks.cc:11: static MojoSystemThunks
g_thunks = {0};
g_system_thunks? (optional; I don't really care)

https://codereview.chromium.org/231353002/diff/140001/mojo/public/platform/na...
File mojo/public/platform/native/system_thunks.h (right):

https://codereview.chromium.org/231353002/diff/140001/mojo/public/platform/na...
mojo/public/platform/native/system_thunks.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/231353002/diff/140001/mojo/public/platform/na...
mojo/public/platform/native/system_thunks.h:102: static inline const
MojoSystemThunks MojoMakeSystemThunks() {
Please remove both "static" and "const".

Alternatively, you could declare system_thunks to be "static const", and return
a pointer to it (a |const MojoSystemThunks*|).

https://codereview.chromium.org/231353002/diff/140001/mojo/public/platform/na...
mojo/public/platform/native/system_thunks.h:137: extern "C" THUNK_EXPORT size_t
MojoSetSystemThunks(
I keep on telling you to delete this (and maybe leave a comment about its
existence), and move it to the .cc file.... (Note that you can annotate the
function definition directly with extern "C", etc.)

The embedder shouldn't see this when it includes it (because it can't call it
directly, only by dlsym() or equivalent).

Also, you should note the lifetime of the now-misnamed |mojo_core| (further nit
-> system_thunks). I.e., does it have to copy it, or can it just save the
pointer? (As currently implemented, you have to copy it, but it'd be awfully
tempting to just save a pointer....)

https://codereview.chromium.org/231353002/diff/140001/mojo/system/entrypoints.cc
File mojo/system/entrypoints.cc (right):

https://codereview.chromium.org/231353002/diff/140001/mojo/system/entrypoints...
mojo/system/entrypoints.cc:7: static mojo::system::Core* g_core = NULL;
nit: I'd still just move this down a few lines, to be able to omit the
"mojo::system::".

https://codereview.chromium.org/231353002/diff/140001/mojo/system/entrypoints.h
File mojo/system/entrypoints.h (right):

https://codereview.chromium.org/231353002/diff/140001/mojo/system/entrypoints...
mojo/system/entrypoints.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
Missing include guards (even if this file could be multiply-included safely
currently).

https://codereview.chromium.org/231353002/diff/140001/mojo/system/entrypoints...
mojo/system/entrypoints.h:7: class Core;
nit: add a blank line above?

https://codereview.chromium.org/231353002/diff/140001/mojo/system/entrypoints...
mojo/system/entrypoints.h:19: 
I have a totally irrational pet peeve against extra end-of-file newlines.

[DaveMoore, 2014-04-11 21:22:14]

Make sure that apps that depend on mojo_common_lib don't depend on mojo_system

[DaveMoore, 2014-04-11 22:00:26]

Add explicit dependency on mojo_system_impl to mojo_service_managaer

[DaveMoore, 2014-04-11 22:22:37]

Add explicit dependency on mojo_system_impl to mojo_gles2_impl

[DaveMoore, 2014-04-11 22:42:29]

Add explicit dependency on mojo_system_impl to mojo_native_viewport_service

[DaveMoore, 2014-04-13 22:03:47]

Make mojo_test_support a component instead of a shared_library

[DaveMoore, 2014-04-14 18:19:43]

Fix the mac loader path dependencies

[DaveMoore, 2014-04-14 20:21:47]

Committed patchset #14 manually as r263717 (presubmit successful).