headless: Initial headless embedder API implementation

headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

Adapted from a patch by Alexander Timin <altimin@chromium.org>;

BUG=546953
Committed: https://crrev.com/fe11616891b5f57ea977c24efdf7946ed2cbf5f7
Cr-Commit-Position: refs/heads/master@{#377973}

[Sami, 2016-02-08 20:57:00]

Description was changed from

==========
headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

BUG=546953
==========

to

==========
headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

Adapted from a patch by Alexander Timin <altimin@chromium.org>

BUG=546953
==========

[Sami, 2016-02-09 18:53:52]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, altimin@chromium.org

[Sami, 2016-02-09 18:54:30]

PTAL, all comments welcome.

[pfeldman, 2016-02-09 19:49:50]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-02-09 19:49:52]

I pasted links to the remote debugging analogs of what you expose. I was hoping
you could structure the API as the "embedder" and "client" tiers with the
"client" being abstracted from /content entirely. Wdyt? I can invest into
extracting the remote debug infrastructure for you to reuse. Writing a doc on
that now.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
File headless/public/headless_network.h (right):

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_network.h:23: static
scoped_refptr<net::URLRequestContextGetter>
I wonder if there is room for the platform API that embedder provides to
headless chrome vs the client API that automation and diverse clients are using.
This clearly is a part of the platform, while OpenURL is clearly a client. And I
think you should use remote debugging approach for the client API - that way you
insulate the caller from the content/ and net/ abstractions, deliver a stable
and maintainable API.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:17: namespace content {
Do you envision content/ to leak into the headless API for clients? We might
want to provide a more stable headless client API in comparison to what content/
provides today.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:40: virtual void
OnModalAlertDialog(const std::string& message) = 0;
How is this one accepted programmatically? That's what we have for web driver:
http://chromedevtools.github.io/debugger-protocol-viewer/Page/#method-handleJ...

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:87: virtual void SetViewportSize(const
gfx::Size& size) = 0;
Looks at how much more powerful the emulation one is:
http://chromedevtools.github.io/debugger-protocol-viewer/Emulation/#method-se...

emulation is now hot in the web driver / continuous integration scenarios.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:94: virtual void OpenURL(const GURL&
url) = 0;
http://chromedevtools.github.io/debugger-protocol-viewer/Page/#method-navigate

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:97: using ScreenshotCallback =
base::Callback<void(scoped_ptr<SkBitmap>)>;
http://chromedevtools.github.io/debugger-protocol-viewer/Page/#method-capture...

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
File headless/public/headless_web_frame.h (right):

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:26: virtual void ExecuteScript(const
std::string& source_code) = 0;
I think we should be reusing the remote debugging API framework for this. I'm
currently writing a doc on how it could be applied. For example, this method in
the remote debugging API already exists and supports execution contexts, return
values, exceptions, etc:
http://chromedevtools.github.io/debugger-protocol-viewer/Runtime/#method-eval...

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:42: virtual void
ExecuteScriptAndReturnValue(
Here you go, we don't need to reinvent the wheel here.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:49: virtual gfx::Size GetScrollOffset()
const = 0;
Are these provisional or are there clients for these?

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:52: virtual float GetPageScaleFactor()
const = 0;
ditto

[Sami, 2016-02-09 21:40:52]

Pavel and I had a chat and I think we're on the same page: we should separate
the client and embedder APIs clearly and to try and implement the former in
terms of a devtools protocol.

This patch was just about the embedder API, so I've now removed the provisional
client API to make that more obvious. We'll reintroduce it later once we figure
out the best mechanics for it. As an exception I've kept OpenURL around for now
just so we can do smoke testing on the headless browser.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
File headless/public/headless_network.h (right):

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_network.h:23: static
scoped_refptr<net::URLRequestContextGetter>
On 2016/02/09 19:49:51, pfeldman wrote:
> I wonder if there is room for the platform API that embedder provides to
> headless chrome vs the client API that automation and diverse clients are
using.
> This clearly is a part of the platform, while OpenURL is clearly a client. And
I
> think you should use remote debugging approach for the client API - that way
you
> insulate the caller from the content/ and net/ abstractions, deliver a stable
> and maintainable API.

I think the client can be isolated from content/ and net/, while the embedder
API might need to touch at least net to provide the level of integration needed.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:17: namespace content {
On 2016/02/09 19:49:51, pfeldman wrote:
> Do you envision content/ to leak into the headless API for clients? We might
> want to provide a more stable headless client API in comparison to what
content/
> provides today.

Right, the idea is to insulate the clients from content/. Removed these.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:40: virtual void
OnModalAlertDialog(const std::string& message) = 0;
On 2016/02/09 19:49:51, pfeldman wrote:
> How is this one accepted programmatically? That's what we have for web driver:
>
http://chromedevtools.github.io/debugger-protocol-viewer/Page/#method-handleJ...

This one was accepted automatically (unlike the two below), but I agree that a
more powerful interface would be better. Now removed.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:87: virtual void SetViewportSize(const
gfx::Size& size) = 0;
On 2016/02/09 19:49:51, pfeldman wrote:
> Looks at how much more powerful the emulation one is:
>
http://chromedevtools.github.io/debugger-protocol-viewer/Emulation/#method-se...
> 
> emulation is now hot in the web driver / continuous integration scenarios.

Yep, let's try to use that one.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:94: virtual void OpenURL(const GURL&
url) = 0;
On 2016/02/09 19:49:51, pfeldman wrote:
> http://chromedevtools.github.io/debugger-protocol-viewer/Page/#method-navigate

Ditto. However I still wanted to keep this one around for now so you can
actually do something with the headless shell and we can test that it works.
Let's replace it with a real client API later.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_contents.h:97: using ScreenshotCallback =
base::Callback<void(scoped_ptr<SkBitmap>)>;
On 2016/02/09 19:49:51, pfeldman wrote:
>
http://chromedevtools.github.io/debugger-protocol-viewer/Page/#method-capture...

Removed for now.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
File headless/public/headless_web_frame.h (right):

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:26: virtual void ExecuteScript(const
std::string& source_code) = 0;
On 2016/02/09 19:49:51, pfeldman wrote:
> I think we should be reusing the remote debugging API framework for this. I'm
> currently writing a doc on how it could be applied. For example, this method
in
> the remote debugging API already exists and supports execution contexts,
return
> values, exceptions, etc:
>
http://chromedevtools.github.io/debugger-protocol-viewer/Runtime/#method-eval...

Done.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:42: virtual void
ExecuteScriptAndReturnValue(
On 2016/02/09 19:49:51, pfeldman wrote:
> Here you go, we don't need to reinvent the wheel here.

Done.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:49: virtual gfx::Size GetScrollOffset()
const = 0;
On 2016/02/09 19:49:51, pfeldman wrote:
> Are these provisional or are there clients for these?

PhantomJS at least exposes these, not sure if anyone uses them.

https://codereview.chromium.org/1674263002/diff/80001/headless/public/headles...
headless/public/headless_web_frame.h:52: virtual float GetPageScaleFactor()
const = 0;
On 2016/02/09 19:49:51, pfeldman wrote:
> ditto

Same for these. Perhaps the emulation path could handle these?

[alex clarke (OOO till 29th), 2016-02-10 13:20:34]

Overall looks good

https://codereview.chromium.org/1674263002/diff/100001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/app/headless_...
headless/app/headless_shell.cc:57: HeadlessBrowser* browser_;
Please document ownership of this pointer.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:22: // BrowserContext
implementation
nit:  Doesn't this usually end with a : ?

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:45:
net::URLRequestContextGetter* CreateRequestContext(
Please document the ownership of the pointer returned.

Does this take ownership of |protocol_handlers|?

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:67:
net::URLRequestContextGetter* getter_;
please document ownership of getter_

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:75: content::ProtocolHandlerMap*
protocol_handlers,
Does this take ownership of |protocol_handlers|?

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:78: void
set_url_request_context_getter(HeadlessURLRequestContextGetter* getter) {
This takes ownership of getter should we pass in a
scoped_ptr<HeadlessURLRequestContextGetter>?

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_main_parts.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_main_parts.h:33: HeadlessBrowserImpl*
browser_;
Please document ownership.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_content_browser_client.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_content_browser_client.h:18:
HeadlessContentBrowserClient(HeadlessBrowserImpl* browser);
nit: explicit

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_content_browser_client.h:32: HeadlessBrowserImpl*
browser_;
Please document ownership.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_browser_browsertest.cc:17: EXPECT_TRUE(web_contents);
Can we check that the view bounds are 800x600?

Probably need to look at web_contents->web_contents()->GetViewBounds()

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
File headless/lib/headless_browser_impl.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_browser_impl.h:47: HeadlessBrowserMainParts*
browser_main_parts_;
I presume we don't own this?

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:22: // Keep in sync with
content/common/content_constants_internal.h.
Can we somehow lock that down in a test?

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:41: base::CommandLine&
command_line = *base::CommandLine::ForCurrentProcess();
Why use a reference here?

[Sami, 2016-02-10 16:43:33]

Thanks, all addressed.

https://codereview.chromium.org/1674263002/diff/100001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/app/headless_...
headless/app/headless_shell.cc:57: HeadlessBrowser* browser_;
On 2016/02/10 13:20:33, alexclarke1 wrote:
> Please document ownership of this pointer.

Done.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:22: // BrowserContext
implementation
On 2016/02/10 13:20:33, alexclarke1 wrote:
> nit:  Doesn't this usually end with a : ?

Done. Tried to be more consistent about this. Unfortunately Chrome isn't :\

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:45:
net::URLRequestContextGetter* CreateRequestContext(
On 2016/02/10 13:20:33, alexclarke1 wrote:
> Please document the ownership of the pointer returned.
> 
> Does this take ownership of |protocol_handlers|?

Done.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:67:
net::URLRequestContextGetter* getter_;
On 2016/02/10 13:20:33, alexclarke1 wrote:
> please document ownership of getter_

Done.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:75: content::ProtocolHandlerMap*
protocol_handlers,
On 2016/02/10 13:20:33, alexclarke1 wrote:
> Does this take ownership of |protocol_handlers|?

No, although it does clear it :\ Added a commment.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:78: void
set_url_request_context_getter(HeadlessURLRequestContextGetter* getter) {
On 2016/02/10 13:20:33, alexclarke1 wrote:
> This takes ownership of getter should we pass in a
> scoped_ptr<HeadlessURLRequestContextGetter>?

Done (refptr).

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_main_parts.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_browser_main_parts.h:33: HeadlessBrowserImpl*
browser_;
On 2016/02/10 13:20:34, alexclarke1 wrote:
> Please document ownership.

Done.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_content_browser_client.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_content_browser_client.h:18:
HeadlessContentBrowserClient(HeadlessBrowserImpl* browser);
On 2016/02/10 13:20:34, alexclarke1 wrote:
> nit: explicit

Done (here and elsewhere).

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_content_browser_client.h:32: HeadlessBrowserImpl*
browser_;
On 2016/02/10 13:20:34, alexclarke1 wrote:
> Please document ownership.

Done.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_browser_browsertest.cc:17: EXPECT_TRUE(web_contents);
On 2016/02/10 13:20:34, alexclarke1 wrote:
> Can we check that the view bounds are 800x600?
> 
> Probably need to look at web_contents->web_contents()->GetViewBounds()

Not currently -- I've removed that method for now. I'll add a TODO for checking
that once we have the client API for it.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
File headless/lib/headless_browser_impl.h (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_browser_impl.h:47: HeadlessBrowserMainParts*
browser_main_parts_;
On 2016/02/10 13:20:34, alexclarke1 wrote:
> I presume we don't own this?

That's right -- the code in content does. Added a comment.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:22: // Keep in sync with
content/common/content_constants_internal.h.
On 2016/02/10 13:20:34, alexclarke1 wrote:
> Can we somehow lock that down in a test?

Yes, once we have tracing capability. Added a TODO as a reminder.

https://codereview.chromium.org/1674263002/diff/100001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:41: base::CommandLine&
command_line = *base::CommandLine::ForCurrentProcess();
On 2016/02/10 13:20:34, alexclarke1 wrote:
> Why use a reference here?

You're right, a pointer is more appropriate.

[alex clarke (OOO till 29th), 2016-02-10 17:49:22]

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_content_browser_client.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_content_browser_client.h:29:
HeadlessBrowserContext* browser_context();
can this be const?

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:94: new
TCPServerSocketFactory("127.0.0.1", port));
I'm not sure how this is used yet, although I hope we can avoid the embedder
having to use localhost sockets.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.h:17:
scoped_ptr<devtools_http_handler::DevToolsHttpHandler> CreateHttpHandler(
Can we get a better name for this. Something that gives you a clue it opens a
localhost devtools port?

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_browser_impl.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_browser_impl.h:31: HeadlessBrowserMainParts*
browser_main_parts();
Can this and browser_context() be const?

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.h:50: content::BrowserContext*
browser_context();
Ditto, can this be const?

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.cc:100: HeadlessWebContents::Observer*
observer_;
I guess this isn't owned?

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.cc:107:
static_cast<HeadlessWebContentsImpl*>(web_contents)->web_contents(),
I hope these casts are safe :)

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_web_contents_impl.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.h:35: content::WebContents*
web_contents();
const?

[Sami, 2016-02-10 18:47:38]

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_content_browser_client.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_content_browser_client.h:29:
HeadlessBrowserContext* browser_context();
On 2016/02/10 17:49:22, alexclarke1 wrote:
> can this be const?

Done.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:94: new
TCPServerSocketFactory("127.0.0.1", port));
On 2016/02/10 17:49:22, alexclarke1 wrote:
> I'm not sure how this is used yet, although I hope we can avoid the embedder
> having to use localhost sockets.

Right, the embedder would only use this if they actually had another app talking
to the browser via the socket (e.g., WebDriver). We'll add a separate API for
direct access.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.h:17:
scoped_ptr<devtools_http_handler::DevToolsHttpHandler> CreateHttpHandler(
On 2016/02/10 17:49:22, alexclarke1 wrote:
> Can we get a better name for this. Something that gives you a clue it opens a
> localhost devtools port?

Good idea, done.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_browser_impl.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_browser_impl.h:31: HeadlessBrowserMainParts*
browser_main_parts();
On 2016/02/10 17:49:22, alexclarke1 wrote:
> Can this and browser_context() be const?

Done.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.h:50: content::BrowserContext*
browser_context();
On 2016/02/10 17:49:22, alexclarke1 wrote:
> Ditto, can this be const?

Done (removed).

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.cc:100: HeadlessWebContents::Observer*
observer_;
On 2016/02/10 17:49:22, alexclarke1 wrote:
> I guess this isn't owned?

Yep, the Observer owns an ObserverImpl and this is the pointer to the opposite
direction. Added a comment and renamed some things.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.cc:107:
static_cast<HeadlessWebContentsImpl*>(web_contents)->web_contents(),
On 2016/02/10 17:49:22, alexclarke1 wrote:
> I hope these casts are safe :)

I've now made the base class only implementable by HeadlessWebContentsImpl to
guarantee that.

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
File headless/lib/headless_web_contents_impl.h (right):

https://codereview.chromium.org/1674263002/diff/120001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.h:35: content::WebContents*
web_contents();
On 2016/02/10 17:49:22, alexclarke1 wrote:
> const?

Done.

[pfeldman, 2016-02-11 01:35:27]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[alex clarke (OOO till 29th), 2016-02-11 15:16:32]

lgtm

[Sami, 2016-02-11 18:12:19]

skyostil@chromium.org changed reviewers:
+ dpranke@chromium.org

[Sami, 2016-02-11 18:16:41]

skyostil@chromium.org changed reviewers:
+ agl@chromium.org, sadrul@chromium.org, sievers@chromium.org

[Sami, 2016-02-11 18:25:06]

Thanks Alex. Adding some owners:

- dpranke@: BUILD.gn

Approvals for new DEPS:

- sievers@: content/public/
- agl@: net/
- sadrul@: ui/
- pfeldman@: components/devtools_http_handler/

PTAL.

[no sievers, 2016-02-11 18:34:56]

On 2016/02/11 18:25:06, Sami wrote:
> Thanks Alex. Adding some owners:
> 
> - dpranke@: BUILD.gn
> 
> Approvals for new DEPS:
> 
> - sievers@: content/public/

no changes here

> - agl@: net/
> - sadrul@: ui/
> - pfeldman@: components/devtools_http_handler/
> 
> PTAL.

[Sami, 2016-02-11 18:37:08]

On 2016/02/11 18:34:56, sievers wrote:
> > - sievers@: content/public/
> 
> no changes here

I think I still need your stamp for the presubmit:

"""
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+components/devtools_http_handler',
    '+content/public',
    '+net',
    '+ui/aura',
    '+ui/base',
    '+ui/base/resource',
    '+ui/gfx',
    '+ui/gfx/geometry',
    '+ui/ozone/public',
"""

[no sievers, 2016-02-11 18:40:02]

On 2016/02/11 18:37:08, Sami wrote:
> On 2016/02/11 18:34:56, sievers wrote:
> > > - sievers@: content/public/
> > 
> > no changes here
> 
> I think I still need your stamp for the presubmit:
> 
> """
> Missing LGTM from OWNERS of dependencies added to DEPS:
>     '+components/devtools_http_handler',
>     '+content/public',
>     '+net',
>     '+ui/aura',
>     '+ui/base',
>     '+ui/base/resource',
>     '+ui/gfx',
>     '+ui/gfx/geometry',
>     '+ui/ozone/public',
> """

oops, my bad. lgtm

[sadrul, 2016-02-11 19:04:17]

I was going to comment that it'd be good to add ui/aura DEPS to lib/browser/,
but then noticed that headless_browser_impl and headless_web_contents_impl lives
outside of lib/browser/. Considering that these are browser-side things, should
these move into lib/browser/?

https://codereview.chromium.org/1674263002/diff/180001/headless/lib/headless_...
File headless/lib/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/180001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.cc:52: if (!parent->Contains(contents))
{
|parent| should never contain |contents| here. This could be a DCHECK instead.

Note that you don't need to create a WindowTreeHost for each WebContents. You
can add multiple WebContents and add them to the same WindowTreeHost. So
HeadlessBrowserImpl could create and own the WindowTreeHost, and send in the
aura::Window to use as the parent for the WebContent's window. It should not add
any restrictions (e.g. you would still be able to change
size/position/visibility etc. of individual windows)

[Dirk Pranke, 2016-02-11 21:44:13]

lgtm. exciting!

[Sami, 2016-02-12 12:25:08]

On 2016/02/11 19:04:17, sadrul wrote:
> I was going to comment that it'd be good to add ui/aura DEPS to lib/browser/,
> but then noticed that headless_browser_impl and headless_web_contents_impl
lives
> outside of lib/browser/. Considering that these are browser-side things,
should
> these move into lib/browser/?

Agreed, that organization makes more sense. Done.

https://codereview.chromium.org/1674263002/diff/180001/headless/lib/headless_...
File headless/lib/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/180001/headless/lib/headless_...
headless/lib/headless_web_contents_impl.cc:52: if (!parent->Contains(contents))
{
On 2016/02/11 19:04:17, sadrul wrote:
> |parent| should never contain |contents| here. This could be a DCHECK instead.

Done.

> Note that you don't need to create a WindowTreeHost for each WebContents. You
> can add multiple WebContents and add them to the same WindowTreeHost. So
> HeadlessBrowserImpl could create and own the WindowTreeHost, and send in the
> aura::Window to use as the parent for the WebContent's window. It should not
add
> any restrictions (e.g. you would still be able to change
> size/position/visibility etc. of individual windows)

Ah right, I missed the relationship of the tree host and the window here. Thanks
for the suggestion, done.

[sadrul, 2016-02-12 19:09:30]

Cool! Thanks lgtm

[agl, 2016-02-12 19:56:42]

I'm not a great reviewer for this because it plays with several bits of the
network stack that I'm not terribly familiar with. I'd recommend emailing
chrome-network-stack@ with a brief description of your plans and hopefully
someone better can take a look.

One brief comment: you're opening a local TCP port. Security-wise, is this ok?
TCP ports can be connected by anyone and it wasn't immediately obvious whether
there's any good authentication here. Have you considered a UNIX domain socket?
That's POSIX-only, of course, but it does allow filesystem permissions to be
used at least.

[Sami, 2016-02-15 12:39:08]

On 2016/02/12 19:56:42, agl wrote:
> I'm not a great reviewer for this because it plays with several bits of the
> network stack that I'm not terribly familiar with. I'd recommend emailing
> chrome-network-stack@ with a brief description of your plans and hopefully
> someone better can take a look.

No problem -- I've now sent an email asking if someone could have a look.

> One brief comment: you're opening a local TCP port. Security-wise, is this ok?
> TCP ports can be connected by anyone and it wasn't immediately obvious whether
> there's any good authentication here. Have you considered a UNIX domain
socket?
> That's POSIX-only, of course, but it does allow filesystem permissions to be
> used at least.

I agree that a UNIX socket would be superior, but unfortunately the tools that
will connect to this program (e.g., WebDriver, Telemetry) currently only
understand TCP. I think we'll eventually want to teach them about UNIX sockets,
but for now I think we'll need TCP.

One thing to note is that the user needs to set a specific command line flag
(--remote-debugging-port) to open the port, so it won't be there by default.
Chrome itself also supports the same flag and functionality and we want to be
compatible with it.

[Ryan Sleevi, 2016-02-22 21:11:12]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-02-22 21:11:13]

This code is quite difficult to review. I do not feel confident that I can
honestly give a good review, because it clearly encompasses a variety of
unrelated and under-documented (for motivation/reasoning) changes.

For the //net side, you're creating a lot of boiler plate, and it's unclear the
motivations for the boiler plate or what you hope to accomplish.

I mentioned on email the overarching design question - how much of this is
needed, particularly if you rethink the network-level integration, but for now,
some design/security points.

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
headless/app/headless_shell.cc:74: temp_port < 65535) {
SECURITY/DESIGN: Please use base/numerics for conversions like this.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:150: true /*
ignore_certificate_errors */, GetPath(),
SECURITY: *woah* this is a really really scary flag that I'd much prefer not be
introduced.

I've been pushing back on any CL that attempts to short-circuit security for
rapid iteration, because time and time again, the shortcut ends up being a
cowpath and isn't backed out.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:154:
content::BrowserThread::FILE),
DESIGN: I cannot see any reason for you to pass the naked (unsafe) message loop,
as opposed to calling "GetMessageLoopProxyForThread". The very name of these
methods was meant to discourage any new code from using it.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:54: class
HeadlessResourceContext : public content::ResourceContext {
Why don't you simply forward declare this class, avoiding leaking your
abstraction into this header.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:82: url_request_getter_ =
getter;
This is a weird API that you have two different classes for holding on to the
_getter

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:94: new
TCPServerSocketFactory("127.0.0.1", port));
Is this correct? Are you sure it should always be 127.0.0.1? For example, this
only supports IPv4, not IPv6 - which is certainly not desirable for a number of
cases.

At the least, this should be up-lifted to a file-level comment and documented.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_network_delegate.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_network_delegate.h:14: class
HeadlessNetworkDelegate : public net::NetworkDelegateImpl {
I don't believe this is strictly required - but it's also underdocumented.

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

Another set of review feedback, after trying to page in more of the context.

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
headless/app/headless_shell.cc:27: web_contents_ =
browser->CreateWebContents({800, 600});
STYLE: This is still banned - https://chromium-cpp.appspot.com/#core-blacklist

(It's also entirely non-obvious that {800, 600} translates to a gfx::Size, fwiw)

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
headless/app/headless_shell.cc:40: web_contents_->OpenURL(GURL(url));
DESIGN: You do nothing to validate the well-formedness of this URL.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:140: }
This leaves me really uncomfortable, from an API design standpoint. Are you sure
allowing consumers to override the URLRequestContextGetter is the right thing?

This is single-handedly the most complicated portion of the //net API, from
lifetime and assurances, and it feels somewhat hand-waved away, and that makes
it hard to reason about.

It also means that, from an API contract perspective, you're ignoring
|protocol_handlers| and |request_interceptors|.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:48:
content::URLRequestInterceptorScopedVector request_interceptors);
STYLE: The documentation for this (which appears to be a class method, given the
lack of override) is lacking.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:75: // URLRequestContextGetter.
Document that it does, not how it's used.
https://google.github.io/styleguide/cppguide.html#Function_Comments

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:23: const
HeadlessBrowser::Options& options)
STYLE: Your .cc order should match your .h order - this should appear at the end
of the file.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:46:
base::MessageLoop::QuitWhenIdleClosure());
Are you sure this is the right place to put this? The runloop here is
externalized in this case - isn't that the code that should be invoking
shutdown? Do you need to propogate any other work to quiesce to an idle state?

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:74: }
DESIGN: This is a confusing API contract that you allow is_null() callbacks. Is
this really what you want? Usually checking .is_null() is an anti-pattern for
callback use, and so it helps to understand.

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
File headless/public/headless_browser.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_browser.cc:17:
user_agent(content::BuildUserAgentFromProduct("HeadlessChrome")),
This is the sort of thing that likely belongs as a constant, in a well-specified
place?

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_browser.h:41: const StartCallback&
on_browser_start_callback);
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

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_browser.h:85: // See HeadlessNetwork for more details.
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)

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_web_contents.h:31: explicit
Observer(HeadlessWebContents* web_contents);
This seems like it violates the SingleResponsibilityPrinciple, and tightly links
the two objects, beyond what may be necessary. Is this CTOR needed?

(Put differently, does this need to be a concrete base class, or can it be a
pure virtual?)

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

Thanks for the detailed review and helpful comments Ryan. I've removed the
network customization API for now and pared down the remaining network plumbing
to what I think is the minimum that will let the headless shell work. Hopefully
this looks more manageable now?

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
headless/app/headless_shell.cc:27: web_contents_ =
browser->CreateWebContents({800, 600});
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> STYLE: This is still banned - https://chromium-cpp.appspot.com/#core-blacklist
> 
> (It's also entirely non-obvious that {800, 600} translates to a gfx::Size,
fwiw)

Agreed & done.

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
headless/app/headless_shell.cc:40: web_contents_->OpenURL(GURL(url));
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> DESIGN: You do nothing to validate the well-formedness of this URL.

Added validation to OpenURL.

https://codereview.chromium.org/1674263002/diff/220001/headless/app/headless_...
headless/app/headless_shell.cc:74: temp_port < 65535) {
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> SECURITY/DESIGN: Please use base/numerics for conversions like this.

Thanks for the tip -- done.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:140: }
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> This leaves me really uncomfortable, from an API design standpoint. Are you
sure
> allowing consumers to override the URLRequestContextGetter is the right thing?
> 
> This is single-handedly the most complicated portion of the //net API, from
> lifetime and assurances, and it feels somewhat hand-waved away, and that makes
> it hard to reason about.
> 
> It also means that, from an API contract perspective, you're ignoring
> |protocol_handlers| and |request_interceptors|.

Agreed on all points. I think we're still finding out what the best way to let
the embedder customize networking, so I've now left that out of this patch so
they only get the default plumbing for now.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:150: true /*
ignore_certificate_errors */, GetPath(),
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> SECURITY: *woah* this is a really really scary flag that I'd much prefer not
be
> introduced.
> 
> I've been pushing back on any CL that attempts to short-circuit security for
> rapid iteration, because time and time again, the shortcut ends up being a
> cowpath and isn't backed out.

Wow, I'm not sure why this flag was set this way -- it's definitely not what we
want or need. Thanks for pointing it out!

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:154:
content::BrowserThread::FILE),
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> DESIGN: I cannot see any reason for you to pass the naked (unsafe) message
loop,
> as opposed to calling "GetMessageLoopProxyForThread". The very name of these
> methods was meant to discourage any new code from using it.

Hmm, looks like this was some relic in Content Shell. I'll put together another
patch to fix it over there too: https://codereview.chromium.org/1730533002

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:48:
content::URLRequestInterceptorScopedVector request_interceptors);
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> STYLE: The documentation for this (which appears to be a class method, given
the
> lack of override) is lacking.

Agreed -- refactored this into HeadlessContentBrowserClient.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:54: class
HeadlessResourceContext : public content::ResourceContext {
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> Why don't you simply forward declare this class, avoiding leaking your
> abstraction into this header.

Done.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:75: // URLRequestContextGetter.
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> Document that it does, not how it's used.
> https://google.github.io/styleguide/cppguide.html#Function_Comments

Removed.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.h:82: url_request_getter_ =
getter;
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> This is a weird API that you have two different classes for holding on to the
> _getter

Agreed, I've now only made one of them hang on to the getter. FWIW the classes
can't be completely merged because content::ResourceContext needs to be
destroyed on the IO thread:

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

(Added a comment.)

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:23: const
HeadlessBrowser::Options& options)
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> STYLE: Your .cc order should match your .h order - this should appear at the
end
> of the file.

Done.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:46:
base::MessageLoop::QuitWhenIdleClosure());
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> Are you sure this is the right place to put this? The runloop here is
> externalized in this case - isn't that the code that should be invoking
> shutdown? Do you need to propogate any other work to quiesce to an idle state?


The RunLoop is actually still owned by content::BrowserMainRunner -- it's just
that the embedder will be able to override the message pump. Based on that it
seems appropriate to quit the run loop here.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:74: }
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> DESIGN: This is a confusing API contract that you allow is_null() callbacks.
Is
> this really what you want? Usually checking .is_null() is an anti-pattern for
> callback use, and so it helps to understand.

Good point. This was here only for testing -- I've added a valid callback to the
test framework instead.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:94: new
TCPServerSocketFactory("127.0.0.1", port));
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> Is this correct? Are you sure it should always be 127.0.0.1? For example, this
> only supports IPv4, not IPv6 - which is certainly not desirable for a number
of
> cases.
> 
> At the least, this should be up-lifted to a file-level comment and documented.

Agreed, I think it would be better for this to be embedder-controlled. I've
added an option for it and moved this constant to the headless shell.

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
File headless/lib/browser/headless_network_delegate.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/lib/browser/h...
headless/lib/browser/headless_network_delegate.h:14: class
HeadlessNetworkDelegate : public net::NetworkDelegateImpl {
On 2016/02/22 21:11:13, Ryan Sleevi wrote:
> I don't believe this is strictly required - but it's also underdocumented.

Looks like you're right. I'll drop it for now and see if it becomes necessary
later.

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
File headless/public/headless_browser.cc (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_browser.cc:17:
user_agent(content::BuildUserAgentFromProduct("HeadlessChrome")),
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> This is the sort of thing that likely belongs as a constant, in a
well-specified
> place?

Yep, done.

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_browser.h:41: const StartCallback&
on_browser_start_callback);
On 2016/02/22 22:01:22, Ryan Sleevi wrote:
> 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

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.

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_browser.h:85: // See HeadlessNetwork for more details.
On 2016/02/22 22:01:21, Ryan Sleevi wrote:
> 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)

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.

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1674263002/diff/220001/headless/public/headle...
headless/public/headless_web_contents.h:31: explicit
Observer(HeadlessWebContents* web_contents);
On 2016/02/22 22:01:22, Ryan Sleevi wrote:
> This seems like it violates the SingleResponsibilityPrinciple, and tightly
links
> the two objects, beyond what may be necessary. Is this CTOR needed?
> 
> (Put differently, does this need to be a concrete base class, or can it be a
> pure virtual?)

This was modeled after WebContentsObserver which has a similarly split
personality. I'm not crazy about that approach either, so I've just made this
pure virtual instead.

[Ryan Sleevi, 2016-02-25 22:04:37]

The size is greatly reduced, and I greatly appreciate that.

I've left a few remarks here, but given that you're not really touching //net
anymore, I'm going to give you an LGTM. I would encourage the other reviewers to
take a close and careful look for STYLE, DESIGN, and TEST issues; I've
highlighted a few I noticed while re-reviewing.

https://codereview.chromium.org/1674263002/diff/260001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/app/headless_...
headless/app/headless_shell.cc:42: const char kDefaultUrl[] =
"https://google.com";
Any reason this couldn't just be about:blank ?

(Reasoning: It's unclear whether //headless should have anything Google specific
/ branded)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:28: void
set_url_request_context_getter(
Document? What thread(s) can/is this called on?

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:30: url_request_context_getter_
= url_request_context_getter;
std::move

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:44:
CHECK(url_request_context_getter_);
I assume the contract here that you're trying to document is that
set_url_request_context_getter must be called before
GetHostResolver/GetRequestContext? That's something ideally documented around
line 28

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:64:
DCHECK(!on_start_callback_.is_null());
? Isn't this better dchecked in the HeadlessBrowserImpl ctor (see the recent
DCHECK discussion on chromium-dev)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:28: TCPServerSocketFactory(const
std::string& address, uint16_t port)
DESIGN: This is what IPEndPoint is meant to encapsulate.

I'm trying to get to why ListenWithAddressAndPort was introduced, as I think
that's an API wart, but for your case, it seems fine to go with the IPEndPoint
option - right?

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:93: DCHECK(address.size());
DCHECK(!address.empty())

[When dealing with STL containers, .empty() when it exists will always be O(1),
while for some containers, like std::list, .size() can be O(N); for that reason,
whenever using the STL, it's easiest to just idiomatically use .empty()]

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:94: DCHECK_GE(port, 0);
This doesn't seem one of the 'recommended' DCHECKs (see recent chromium-dev
thread)

The same applies to line 93, to be clear, but I wanted to add that remark about
.empty() ;)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_url_request_context_getter.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_url_request_context_getter.cc:63:
file_task_runner_(file_task_runner),
I like to std::move() it, std::move() it ;)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_url_request_context_getter.cc:197: for
(content::URLRequestInterceptorScopedVector::reverse_iterator i =
the original auto here was fine and in the blessed uses of it :)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
headless/lib/headless_browser_browsertest.cc:16:
browser()->CreateWebContents({800, 600});
STYLE: uniform initialization syntax is not allowed

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:100: bool r =
PathService::Get(base::DIR_MODULE, &pak_file);
STYLE: "r" is not descriptive enough.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
headless/lib/headless_web_contents_browsertest.cc:44:
base::MessageLoop::current());
o_O

ScopedNestableTasks are generally anathema - is this really needed? If so, why?
This is the kind of thing that definitely benefits from clear documentation,
since ideally we should
1) Never introduce new ones
2) Remove all existing

(OK, 2 is a bit of a pipe dream, since there are some special cases where it's
needed; but many can be refactored away)

https://codereview.chromium.org/1674263002/diff/260001/headless/public/headle...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/public/headle...
headless/public/headless_browser.h:91: Builder& EnableDevToolsServer(const
std::string& address, int port);
IPEndPoint? :)

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
File headless/test/headless_browser_test.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
headless/test/headless_browser_test.cc:30:
base::MessageLoopForUI::current()->RunUntilIdle();
Does this need to be as base::MessageLoopForUI::current()?

Does base::ThreadTaskRunnerHandle accomplish the same? Or just MessageLoop?

If none of these make sense, it may help to document why this explicitly has to
be a ForUI::current() and not any of our abstractions

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
File headless/test/headless_test_launcher.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
headless/test/headless_test_launcher.cc:24: void OnStart(HeadlessBrowser*
browser) { DCHECK(browser == this); }
You should never DCHECK in tests.

[Sami, 2016-02-26 18:49:17]

Thanks again, all addressed.

https://codereview.chromium.org/1674263002/diff/260001/headless/app/headless_...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/app/headless_...
headless/app/headless_shell.cc:42: const char kDefaultUrl[] =
"https://google.com";
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> Any reason this couldn't just be about:blank ?
> 
> (Reasoning: It's unclear whether //headless should have anything Google
specific
> / branded)

Good point, about:blank sgtm.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_context.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:28: void
set_url_request_context_getter(
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> Document? What thread(s) can/is this called on?

Done.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:30: url_request_context_getter_
= url_request_context_getter;
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> std::move

Done.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_context.cc:44:
CHECK(url_request_context_getter_);
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> I assume the contract here that you're trying to document is that
> set_url_request_context_getter must be called before
> GetHostResolver/GetRequestContext? That's something ideally documented around
> line 28

Indeed, added a comment to that effect.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_browser_impl.cc:64:
DCHECK(!on_start_callback_.is_null());
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> ? Isn't this better dchecked in the HeadlessBrowserImpl ctor (see the recent
> DCHECK discussion on chromium-dev)

Agreed & done.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:28: TCPServerSocketFactory(const
std::string& address, uint16_t port)
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> DESIGN: This is what IPEndPoint is meant to encapsulate.
> 
> I'm trying to get to why ListenWithAddressAndPort was introduced, as I think
> that's an API wart, but for your case, it seems fine to go with the IPEndPoint
> option - right?

Neat, IPEndPoint seems like good fit here. Done.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:93: DCHECK(address.size());
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> DCHECK(!address.empty())
> 
> [When dealing with STL containers, .empty() when it exists will always be
O(1),
> while for some containers, like std::list, .size() can be O(N); for that
reason,
> whenever using the STL, it's easiest to just idiomatically use .empty()]

Done.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_devtools.cc:94: DCHECK_GE(port, 0);
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> This doesn't seem one of the 'recommended' DCHECKs (see recent chromium-dev
> thread)
> 
> The same applies to line 93, to be clear, but I wanted to add that remark
about
> .empty() ;)

Did you mean the thread about DCHECKs and pointer dereferences? TBH, I'm not
completely sure how that applies here. The main thing I'm guarding against is
internal API contract violations where we try to turn on the devtools server
without a valid address. I've moved the dcheck into the socket factory -- maybe
it's more obvious that way.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
File headless/lib/browser/headless_url_request_context_getter.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_url_request_context_getter.cc:63:
file_task_runner_(file_task_runner),
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> I like to std::move() it, std::move() it ;)
> 

I've noticed :)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/browser/h...
headless/lib/browser/headless_url_request_context_getter.cc:197: for
(content::URLRequestInterceptorScopedVector::reverse_iterator i =
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> the original auto here was fine and in the blessed uses of it :)

Done. (BTW the file on the left is the blimp version of this, and I think
they've added the auto later.)

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
headless/lib/headless_browser_browsertest.cc:16:
browser()->CreateWebContents({800, 600});
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> STYLE: uniform initialization syntax is not allowed

Oops, missed this one. Fixed.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:100: bool r =
PathService::Get(base::DIR_MODULE, &pak_file);
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> STYLE: "r" is not descriptive enough.

Done.

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/lib/headless_...
headless/lib/headless_web_contents_browsertest.cc:44:
base::MessageLoop::current());
On 2016/02/25 22:04:36, Ryan Sleevi wrote:
> o_O
> 
> ScopedNestableTasks are generally anathema - is this really needed? If so,
why?
> This is the kind of thing that definitely benefits from clear documentation,
> since ideally we should
> 1) Never introduce new ones
> 2) Remove all existing
> 
> (OK, 2 is a bit of a pipe dream, since there are some special cases where it's
> needed; but many can be refactored away)

Completely agreed about nested run loops -- they've caused nothing but strife in
our scheduler work. The fundamental problem here is that:

1. gtests have to complete synchronously, so some kind of explicit run loop must
be used if we want to wait for posted tasks to complete.
2. content/ requires a message loop in order to bring up the headless browser,
so by the time we end up here we can't create another one on the same thread.

Because of this the only way to wait for code that posts tasks to complete here
is to use a nested run loop. All of the existing browser tests have the same
basic flaw.

I think the only rational way to fix this is to teach gtest (and the rest of
Chromium) about std::future or some similar promise concept so that no-one ever
needs to deal with a message loop anymore.

https://codereview.chromium.org/1674263002/diff/260001/headless/public/headle...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/public/headle...
headless/public/headless_browser.h:91: Builder& EnableDevToolsServer(const
std::string& address, int port);
On 2016/02/25 22:04:37, Ryan Sleevi wrote:
> IPEndPoint? :)

Done.

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
File headless/test/headless_browser_test.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
headless/test/headless_browser_test.cc:30:
base::MessageLoopForUI::current()->RunUntilIdle();
On 2016/02/25 22:04:37, Ryan Sleevi wrote:
> Does this need to be as base::MessageLoopForUI::current()?
> 
> Does base::ThreadTaskRunnerHandle accomplish the same? Or just MessageLoop?
> 
> If none of these make sense, it may help to document why this explicitly has
to
> be a ForUI::current() and not any of our abstractions

I only used MessageLoopForUI for the side-effect DCHECK that we're actually on
the UI thread. I've now made that explicit and just use a normal message loop.

FWIW base::ThreadTaskRunnerHandle() doesn't quite fit the bill because it just
provides the task posting part, not the run-to-idle part.

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
File headless/test/headless_test_launcher.cc (right):

https://codereview.chromium.org/1674263002/diff/260001/headless/test/headless...
headless/test/headless_test_launcher.cc:24: void OnStart(HeadlessBrowser*
browser) { DCHECK(browser == this); }
On 2016/02/25 22:04:37, Ryan Sleevi wrote:
> You should never DCHECK in tests.

Changed into EXPECT_EQ.

[pfeldman, 2016-02-26 18:55:16]

lgtm

[Sami, 2016-02-26 19:12:18]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-02-26 19:12:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org, dpranke@chromium.org, sievers@chromium.org,
sadrul@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1674263002/#ps300001
(title: "Review comments.")

[commit-bot: I haz the power, 2016-02-26 19:13:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674263002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674263002/300001

[commit-bot: I haz the power, 2016-02-26 19:40:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 19:40:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sami, 2016-02-26 19:51:19]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-02-26 19:51:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, sadrul@chromium.org, rsleevi@chromium.org,
sievers@chromium.org, pfeldman@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/1674263002/#ps320001
(title: "Fine, no console logging Mr. Presubmit.")

[commit-bot: I haz the power, 2016-02-26 19:53:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674263002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674263002/320001

[commit-bot: I haz the power, 2016-02-26 20:53:40]

Description was changed from

==========
headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

Adapted from a patch by Alexander Timin <altimin@chromium.org>

BUG=546953
==========

to

==========
headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

Adapted from a patch by Alexander Timin <altimin@chromium.org>

BUG=546953
==========

[commit-bot: I haz the power, 2016-02-26 20:53:42]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-02-26 20:59:36]

Description was changed from

==========
headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

Adapted from a patch by Alexander Timin <altimin@chromium.org>

BUG=546953
==========

to

==========
headless: Initial headless embedder API implementation

This patch introduces the initial headless embedder API implementation.
It allows the embedder to initialize a headless browsing environment and
to navigate to a URL.

We also add a headless shell application to demonstrate the use of the
embedder API.

Adapted from a patch by Alexander Timin <altimin@chromium.org>

BUG=546953

Committed: https://crrev.com/fe11616891b5f57ea977c24efdf7946ed2cbf5f7
Cr-Commit-Position: refs/heads/master@{#377973}
==========

[commit-bot: I haz the power, 2016-02-26 20:59:38]

Patchset 17 (id:??) landed as
https://crrev.com/fe11616891b5f57ea977c24efdf7946ed2cbf5f7
Cr-Commit-Position: refs/heads/master@{#377973}