headless: Initial headless embedder API implementation

headless/public/headless_browser.h

Index: headless/public/headless_browser.h
diff --git a/headless/public/headless_browser.h b/headless/public/headless_browser.h
index a94a90c7541df50567bad71262beff2b6aa2d40c..309eec0fff662d91fae25f206193bbb3061b18ec 100644
--- a/headless/public/headless_browser.h
+++ b/headless/public/headless_browser.h
@@ -5,18 +5,18 @@
 #ifndef HEADLESS_PUBLIC_HEADLESS_BROWSER_H_
 #define HEADLESS_PUBLIC_HEADLESS_BROWSER_H_
 
+#include <string>
+
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "headless/public/headless_export.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace base {
+class MessagePump;
 class SingleThreadTaskRunner;
-
-namespace trace_event {
-class TraceConfig;
-}
 }
 
 namespace gfx {
@@ -28,42 +28,38 @@ class URLRequestContextGetter;
 }
 
 namespace headless {
-class WebContents;
+class HeadlessWebContents;
 
 class HEADLESS_EXPORT HeadlessBrowser {
  public:
-  static HeadlessBrowser* Get();
-
   struct Options;
 
   // Main routine for running browser.
   // Takes command line args and callback to run as soon as browser starts.
-  static int Run(
-      const Options& options,
-      const base::Callback<void(HeadlessBrowser*)>& on_browser_start_callback);
+  using StartCallback = base::Callback<void(HeadlessBrowser*)>;
+  static int Run(const Options& options,
+                 const StartCallback& on_browser_start_callback);

[Ryan Sleevi, 2016/02/22 22:01:22]

DESIGN: It's worth highlighting that this is a weird API, that you have a static
method on a pure virtual base class. I'm not even sure how to reason how the
abstraction is meant to work, simply from the header.

STYLE: Also, why the abstraction with the typedef? It seems to go against the
general style of
https://google.github.io/styleguide/cppguide.html#Declaration_Order

[Sami, 2016/02/23 20:19:08]

I've now refactored this to split out the main entrypoint. Hopefully it makes it
more obvious how to boot up the browser. PTAL.

The typedef was there just to shorten some of the identifiers. I've taken it
out.

 
-  // Create a new browser tab.
-  virtual scoped_ptr<WebContents> CreateWebContents(const gfx::Size& size) = 0;
+  // Create a new browser tab. |size| is in physical pixels.
+  virtual scoped_ptr<HeadlessWebContents> CreateWebContents(
+      const gfx::Size& size) = 0;
 
-  virtual scoped_refptr<base::SingleThreadTaskRunner> BrowserMainThread() = 0;
-  virtual scoped_refptr<base::SingleThreadTaskRunner> RendererMainThread() = 0;
+  virtual scoped_refptr<base::SingleThreadTaskRunner> BrowserMainThread()
+      const = 0;
 
   // Requests browser to stop as soon as possible.
   // |Run| will return as soon as browser stops.
   virtual void Stop() = 0;
 
-  virtual void StartTracing(const base::trace_event::TraceConfig& trace_config,
-                            const base::Closure& on_tracing_started) = 0;
-  virtual void StopTracing(const std::string& log_file_name,
-                           const base::Closure& on_tracing_stopped) = 0;
-
  protected:
+  HeadlessBrowser() {}
   virtual ~HeadlessBrowser() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(HeadlessBrowser);
 };
 
+// Embedding API overrides for the headless browser.
 struct HeadlessBrowser::Options {
   ~Options();
 
@@ -77,19 +73,20 @@ struct HeadlessBrowser::Options {
   std::string navigator_platform;
 
   static const int kInvalidPort = -1;
-  // If not null, create start devtools for remote debugging
-  // on specified port.
   int devtools_http_port;
 
-  // Optional URLRequestContextGetter for customizing network stack.
-  // Allows overriding:
+  // Optional URLRequestContextGetter for customizing network stack. Allows
+  // overriding, e.g.:
   // - Cookie storage
   // - HTTP cache
   // - SSL config
   // - Proxy service
+  //
+  // See HeadlessNetwork for more details.

[Ryan Sleevi, 2016/02/22 22:01:21]

DESIGN/LAYER: This seems to document your implementation details, which seems
counter in headless/public.

Put differently, will this implementation always depend on HeadlessNetwork? Do
callers need to be informed of this? Separately, if all the useful documentation
is in HeadlessNetwork, why isn't this exposed by providing that class explicitly
here, making such documentation explicit?

As far as documentation goes, this seems both over-specific and yet
under-specified - it doesn't provide enough context to know what matters for
HeadlessNetwork, but also documents there's a dependency that callers might
otherwise be unaware of (through good layering)

[Sami, 2016/02/23 20:19:08]

Agreed, I think we'll need to do some more design work for this part to come up
with a more precisely defined API. Overriding URLRequestContextGetter was just
the first approximation from earlier prototyping. What I'll do is remove this
customization point for now and start a design doc that we can evolve to a
better approach.

   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter;
 
-  scoped_ptr<base::MessagePump> message_pump;
+  // Optional message pump that overrides the default. Must outlive the browser.
+  base::MessagePump* message_pump;
 
  private:
   Options(int argc, const char** argv);
@@ -104,6 +101,7 @@ class HeadlessBrowser::Options::Builder {
   Builder& EnableDevToolsServer(int port);
   Builder& SetURLRequestContextGetter(
       scoped_refptr<net::URLRequestContextGetter> url_request_context_getter);
+  Builder& SetMessagePump(base::MessagePump* message_pump);
 
   Options Build();