JavaScript Content Handler Version 0.0

mojo/apps/js/js_content_handler_app.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "mojo/apps/js/js_app.h"
+#include "mojo/apps/js/js_content_handler_app.h"
+
+namespace mojo {
+namespace apps {
+
+JSContentHandlerApp::JSContentHandlerApp()
+    : applicationImpl_(NULL), content_handler_factory_(this) {
+}
+
+void JSContentHandlerApp::Initialize(ApplicationImpl* app) {
+  applicationImpl_ = app;
+}
+
+JSContentHandlerApp::~JSContentHandlerApp() {
+}
+
+bool JSContentHandlerApp::ConfigureIncomingConnection(
+    ApplicationConnection* connection) {
+  connection->AddService(&content_handler_factory_);
+  return true;
+}
+
+void JSContentHandlerApp::StartJSApp(
+    const std::string& url, URLResponsePtr content) {
+  JSApp* app = new JSApp(this, url, content.Pass());
+  apps_.push_back(app);

[Aaron Boodman, 2014/09/10 02:22:09]

There does not seem to be any instance of JSContentHandlerApp using this pointer
to JSApp... perhaps it should not be stored in the first place? JSApp could
perhaps manage its own lifetime.

[hansmuller, 2014/09/11 00:26:17]

At the moment the apps_ list entries defined the lifetimes of their JSApp
values. I've removed the ref counting stuff from JSApp, so this is it.
The QuitJSApp() method destroys a JSApp when it "erases" its list element.

+  // TODO(hansmuller): deal with the Start() return value.
+  app->Start();
+}
+
+void JSContentHandlerApp::QuitJSApp(JSApp* app) {
+  Apps::iterator itr = std::find(apps_.begin(), apps_.end(), app);
+  if (itr != apps_.end())
+    apps_.erase(itr);
+}
+
+void JSContentHandlerApp::ConnectToService(ScopedMessagePipeHandle pipe_handle1,

[Aaron Boodman, 2014/09/10 02:22:09]

Since there is only one pipe handle here, you don't need the 1.

[hansmuller, 2014/09/11 00:26:17]

That was just a reminder that the caller was holding on to pipe.handle0. But
maybe the suffix number is confusing.  I'll remove it.

+    const std::string& application_url, const std::string& interface_name)
+{
+  CHECK(applicationImpl_);
+  ServiceProvider *service_provider = applicationImpl_->ConnectToApplication(

[Aaron Boodman, 2014/09/10 02:22:09]

Instead of saving the ptr to ApplicationImpl*, save the ptr to
ApplicationImpl::shell(). Darin recommended in a recent CL that we eventually
remove ApplicationImpl::ConnectToApplication(), and it makes sense to me.

[hansmuller, 2014/09/11 00:26:17]

ApplicationImpl::ConnectToApplication() updates an internal service registry
after calling shell()->ConnectToApplication(). Do I need to do that somehow?

[Aaron Boodman, 2014/09/11 05:24:53]

Huh. I'm not really sure. I guess this can be part of the "figure out lifetime"
that I mentioned in the other comment.

+      application_url)->GetServiceProvider();
+  service_provider->ConnectToService(interface_name, pipe_handle1.Pass());
+}
+
+JSContentHandlerImpl::JSContentHandlerImpl(JSContentHandlerApp* app)
+    : content_handler_app_(app) {
+}
+
+JSContentHandlerImpl::~JSContentHandlerImpl() {
+}
+
+void JSContentHandlerImpl::OnConnect(
+    const mojo::String& url,
+    URLResponsePtr content,
+    InterfaceRequest<ServiceProvider> service_provider) {
+  content_handler_app_->StartJSApp(url.To<std::string>(), content.Pass());

[Aaron Boodman, 2014/09/10 02:22:09]

I'm not sure this is the right lifetime for these JSApp instances. Right now
they will never die unless they themselves call quit(). In the PNGViewer, they
go away when the view hosting them goes away. Probably neither of those is
correct.

One way to think about it is that the client called Shell::ConnectToApplication
and passed a service provider, presumably so it could get or provide some
services. So maybe the JSApp should go away when that service provider
connection goes away... Or maybe it should go away when all services vended by
that service provider have gone away...

Darin?

[Aaron Boodman, 2014/09/11 05:24:53]

We still need to figure this out. I suppose in can be a subsequent CL though.

+}
+
+}  // namespace apps
+}  // namespace mojo