JavaScript Content Handler Version 0.0

mojo/apps/js/js_app.cc

Index: mojo/apps/js/js_app.cc
diff --git a/mojo/apps/js/js_app.cc b/mojo/apps/js/js_app.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4d09c78fae5c8799f2149bbeb2d41b316084d52d
--- /dev/null
+++ b/mojo/apps/js/js_app.cc
@@ -0,0 +1,125 @@
+// 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 "base/bind.h"
+#include "gin/public/gin_embedders.h"
+#include "mojo/apps/js/js_content_handler_app.h"
+#include "mojo/apps/js/mojo_module.h"
+#include "mojo/common/data_pipe_utils.h"
+
+namespace mojo {
+namespace apps {
+
+JSAppRunnerDelegate::JSAppRunnerDelegate() {
+  AddBuiltinModule(Mojo::kModuleName, Mojo::GetModule);

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

So there is a lot of legwork in here to route these JS methods to right instance
of JSApp. Gin already includes this kind of functionality, so you don't need to
re-implement it.

The following call:

object_template_builder.SetMethod("connect",
base::Bind(&JSApp::ConnectToService, js_app))

... will route the JS method "connect" to the ConnectToService method of js_app.

This will also allow you to get rid of kEmbedderMojo and the mojo_module.*
files.

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

I've updated gin::ModuleRunnerDelegate() as we discussed: the elements of the
list of not-yet-loaded modules are now Callbacks, instead of ModuelGetter
functions. This made it possible to defined the "mojo" builtin module with a
Callback that captures the corresponding JSApp instance.

+}
+
+JSApp::JSApp(JSContentHandlerApp* content_handler_app,
+    const std::string& url,

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

You should run `git cl format` on this entire patch.

+    URLResponsePtr content)
+  : content_handler_app_(content_handler_app),
+    url_(url),
+    content_(content.Pass()),
+    thread_("Mojo JS " + url),

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

Is there a naming convention for threads in Mojo or Chrome?

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

I haven't observed one, but I'll ask around.

+    content_handler_task_runner_(
+        base::MessageLoop::current()->task_runner()) {
+
+    CHECK(on_content_handler_thread());
+}
+
+JSApp::~JSApp() {
+}
+
+// static
+JSApp* JSApp::From(v8::Isolate* isolate) {
+  return static_cast<JSApp*>(isolate->GetData(gin::kEmbedderMojo));
+}
+
+ScopedMessagePipeHandle JSApp::ConnectToService(
+  const std::string& application_url, const std::string& interface_name) {
+  CHECK(on_js_app_thread());
+  MessagePipe pipe;
+
+  // The lifetime of content_handler_app_ is managed by the ApplicationRunner.
+  content_handler_task_runner_->PostTask(FROM_HERE,
+          base::Bind(&JSContentHandlerApp::ConnectToService,
+              base::Unretained(content_handler_app_),
+          base::Passed(pipe.handle1.Pass()), application_url, interface_name));
+
+  return pipe.handle0.Pass();
+}
+
+void JSApp::Run() {
+  CHECK(!js_app_task_runner_.get() && !on_content_handler_thread());
+  js_app_task_runner_ = base::MessageLoop::current()->task_runner();
+
+  // TODO(hansmuller): check the return value and fail gracefully.
+  std::string module;
+  common::BlockingCopyToString(content_->body.Pass(), &module);
+
+  isolate_holder_.reset(
+      new gin::IsolateHolder(gin::IsolateHolder::kStrictMode));
+  // Record the mapping from this JS thread's isolate to this JSApp
+  isolate_holder_->isolate()->SetData(gin::kEmbedderMojo, this);
+
+  shell_runner_.reset(new gin::ShellRunner(&runner_delegate_,
+      isolate_holder_->isolate()));
+  // TODO(hansmuller): exiting this scope here is OK?
+  gin::Runner::Scope scope(shell_runner_.get());
+  shell_runner_->Run(module.c_str(), url_.c_str());
+}
+
+
+bool JSApp::Start() {
+  CHECK(!js_app_task_runner_.get() && on_content_handler_thread());
+  base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
+  thread_.StartWithOptions(thread_options);
+
+  // TODO(hansmuller): check thread_.StartWithOptions() return value.
+  // TODO(hansmuller): need to funnel Run() failures back to the caller.
+
+  // The lifetime of the this JSApp is managed by the content_handler_app_.

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

I understand you're trying to explain the Unretained(this), but I think having
this comment at every PostTask() is not worth the noise. Recommend removing it
here and elsewhere.

[hansmuller, 2014/09/11 16:44:43]

Done.

+  thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&JSApp::Run, base::Unretained(this)));
+  return true;
+}
+
+void JSApp::Terminate() {
+  shell_runner_.reset(NULL);
+
+  // This JSApp's thread must be stopped on the thread that started it. Ask the
+  // content_handler_app_ to drop its reference to this app, which implicitly

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

Since this class is no longer ref-counted "drop its reference" is incorrect.

[hansmuller, 2014/09/11 16:44:43]

I'll reword this.

+  // destroys this JSApp and stops its thread.
+  // The lifetime of content_handler_app_ is managed by the ApplicationRunner.
+  // The lifetime of the this JSApp is managed by the content_handler_app_.
+  content_handler_task_runner_->PostTask(FROM_HERE,
+      base::Bind(&JSContentHandlerApp::QuitJSApp,
+          base::Unretained(content_handler_app_), base::Unretained(this)));
+}
+
+void JSApp::Quit() {
+  CHECK(on_js_app_thread());
+
+  // The lifetime of the this JSApp is managed by the content_handler_app_.
+  thread_.message_loop()->PostTask(FROM_HERE,

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

This PostTask() doesn't seem necessary. We're posting to the same thread we're
already on, no?

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

Sorry, I should have included a comment to explain what's going on. I'm posting
a task instead of just doing the work directly so that the shell_runner isn't
destroyed while we're still inside the JavaScript mojo.quit() function.

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

I see. I was thinking that since the only reason that ApplicationDelegateImpl
maintains a pointer to JSApp is so that JSApp can tell it "hey, delete me now
please", then JSApp might as well just delete itself.

So basically, the AppVector would go away and Quit would turn into
thread_.message_loop()->DeleteSoon(FROM_HERE, this).

But I can see the argument that ApplicationDelegateImpl should really own these
things, too. So your way is fine.

+      base::Bind(&JSApp::Terminate, base::Unretained(this)));
+}
+
+bool JSApp::on_content_handler_thread() const {
+  return content_handler_task_runner_.get()
+      && content_handler_task_runner_.get() ==
+          base::MessageLoop::current()->task_runner().get();
+}
+
+bool JSApp::on_js_app_thread() const {
+  return js_app_task_runner_.get() &&
+      js_app_task_runner_.get() ==
+          base::MessageLoop::current()->task_runner().get();
+}
+
+}  // namespace apps
+}  // namespace mojo
+