Make sure that Content Handled application can be connected multiple times.

sky/viewer/content_handler_impl.cc

Index: sky/viewer/content_handler_impl.cc
diff --git a/sky/viewer/content_handler_impl.cc b/sky/viewer/content_handler_impl.cc
index 6a2d103f239b3f980efbcc1e6293aa5f160d0129..fac624331b8a62c4b2c655d69a16d1b2599c60a1 100644
--- a/sky/viewer/content_handler_impl.cc
+++ b/sky/viewer/content_handler_impl.cc
@@ -4,10 +4,86 @@
 
 #include "sky/viewer/content_handler_impl.h"
 
+#include "base/bind.h"
+#include "mojo/services/public/interfaces/network/network_service.mojom.h"
 #include "sky/viewer/document_view.h"
 
 namespace sky {
 
+namespace {
+
+class EmptyServiceProvider : public mojo::InterfaceImpl<mojo::ServiceProvider> {

[Aaron Boodman, 2014/11/18 23:46:15]

unused?

[qsr, 2014/11/19 13:42:38]

Seems so.

+ private:
+  void ConnectToService(const mojo::String& service_name,
+                        mojo::ScopedMessagePipeHandle client_handle) override {}
+};
+
+}  // namespace
+
+class SkyApplication : public mojo::Application {
+ public:
+  SkyApplication(scoped_refptr<base::MessageLoopProxy> compositor_thread,
+                 mojo::ShellPtr shell,
+                 mojo::URLResponsePtr response)
+      : compositor_thread_(compositor_thread),
+        url_(response->url),
+        shell_(shell.Pass()),
+        initial_response_(response.Pass()),
+        nb_views_(0) {

[Aaron Boodman, 2014/11/18 23:46:15]

view_count_ ? or num_views_

[qsr, 2014/11/19 13:42:38]

Done.

+    shell_.set_client(this);
+    shell_->ConnectToApplication("mojo:network_service",
+                                 mojo::GetProxy(&network_service_app_));

[Aaron Boodman, 2014/11/18 23:46:15]

Just make network_service_app a local. You don't need to retain it. You can call
it service_provider, too.

[qsr, 2014/11/19 13:42:38]

 I was not sure of what will happen when I close the service provider. Done.
Thanks.

+    mojo::MessagePipe pipe;
+    network_service_app_->ConnectToService(mojo::NetworkService::Name_,

[Aaron Boodman, 2014/11/18 23:46:15]

Use mojo/public/cpp/application/connect.h

[qsr, 2014/11/19 13:42:38]

Done.

+                                           pipe.handle0.Pass());
+    network_service_.Bind(pipe.handle1.Pass());
+  }
+
+  void Initialize(mojo::Array<mojo::String> args) override {}
+
+  void AcceptConnection(const mojo::String& requestor_url,
+                        mojo::ServiceProviderPtr provider) override {
+    ++nb_views_;
+    if (initial_response_) {
+      OnResponseReceived(nullptr, provider.Pass(), initial_response_.Pass());
+    } else {
+      mojo::URLLoaderPtr* loader = new mojo::URLLoaderPtr;

[Aaron Boodman, 2014/11/18 23:46:15]

What happens if this is a normal URLLoaderPtr, and you Pass() it to Start()?
Seems like that would be the lifetime you want, but you must have tried it and
it didn't work...?

[qsr, 2014/11/19 13:42:38]

 The issue is we then do:

loader->Start(..., loader.Pass())

I'm not enough of a C++ expect to know the order of the -> and .Pass() calls.

As all I can find on order of execution is that order of discussion of
parameters is not specified, I'm not certain that I can have the -> be evaluated
before the .Pass(), and if it is not the case, then this will be a segfault.

[Aaron Boodman, 2014/11/19 15:58:15]

I pinged jyasskin, my go-to C++ expert. He says you're right -- the order is
undefined.

How about:

URLLoaderPtr loader;

< some explanatory comment >
auto loader_raw = loader.get();

loader_raw->Start(..., loader.Pass());

That way you avoid the scary-looking raw pointer at OnResponseReceived(). 

[qsr, 2014/11/19 16:37:22]

Done.

+      network_service_->CreateURLLoader(mojo::GetProxy(loader));
+      mojo::URLRequestPtr request(mojo::URLRequest::New());
+      request->url = url_;
+      request->auto_follow_redirects = true;
+      (*loader)->Start(request.Pass(),
+                       base::Bind(&SkyApplication::OnResponseReceived,
+                                  base::Unretained(this), base::Owned(loader),
+                                  base::Passed(&provider)));
+    }
+  }
+
+ private:
+  void OnViewDestroyed() {
+    --nb_views_;
+    if (nb_views_ == 0) {
+      delete this;
+    }
+  }
+
+  void OnResponseReceived(mojo::URLLoaderPtr* loader,

[Aaron Boodman, 2014/11/18 23:46:15]

Doesn't loader leak?

You may have to keep the loader alive as long as the URLResponse is, which would
mean passing into DocumentView. I can't remember the rules there.

[qsr, 2014/11/19 13:42:38]

 Unless I'm misunderstanding base::Owned, I don't think it does. The loader is
owned by the callback that will die some time just after this call.
 I don't think I need to do this. At least all seems to be working :) (and I had
test failing without this change, so we indeed reach the case where the same sky
app is connected to multiple time).

[Aaron Boodman, 2014/11/19 15:58:15]

OK, just scary-looking. See above comment.
OK, it's probably that you need to keep URLResponse alive in order to use the
body pipe. Which you're doing.

[qsr, 2014/11/19 16:37:22]

 Yes, that's for sure. The response owns the body. But I can clearly pass the
body and kill the response, and that has to work, because nothing can keep track
for struct on the other side.

+                          mojo::ServiceProviderPtr provider,
+                          mojo::URLResponsePtr response) {
+    new DocumentView(
+        base::Bind(&SkyApplication::OnViewDestroyed, base::Unretained(this)),
+        provider.Pass(), response.Pass(), shell_.get(), compositor_thread_);
+  }
+
+  scoped_refptr<base::MessageLoopProxy> compositor_thread_;
+  mojo::String url_;
+  mojo::ShellPtr shell_;
+  mojo::ServiceProviderPtr network_service_app_;

[Aaron Boodman, 2014/11/18 23:46:15]

You don't need to keep this alive. Just make it a local where it is used.

[qsr, 2014/11/19 13:42:38]

Done.

+  mojo::NetworkServicePtr network_service_;
+  mojo::URLResponsePtr initial_response_;
+  uint32_t nb_views_;
+};
+
 ContentHandlerImpl::ContentHandlerImpl(
     scoped_refptr<base::MessageLoopProxy> compositor_thread)
     : compositor_thread_(compositor_thread) {
@@ -18,7 +94,7 @@ ContentHandlerImpl::~ContentHandlerImpl() {
 
 void ContentHandlerImpl::StartApplication(mojo::ShellPtr shell,
                                           mojo::URLResponsePtr response) {
-  new DocumentView(response.Pass(), shell.Pass(), compositor_thread_);
+  new SkyApplication(compositor_thread_, shell.Pass(), response.Pass());
 }
 
 }  // namespace sky