JavaScript Content Handler Version 0.0

mojo/apps/js/js_content_handler_app.h

+// 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.
+
+#ifndef JS_CONTENT_HANDLER_H_

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

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_G...

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

Done.

+#define JS_CONTENT_HANDLER_H_
+
+#include "base/bind.h"

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

Is this used?

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

No, I've removed it.

+#include "base/memory/ref_counted.h"
+#include "mojo/public/cpp/application/application_delegate.h"
+#include "mojo/public/cpp/application/application_impl.h"

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

I think you can forward declare ApplicationImpl to avoid this include.
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...).

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

Good point, I've forwarded it.

+#include "mojo/public/cpp/application/interface_factory_impl.h"
+#include "mojo/services/public/interfaces/content_handler/content_handler.mojom.h"
+
+namespace mojo {
+namespace apps {
+
+class JSContentHandlerApp;
+class JSApp;
+
+class JSContentHandlerImpl : public InterfaceImpl<ContentHandler> {

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

Is there any reason for the "JS" prefix? Why not just ContentHandlerImpl?

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

I thought the prefix was warranted since this was a content handler for
JavaScript. I don't mind removing the prefix however if we have a bunch of
classes all called ContentHandlerImpl (and etc) it may become confusing. The
fact there are two files called core.cc already confuses emacs gdb-mode :-).

+ public:
+  JSContentHandlerImpl(JSContentHandlerApp* content_handler_app);
+  virtual ~JSContentHandlerImpl();
+
+ private:
+  virtual void OnConnect(
+      const mojo::String& url,
+      URLResponsePtr content,
+      InterfaceRequest<ServiceProvider> service_provider) MOJO_OVERRIDE;
+
+  JSContentHandlerApp* content_handler_app_;
+};
+
+class JSContentHandlerApp :

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

I think it is confusing to call this ContentHandlerApp in the presence of the
ContentHandler mojo interface, and ContentHandlerImpl.

I know there are a lot of "apps" in this stack of abstractions ... Maybe fall
back on the interface names for help? ApplicationDelegateImpl,
ContentHandlerImpl, etc?

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

OK, I'll remove the JS prefixes

I agree that the profusion of things called application has pretty much drained
any meaning from the term.

+    public ApplicationDelegate,
+    public base::RefCountedThreadSafe<JSContentHandlerApp> {
+
+ public:
+  JSContentHandlerApp();
+  virtual void Initialize(ApplicationImpl* app) MOJO_OVERRIDE;

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

Add a comment like:

// ApplicationDelegate methods

Also, consider making these interface implementations private if they are not
actually being called except for via the interface.

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

Done.

+  virtual bool ConfigureIncomingConnection(
+      ApplicationConnection* connection) MOJO_OVERRIDE;
+
+  void StartJSApp(const std::string& url, URLResponsePtr content);
+  void QuitJSApp(JSApp* app);
+
+  void ConnectToService(ScopedMessagePipeHandle pipe_handle1,
+      const std::string& application_url, const std::string& interface_name);
+
+ private:
+  typedef std::vector<scoped_refptr<JSApp> > Apps;

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

We would typically call this type AppVector, but see comments elsewhere about
not having this.

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

I've changed the type to AppVector since we haven't decided on another way to
manage the lifetime of JSApps yet.

+
+  ApplicationImpl* applicationImpl_;

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

application_impl_, but see comments about saving Shell* instead.

+  InterfaceFactoryImplWithContext<
+     JSContentHandlerImpl, JSContentHandlerApp> content_handler_factory_;
+  Apps apps_;
+
+  virtual ~JSContentHandlerApp();

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

The ordering is incorrect. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

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

Done.

+  friend class base::RefCountedThreadSafe<JSContentHandlerApp>;
+  DISALLOW_COPY_AND_ASSIGN(JSContentHandlerApp);
+};
+
+}  // namespace apps
+}  // namespace mojo
+
+#endif  // JS_CONTENT_HANDLER_H_