Add test for ServiceManager

Add test for ServiceManager

I also separated out the dynamic service loader from the ServiceManager file, to remove the dependency of the ServiceManager on the shell's context. This made testing cleaner.

BUG=None
R=viettrungluu@chromium.org, viettrungluu

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

[DaveMoore, 2014-01-03 00:02:08]

cleanup

[DaveMoore, 2014-01-03 16:52:12]

Fix compile error

[viettrungluu, 2014-01-03 17:44:49]

https://codereview.chromium.org/122173004/diff/90001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/mojo.gyp#newcode325
mojo/mojo.gyp:325: 'target_name': 'mojo_shell_unittests',
Could you add this to the list in tools/mojob.sh while you're at it?

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/context.h
File mojo/shell/context.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/context.h#new...
mojo/shell/context.h:42: ServiceManager service_manager_;
Ordering: It seems bad that |service_manager_| (slightly) outlives
|dynamic_service_loader_|, yet is never told about |dynamic_service_loader_|'s
destruction.

Probably the order should be reversed here, and a comment added.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.h:19: class DynamicServiceLoader : public
ServiceManager::Loader {
A class-level comment would be nice.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.h:33:
DISALLOW_COPY_AND_ASSIGN(DynamicServiceLoader);
#include "base/basictypes.h" for this.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager.h:34: void set_default_loader(Loader* loader) {
default_loader_ = loader; }
Who owns the default loader? (Someone else.)
And what are the requirements on its lifetime?

Or you could make set_default_loader() pass ownership (and take a scoped_ptr
argument).

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager.h:50: DISALLOW_COPY_AND_ASSIGN(ServiceManager);
nit: #include "base/basictypes.h", while you're at it.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
File mojo/shell/service_manager_unittest.cc (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:1: #include
"base/message_loop/message_loop.h"
nit: Missing lame copyright boilerplate.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:22: MOJO_OVERRIDE {
Typically, I think our pattern is to use the regular base macros, etc. (e.g.,
OVERRIDE, from base/compiler_specific.h) if we have base available.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:39: virtual void Test(const mojo::String&
test_string) MOJO_OVERRIDE {
...

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:60: virtual void AckTest() MOJO_OVERRIDE
{
...

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:103: ScopedMessagePipeHandle
shell_handle) MOJO_OVERRIDE {
...

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:110: TestApp* test_app_;
I think you should use scoped_ptrs here (and .reset() in TearDown()).

[DaveMoore, 2014-01-03 18:05:19]

Review changes

[DaveMoore, 2014-01-03 18:05:49]

https://codereview.chromium.org/122173004/diff/90001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/mojo.gyp#newcode325
mojo/mojo.gyp:325: 'target_name': 'mojo_shell_unittests',
On 2014/01/03 17:44:49, viettrungluu wrote:
> Could you add this to the list in tools/mojob.sh while you're at it?

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/context.h
File mojo/shell/context.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/context.h#new...
mojo/shell/context.h:42: ServiceManager service_manager_;
I've reversed the order and set the default loader in Context's destructor.
On 2014/01/03 17:44:49, viettrungluu wrote:
> Ordering: It seems bad that |service_manager_| (slightly) outlives
> |dynamic_service_loader_|, yet is never told about |dynamic_service_loader_|'s
> destruction.
> 
> Probably the order should be reversed here, and a comment added.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/dynamic_servi...
File mojo/shell/dynamic_service_loader.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.h:19: class DynamicServiceLoader : public
ServiceManager::Loader {
On 2014/01/03 17:44:49, viettrungluu wrote:
> A class-level comment would be nice.

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/dynamic_servi...
mojo/shell/dynamic_service_loader.h:33:
DISALLOW_COPY_AND_ASSIGN(DynamicServiceLoader);
On 2014/01/03 17:44:49, viettrungluu wrote:
> #include "base/basictypes.h" for this.

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager.h:34: void set_default_loader(Loader* loader) {
default_loader_ = loader; }
I thought it would be cleaner to have the Loaders owned externally, as they're
really part of configuration. So neither api takes ownership. I could be
convinced to change this if you feel strongly.
On 2014/01/03 17:44:49, viettrungluu wrote:
> Who owns the default loader? (Someone else.)
> And what are the requirements on its lifetime?
> 
> Or you could make set_default_loader() pass ownership (and take a scoped_ptr
> argument).

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager.h:50: DISALLOW_COPY_AND_ASSIGN(ServiceManager);
On 2014/01/03 17:44:49, viettrungluu wrote:
> nit: #include "base/basictypes.h", while you're at it.

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
File mojo/shell/service_manager_unittest.cc (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:1: #include
"base/message_loop/message_loop.h"
On 2014/01/03 17:44:49, viettrungluu wrote:
> nit: Missing lame copyright boilerplate.

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:22: MOJO_OVERRIDE {
On 2014/01/03 17:44:49, viettrungluu wrote:
> Typically, I think our pattern is to use the regular base macros, etc. (e.g.,
> OVERRIDE, from base/compiler_specific.h) if we have base available.

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:39: virtual void Test(const mojo::String&
test_string) MOJO_OVERRIDE {
On 2014/01/03 17:44:49, viettrungluu wrote:
> ...

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:60: virtual void AckTest() MOJO_OVERRIDE
{
On 2014/01/03 17:44:49, viettrungluu wrote:
> ...

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:103: ScopedMessagePipeHandle
shell_handle) MOJO_OVERRIDE {
On 2014/01/03 17:44:49, viettrungluu wrote:
> ...

Done.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:110: TestApp* test_app_;
I went back and forth on this, but didn't see any value in doing so, and this
was more direct. Why do you think the scoped_ptrs are better? Is there some
chance we could destroy the Test w/out calling TearDown()? Not clearing the
BindingsSupport is worse than leaking.

On 2014/01/03 17:44:49, viettrungluu wrote:
> I think you should use scoped_ptrs here (and .reset() in TearDown()).

[DaveMoore, 2014-01-03 18:08:15]

Review changes

[viettrungluu, 2014-01-03 18:23:14]

LGTM w/added comment and probably change to scoped_ptrs.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
File mojo/shell/service_manager.h (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager.h:34: void set_default_loader(Loader* loader) {
default_loader_ = loader; }
On 2014/01/03 18:05:49, DaveMoore wrote:
> I thought it would be cleaner to have the Loaders owned externally, as they're
> really part of configuration. So neither api takes ownership. I could be
> convinced to change this if you feel strongly.
> On 2014/01/03 17:44:49, viettrungluu wrote:
> > Who owns the default loader? (Someone else.)
> > And what are the requirements on its lifetime?
> > 
> > Or you could make set_default_loader() pass ownership (and take a scoped_ptr
> > argument).

I don't feel strongly, but then you should probably add a comment about
ownership/lifetime.

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
File mojo/shell/service_manager_unittest.cc (right):

https://codereview.chromium.org/122173004/diff/90001/mojo/shell/service_manag...
mojo/shell/service_manager_unittest.cc:110: TestApp* test_app_;
On 2014/01/03 18:05:49, DaveMoore wrote:
> I went back and forth on this, but didn't see any value in doing so, and this
> was more direct. Why do you think the scoped_ptrs are better? Is there some
> chance we could destroy the Test w/out calling TearDown()? Not clearing the
> BindingsSupport is worse than leaking.

a) It's more idiomatic (in Chromium), and makes ownership more clear.
scoped_ptrs are self-documenting; dumb pointers aren't.
b) It eliminates the risk of accidentally using the objects in question after
destruction. Or, you could reset the pointers to null after the deletes (but
then see c)).
c) It's shorter. No need to explicitly initialize to null, etc.

> 
> On 2014/01/03 17:44:49, viettrungluu wrote:
> > I think you should use scoped_ptrs here (and .reset() in TearDown()).
>

[DaveMoore, 2014-01-03 18:32:03]

Change ivars to scoped_ptrs

[commit-bot: I haz the power, 2014-01-03 18:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davemoore@chromium.org/122173004/320001

[commit-bot: I haz the power, 2014-01-03 18:48:29]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[DaveMoore, 2014-01-03 19:31:14]

Update new copyrights

[commit-bot: I haz the power, 2014-01-03 19:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davemoore@chromium.org/122173004/460001

[commit-bot: I haz the power, 2014-01-03 20:43:01]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[DaveMoore, 2014-01-03 22:50:42]

Committed patchset #7 manually as r242952 (presubmit successful).