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

Review URL: https://codereview.chromium.org/1254383016

Cr-Commit-Position: refs/heads/master@{#342947}

5 files changed, 15 insertions, 20 deletions

diff --git a/mandoline/tab/frame_connection.h b/mandoline/tab/frame_connection.h
index 2aa3558..63065e9 100644
--- a/mandoline/tab/frame_connection.h
+++ b/mandoline/tab/frame_connection.h
@@ -6,6 +6,7 @@
 #define MANDOLINE_TAB_FRAME_CONNECTION_H_
 
 #include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
 #include "components/view_manager/public/interfaces/view_manager.mojom.h"
 #include "mandoline/tab/frame_user_data.h"
 #include "mandoline/tab/public/interfaces/frame_tree.mojom.h"
@@ -33,14 +34,14 @@ class FrameConnection : public FrameUserData {
   FrameTreeClient* frame_tree_client() { return frame_tree_client_.get(); }
 
   mojo::ApplicationConnection* application_connection() {
-    return application_connection_;
+    return application_connection_.get();
   }
 
  private:
   FrameTreeClientPtr frame_tree_client_;
 
   // TODO(sky): needs to be destroyed when connection lost.
-  mojo::ApplicationConnection* application_connection_;
+  scoped_ptr<mojo::ApplicationConnection> application_connection_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameConnection);
 };
diff --git a/mandoline/ui/browser/browser.cc b/mandoline/ui/browser/browser.cc
index 15ab1f2..c8cdac3 100644
--- a/mandoline/ui/browser/browser.cc
+++ b/mandoline/ui/browser/browser.cc
@@ -117,7 +117,7 @@ void Browser::OnEmbedForDescendant(mojo::View* view,
   if (!frame || !frame->HasAncestor(frame_tree_->root())) {
     // TODO(sky): add requestor url so that we can return false if it's not
     // an app we expect.
-    mojo::ApplicationConnection* connection =
+    scoped_ptr<mojo::ApplicationConnection> connection =
         app_->ConnectToApplication(request.Pass());
     connection->ConnectToService(client);
     return;
@@ -215,7 +215,7 @@ void Browser::ShowOmnibox(mojo::URLRequestPtr request) {
     omnibox_->SetBounds(root_->bounds());
   }
   mojo::ViewManagerClientPtr view_manager_client;
-  mojo::ApplicationConnection* connection =
+  scoped_ptr<mojo::ApplicationConnection> connection =
       app_->ConnectToApplication(request.Pass());
   connection->AddService<ViewEmbedder>(this);
   connection->ConnectToService(&view_manager_client);
diff --git a/mandoline/ui/browser/browser_apptest.cc b/mandoline/ui/browser/browser_apptest.cc
index f99a842..51815b9 100644
--- a/mandoline/ui/browser/browser_apptest.cc
+++ b/mandoline/ui/browser/browser_apptest.cc
@@ -52,7 +52,6 @@ class BrowserTest : public mojo::test::ApplicationTestBase,
  public:
   BrowserTest()
       : app_(nullptr),
-        last_closed_connection_(nullptr),
         last_browser_closed_(nullptr) {}
 
   // Creates a new Browser object.
@@ -75,21 +74,11 @@ class BrowserTest : public mojo::test::ApplicationTestBase,
     return last_browser;
   }
 
-  // Returns the last ApplicationConnection closed.
-  void* last_closed_connection() {
-    return last_closed_connection_;
-  }
-
   // Overridden from ApplicationDelegate:
   void Initialize(mojo::ApplicationImpl* app) override {
     app_ = app;
   }
 
-  void OnWillCloseConnection(mojo::ApplicationConnection* connection) override {
-    // WARNING: DO NOT FOLLOW THIS POINTER. IT WILL BE DESTROYED.
-    last_closed_connection_ = connection;
-  }
-
   // ApplicationTestBase:
   ApplicationDelegate* GetApplicationDelegate() override { return this; }
 
@@ -110,7 +99,6 @@ class BrowserTest : public mojo::test::ApplicationTestBase,
 
  private:
   mojo::ApplicationImpl* app_;
-  void* last_closed_connection_;
   std::set<TestBrowser*> browsers_;
   TestBrowser* last_browser_closed_;
   scoped_ptr<base::RunLoop> browser_closed_run_loop_;
@@ -125,9 +113,13 @@ TEST_F(BrowserTest, ClosingBrowserClosesAppConnection) {
   ASSERT_NE(nullptr, browser);
   mojo::ApplicationConnection* view_manager_connection =
       browser->view_manager_init_.connection();
+  mojo::ApplicationConnection::TestApi connection_test_api(
+      view_manager_connection);
   ASSERT_NE(nullptr, view_manager_connection);
+  base::WeakPtr<mojo::ApplicationConnection> ptr =
+      connection_test_api.GetWeakPtr();
   BrowserClosed(browser);
-  EXPECT_EQ(last_closed_connection(), view_manager_connection);
+  EXPECT_FALSE(ptr);
 }
 
 // This test verifies that we can create two Browsers and each Browser has a
diff --git a/mandoline/ui/browser/desktop/desktop_ui.cc b/mandoline/ui/browser/desktop/desktop_ui.cc
index 889ee64..2e5737c 100644
--- a/mandoline/ui/browser/desktop/desktop_ui.cc
+++ b/mandoline/ui/browser/desktop/desktop_ui.cc
@@ -151,10 +151,10 @@ void DesktopUI::ButtonPressed(views::Button* sender, const ui::Event& event) {
     DCHECK(!client_binding_.is_bound());
     mojo::URLRequestPtr request(mojo::URLRequest::New());
     request->url = mojo::String::From("mojo:omnibox");
-    mojo::ApplicationConnection* application_connection =
+    omnibox_connection_ =
         application_impl_->ConnectToApplication(request.Pass());
-    application_connection->AddService<ViewEmbedder>(browser_);
-    application_connection->ConnectToService(&omnibox_);
+    omnibox_connection_->AddService<ViewEmbedder>(browser_);
+    omnibox_connection_->ConnectToService(&omnibox_);
     OmniboxClientPtr client;
     client_binding_.Bind(&client);
     omnibox_->SetClient(client.Pass());
diff --git a/mandoline/ui/browser/desktop/desktop_ui.h b/mandoline/ui/browser/desktop/desktop_ui.h
index 90ed638..6f5ce3f 100644
--- a/mandoline/ui/browser/desktop/desktop_ui.h
+++ b/mandoline/ui/browser/desktop/desktop_ui.h
@@ -11,6 +11,7 @@
 #include "ui/views/layout/layout_manager.h"
 
 namespace mojo {
+class ApplicationConnection;
 class Shell;
 class View;
 }
@@ -53,6 +54,7 @@ class DesktopUI : public BrowserUI,
   mojo::View* content_;
   OmniboxPtr omnibox_;
   mojo::Binding<OmniboxClient> client_binding_;
+  scoped_ptr<mojo::ApplicationConnection> omnibox_connection_;
 
   DISALLOW_COPY_AND_ASSIGN(DesktopUI);
 };