ApplicationConnection lifetime management changes.

Changes lifetime of ApplicationConnection instances.
Before this change, ApplicationConnections were owned by the ApplicationImpl, one created for every call to ConnectToApplication/AcceptConnection. While this makes sense for incoming connections (it doesn't make sense to transfer ownership of the connection to the delegate), it doesn't for outgoing connections.

So we change ConnectToApplication to return a scoped_ptr<ApplicationConnection> which the caller must manage.

R=sky@chromium.org
http://crbug.com/519583

Committed: https://crrev.com/efecf8ac86019d7f191159279f46c1a90d071be9
Cr-Commit-Position: refs/heads/master@{#342947}

[Ben Goodger (Google), 2015-08-11 17:20:50]

ben@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-08-11 18:28:04]

https://codereview.chromium.org/1254383016/diff/160001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_apptest.cc (right):

https://codereview.chromium.org/1254383016/diff/160001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_apptest.cc:121: EXPECT_TRUE(ptr);
Won't this always be true?
Also, this test is meant to ensure the viewmanagerconnection is destroyed after
BrowserClosed(). I don't think you're doing that here.

https://codereview.chromium.org/1254383016/diff/160001/mandoline/ui/browser/d...
File mandoline/ui/browser/desktop/desktop_ui.cc (right):

https://codereview.chromium.org/1254383016/diff/160001/mandoline/ui/browser/d...
mandoline/ui/browser/desktop/desktop_ui.cc:154:
scoped_ptr<mojo::ApplicationConnection> application_connection =
Because you're putting this connection in a scoped_ptr won't the ViewEmbedder
that is registered on the next line get lost? In other words don't you need to
keep this connection around forever?

https://codereview.chromium.org/1254383016/diff/160001/mojo/application/publi...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1254383016/diff/160001/mojo/application/publi...
mojo/application/public/cpp/application_impl.h:82: // or nullptr otherwise.
Caller does not take ownership. The pointer remains
Update documentation.

https://codereview.chromium.org/1254383016/diff/160001/mojo/application/publi...
File mojo/application/public/cpp/lib/service_registry.cc (right):

https://codereview.chromium.org/1254383016/diff/160001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.cc:82:
ServiceRegistry::~ServiceRegistry() {
Move to match new position in header.

https://codereview.chromium.org/1254383016/diff/160001/mojo/application/publi...
File mojo/application/public/cpp/tests/service_registry_unittest.cc (right):

https://codereview.chromium.org/1254383016/diff/160001/mojo/application/publi...
mojo/application/public/cpp/tests/service_registry_unittest.cc:32:
scoped_ptr<ServiceRegistry> registry(new ServiceRegistry);
nit: declare on stack here and remaining places (except for line 40).

https://codereview.chromium.org/1254383016/diff/160001/mojo/common/tracing_im...
File mojo/common/tracing_impl.cc (right):

https://codereview.chromium.org/1254383016/diff/160001/mojo/common/tracing_im...
mojo/common/tracing_impl.cc:23: scoped_ptr<ApplicationConnection> connection =
Don't you need to persist this connection, else the AddService() doesn't stay
around?

[Ben Goodger (Google), 2015-08-11 21:24:25]

Updated. PTAL.

[sky, 2015-08-11 21:40:39]

LGTM

https://codereview.chromium.org/1254383016/diff/190001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_apptest.cc (right):

https://codereview.chromium.org/1254383016/diff/190001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_apptest.cc:120: ASSERT_NE(nullptr,
view_manager_connection);
nit: move this right beneath where you get it (line 116) so that it's not
related to 116-119.

[Ben Goodger (Google), 2015-08-11 22:11:28]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-08-11 22:11:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1254383016/#ps210001
(title: ".")

[commit-bot: I haz the power, 2015-08-11 22:11:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254383016/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254383016/210001

[commit-bot: I haz the power, 2015-08-11 23:06:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-11 23:06:49]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ben Goodger (Google), 2015-08-11 23:33:27]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2015-08-11 23:33:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254383016/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254383016/210001

[commit-bot: I haz the power, 2015-08-12 00:39:54]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-08-12 00:40:39]

Patchset 12 (id:??) landed as
https://crrev.com/efecf8ac86019d7f191159279f46c1a90d071be9
Cr-Commit-Position: refs/heads/master@{#342947}