[headless] Remove default browser context.

[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for BrowserContext and for fallback to default browser-wide settings from HeadlessBrowser::Options.
- Removed SetOptionsForTesting. Now BrowserContext should be created for this purposes.

Misc:
- Made linter happy.

BUG=546953, 624837
Committed: https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e
Cr-Commit-Position: refs/heads/master@{#409527}

[altimin, 2016-07-26 17:36:35]

Description was changed from

==========
[headless] Make user data dir configurable from headless API.

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

to

==========
[headless] Make user data dir configurable from headless API.

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

[altimin, 2016-07-26 17:36:36]

altimin@chromium.org changed reviewers:
+ skyostil@google.com

[altimin, 2016-08-02 18:15:04]

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

[altimin, 2016-08-02 18:29:32]

Description was changed from

==========
[headless] Make user data dir configurable from headless API.

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

to

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options. 
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.


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

[altimin, 2016-08-02 18:30:34]

PTAL

[Sami, 2016-08-03 11:20:29]

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

[Sami, 2016-08-03 11:20:31]

Great cleanup! Some suggestions below.

https://codereview.chromium.org/2181413002/diff/20001/headless/app/headless_s...
File headless/app/headless_shell_switches.cc (right):

https://codereview.chromium.org/2181413002/diff/20001/headless/app/headless_s...
headless/app/headless_shell_switches.cc:34: // User data directory for browser
to store its state in.
Note to self: we should have a way of building a purely in-memory context too.

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_impl.h (right):

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_impl.h:82: // web contents are
owned by |HeadlessBrowser|, here we are keeping track of
nit: capital W.

This description makes it sound like we *do* own the WebContents here. I wonder
if it could be clarified?

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_options.cc (right):

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_options.cc:15: const T&
ReturnOverridenValue(const base::Optional<T>& value,
typo: Overridden

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_impl.cc:96: auto all_web_contents
= GetAllWebContents();
Should we do this first before getting rid of the storage partitions?

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_options.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_options.h:30: // See
HeadlessBrowser::Options::proxy_server.
Since this is an internal implementation class I don't think we need to repeat
these comment pointers here.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:84: // We need to close all
WebContents here.
Is it a problem if a HeadlessBrowserContext outlives the HeadlessBrowser? I
think it's fine to require folks to delete the browser contexts before shutting
down the browser if we can make that fail in an explicit way.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_url_request_context_getter.cc (left):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_url_request_context_getter.cc:35:
std::swap(protocol_handlers_, *protocol_handlers);
We still need to apply |protocol_handlers| I think. I don't think they're used
for anything at the moment but it would be weird to just drop them on the floor.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:40:
EXPECT_EQ(browser()->GetAllWebContents(),
Should we change this to GetAllBrowserContexts to better reflect the hierarchy?
You could still use it to iterate over all tabs.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:61:
EXPECT_EQ(browser()->GetAllWebContents(),
nit: I'm happy with this but another alternative would be ASSERT_THAT(...,
ElementsAre(...));

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:67: // TODO(skyostil): Verify
viewport dimensions once we can.
No need to do this in every test :)

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_d...
File headless/lib/headless_devtools_client_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_d...
headless/lib/headless_devtools_client_browsertest.cc:215: class
BrowserDomainCreateAndDeleteBrowserContextTest
Thanks for adding this test.

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
headless/public/headless_browser.h:60: // Create a new browser context, which
can be used to isolate
Please update this to say:

Create a new browser context which can be used create tabs and isolate them from
one another.

We might also want to move this to the top of this class to make sure people
find it.

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
File headless/public/headless_browser_context.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
headless/public/headless_browser_context.h:29: // profile/session related data.
Please add a note saying when a browser context is deleted, all the associated
tabs are closed too.

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
headless/public/headless_browser_context.h:80: // Set
HeadlessBrowser::Builder::SetUserAgent.
How about formatting this thusly:

// By default HeadlessBrowserContext inherits the following options from 
// the browser instance. The methods below can be used to override these 
// settings. See HeadlessBrowser::Builder for their meaning.
Builder& SetUserAgent(const std::string& user_agent);
Builder& SetHostResolverRules(const std::string&
Builder& SetUserDataDir(const base::FilePath& user_data_dir);
...

[altimin, 2016-08-03 11:54:10]

PTAL

https://codereview.chromium.org/2181413002/diff/20001/headless/app/headless_s...
File headless/app/headless_shell_switches.cc (right):

https://codereview.chromium.org/2181413002/diff/20001/headless/app/headless_s...
headless/app/headless_shell_switches.cc:34: // User data directory for browser
to store its state in.
On 2016/08/03 11:20:30, Sami wrote:
> Note to self: we should have a way of building a purely in-memory context too.

I was planning to introduce it in a next patch - this already has a lot of
things.
We can mark context as an incognito.

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_impl.h (right):

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_impl.h:82: // web contents are
owned by |HeadlessBrowser|, here we are keeping track of
On 2016/08/03 11:20:30, Sami wrote:
> nit: capital W.
> 
> This description makes it sound like we *do* own the WebContents here. I
wonder
> if it could be clarified?

nit: fixed.

It's |HeadlessBrowserContext| here, and it is |HeadlessBrowser| who actually
owns  |WebContents|. But when |HeadlessBrowserContext| goes away, we call
Close() on appropriate |WebContents| and |HeadlessBrowser| actually deletes
them. I think that this comment reflects that.

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_options.cc (right):

https://codereview.chromium.org/2181413002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_options.cc:15: const T&
ReturnOverridenValue(const base::Optional<T>& value,
On 2016/08/03 11:20:30, Sami wrote:
> typo: Overridden

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_impl.cc:96: auto all_web_contents
= GetAllWebContents();
On 2016/08/03 11:20:30, Sami wrote:
> Should we do this first before getting rid of the storage partitions?

Very well spotted, thanks.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_options.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_options.h:30: // See
HeadlessBrowser::Options::proxy_server.
On 2016/08/03 11:20:30, Sami wrote:
> Since this is an internal implementation class I don't think we need to repeat
> these comment pointers here.

I think it's useful to distinguish overridden things and newly set things (like
protocol_handlers()).

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:84: // We need to close all
WebContents here.
On 2016/08/03 11:20:30, Sami wrote:
> Is it a problem if a HeadlessBrowserContext outlives the HeadlessBrowser? I
> think it's fine to require folks to delete the browser contexts before
shutting
> down the browser if we can make that fail in an explicit way.

We can have weak pointers to browser contexts and check that they have gone
away.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_url_request_context_getter.cc (left):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_url_request_context_getter.cc:35:
std::swap(protocol_handlers_, *protocol_handlers);
On 2016/08/03 11:20:31, Sami wrote:
> We still need to apply |protocol_handlers| I think. I don't think they're used
> for anything at the moment but it would be weird to just drop them on the
floor.

My bad. Fixed.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:40:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 11:20:31, Sami wrote:
> Should we change this to GetAllBrowserContexts to better reflect the
hierarchy?
> You could still use it to iterate over all tabs.

The problem is that we do not own browser contexts and so it can be tricky. Weak
pointers can solve the problem, however.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:61:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 11:20:31, Sami wrote:
> nit: I'm happy with this but another alternative would be ASSERT_THAT(...,
> ElementsAre(...));

Acknowledged.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:67: // TODO(skyostil): Verify
viewport dimensions once we can.
On 2016/08/03 11:20:31, Sami wrote:
> No need to do this in every test :)

Amongst our weaponry are such diverse elements as: copy and paste.
Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_d...
File headless/lib/headless_devtools_client_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_d...
headless/lib/headless_devtools_client_browsertest.cc:215: class
BrowserDomainCreateAndDeleteBrowserContextTest
On 2016/08/03 11:20:31, Sami wrote:
> Thanks for adding this test.

Acknowledged.

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
headless/public/headless_browser.h:60: // Create a new browser context, which
can be used to isolate
On 2016/08/03 11:20:31, Sami wrote:
> Please update this to say:
> 
> Create a new browser context which can be used create tabs and isolate them
from
> one another.
> 
> We might also want to move this to the top of this class to make sure people
> find it.

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
File headless/public/headless_browser_context.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
headless/public/headless_browser_context.h:29: // profile/session related data.
On 2016/08/03 11:20:31, Sami wrote:
> Please add a note saying when a browser context is deleted, all the associated
> tabs are closed too.

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/public/headles...
headless/public/headless_browser_context.h:80: // Set
HeadlessBrowser::Builder::SetUserAgent.
On 2016/08/03 11:20:31, Sami wrote:
> How about formatting this thusly:
> 
> // By default HeadlessBrowserContext inherits the following options from 
> // the browser instance. The methods below can be used to override these 
> // settings. See HeadlessBrowser::Builder for their meaning.
> Builder& SetUserAgent(const std::string& user_agent);
> Builder& SetHostResolverRules(const std::string&
> Builder& SetUserDataDir(const base::FilePath& user_data_dir);
> ...

Done.

[alex clarke (OOO till 29th), 2016-08-03 11:55:20]

https://codereview.chromium.org/2181413002/diff/40001/headless/app/headless_s...
File headless/app/headless_shell_switches.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/app/headless_s...
headless/app/headless_shell_switches.cc:34: // User data directory for browser
to store its state in.
Would it be more accurate to say:

Directory where the browser stores the user profile.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_impl.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_impl.h:82: // web contents are
owned by |HeadlessBrowser|, here we are keeping track of
nit: Web contents are owned by |HeadlessBrowser|, we keep track of...

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_options.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_options.h:59:
DISALLOW_COPY_AND_ASSIGN(HeadlessBrowserContextOptions);
I wonder if it would be simpler to remove this macro and the move constructor
and assignment operator.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:61:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 11:20:31, Sami wrote:
> nit: I'm happy with this but another alternative would be ASSERT_THAT(...,
> ElementsAre(...));

I'd prefer that, it has better error message on failure

[Eric Seckler, 2016-08-03 12:17:58]

+1 :)

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_main_parts.cc (right):

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_browser_main_parts.cc:11: #include
"headless/lib/browser/headless_devtools_manager_delegate.h"
Do we need this here?

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:33:
std::unique_ptr<HeadlessDevToolsManagerDelegate> devtools_manager_delegate =
do we need a unique_ptr here? (I find it a little confusing to see a unique_ptr
in combination with weak pointers.) maybe a simple raw pointer is more suitable?

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

https://codereview.chromium.org/2181413002/diff/80001/headless/public/headles...
headless/public/headless_browser_context.h:83: // settings. See
HeadlessBrowser::Builder for their meaning.
Nit: "HeadlessBrowser::Options" instead of "::Builder".

Alternatively, we could move the comments from HeadlessBrowser::Options here and
refer to here from there. Not sure if that'd be more appropriate.

[Sami, 2016-08-03 12:40:30]

By the way, could you link this to bug 624837 since I think the data directory
can be used to find the devtools port.

[altimin, 2016-08-03 12:42:43]

Description was changed from

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options. 
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.


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

to

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options. 
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.


BUG=546953,624837
==========

[altimin, 2016-08-03 12:46:18]

PTAL

https://codereview.chromium.org/2181413002/diff/40001/headless/app/headless_s...
File headless/app/headless_shell_switches.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/app/headless_s...
headless/app/headless_shell_switches.cc:34: // User data directory for browser
to store its state in.
On 2016/08/03 11:55:19, alex clarke wrote:
> Would it be more accurate to say:
> 
> Directory where the browser stores the user profile.

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_impl.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_impl.h:82: // web contents are
owned by |HeadlessBrowser|, here we are keeping track of
On 2016/08/03 11:55:19, alex clarke wrote:
> nit: Web contents are owned by |HeadlessBrowser|, we keep track of...

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_context_options.h (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_context_options.h:59:
DISALLOW_COPY_AND_ASSIGN(HeadlessBrowserContextOptions);
On 2016/08/03 11:55:19, alex clarke wrote:
> I wonder if it would be simpler to remove this macro and the move constructor
> and assignment operator.

I believe that in this case copy constructor is generated (and will generated a
compile error when is used) and we have to explicitly specify use of
move-constructor. So no, not simpler :)

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:61:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 11:55:19, alex clarke wrote:
> On 2016/08/03 11:20:31, Sami wrote:
> > nit: I'm happy with this but another alternative would be ASSERT_THAT(...,
> > ElementsAre(...));
> 
> I'd prefer that, it has better error message on failure

Vox Populi, Vox Dei.
Done.

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_main_parts.cc (right):

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_browser_main_parts.cc:11: #include
"headless/lib/browser/headless_devtools_manager_delegate.h"
On 2016/08/03 12:17:57, Eric Seckler wrote:
> Do we need this here?

No, thanks.

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:33:
std::unique_ptr<HeadlessDevToolsManagerDelegate> devtools_manager_delegate =
On 2016/08/03 12:17:57, Eric Seckler wrote:
> do we need a unique_ptr here? (I find it a little confusing to see a
unique_ptr
> in combination with weak pointers.) maybe a simple raw pointer is more
suitable?

We're using unique_ptr to show that we're explicitly pass ownership to caller
(TBH, GetDevToolsManagerDelegate should have returned a unique_ptr instead of
raw pointer because it takes ownership. I hope that it will be fixed when
someone has some free time).

And what is the problem with using weak pointers with unique pointer? We do not
own this object, but we want to reference it if is alive.

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

https://codereview.chromium.org/2181413002/diff/80001/headless/public/headles...
headless/public/headless_browser_context.h:83: // settings. See
HeadlessBrowser::Builder for their meaning.
On 2016/08/03 12:17:58, Eric Seckler wrote:
> Nit: "HeadlessBrowser::Options" instead of "::Builder".
> 
> Alternatively, we could move the comments from HeadlessBrowser::Options here
and
> refer to here from there. Not sure if that'd be more appropriate.

Done.

[Sami, 2016-08-03 13:46:14]

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:84: // We need to close all
WebContents here.
On 2016/08/03 11:54:10, altimin wrote:
> On 2016/08/03 11:20:30, Sami wrote:
> > Is it a problem if a HeadlessBrowserContext outlives the HeadlessBrowser? I
> > think it's fine to require folks to delete the browser contexts before
> shutting
> > down the browser if we can make that fail in an explicit way.
> 
> We can have weak pointers to browser contexts and check that they have gone
> away.

If the user needs to delete their contexts before shutting down, let's mention
this in a class comment somewhere.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:40:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 11:54:10, altimin wrote:
> On 2016/08/03 11:20:31, Sami wrote:
> > Should we change this to GetAllBrowserContexts to better reflect the
> hierarchy?
> > You could still use it to iterate over all tabs.
> 
> The problem is that we do not own browser contexts and so it can be tricky.
Weak
> pointers can solve the problem, however.

Sure, we don't own them but we can tell when they are created and deleted. I
think we should either change this to GetAllBrowserContexts() or remove it
completely since the user can iterate their own browser contexts. WDYT?

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

https://codereview.chromium.org/2181413002/diff/100001/headless/public/headle...
headless/public/headless_browser.h:40: // from one another.
nit: "them from one another"

[Eric Seckler, 2016-08-03 13:55:30]

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2181413002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:33:
std::unique_ptr<HeadlessDevToolsManagerDelegate> devtools_manager_delegate =
On 2016/08/03 12:46:18, altimin wrote:
> On 2016/08/03 12:17:57, Eric Seckler wrote:
> > do we need a unique_ptr here? (I find it a little confusing to see a
> unique_ptr
> > in combination with weak pointers.) maybe a simple raw pointer is more
> suitable?
> 
> We're using unique_ptr to show that we're explicitly pass ownership to caller
> (TBH, GetDevToolsManagerDelegate should have returned a unique_ptr instead of
> raw pointer because it takes ownership. I hope that it will be fixed when
> someone has some free time).
> 
> And what is the problem with using weak pointers with unique pointer? We do
not
> own this object, but we want to reference it if is alive. 

Hm, I guess this was a misguided feeling influenced by std::weak_ptr and not
actually applicable here :) Thanks!

[altimin, 2016-08-03 14:18:51]

PTAL

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:84: // We need to close all
WebContents here.
On 2016/08/03 13:46:14, Sami wrote:
> On 2016/08/03 11:54:10, altimin wrote:
> > On 2016/08/03 11:20:30, Sami wrote:
> > > Is it a problem if a HeadlessBrowserContext outlives the HeadlessBrowser?
I
> > > think it's fine to require folks to delete the browser contexts before
> > shutting
> > > down the browser if we can make that fail in an explicit way.
> > 
> > We can have weak pointers to browser contexts and check that they have gone
> > away.
> 
> If the user needs to delete their contexts before shutting down, let's mention
> this in a class comment somewhere.

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:40:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 13:46:14, Sami wrote:
> On 2016/08/03 11:54:10, altimin wrote:
> > On 2016/08/03 11:20:31, Sami wrote:
> > > Should we change this to GetAllBrowserContexts to better reflect the
> > hierarchy?
> > > You could still use it to iterate over all tabs.
> > 
> > The problem is that we do not own browser contexts and so it can be tricky.
> Weak
> > pointers can solve the problem, however.
> 
> Sure, we don't own them but we can tell when they are created and deleted. I
> think we should either change this to GetAllBrowserContexts() or remove it
> completely since the user can iterate their own browser contexts. WDYT?

I was planning to do it in a next patch today or tomorrow. This one is already
too big.

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

https://codereview.chromium.org/2181413002/diff/100001/headless/public/headle...
headless/public/headless_browser.h:40: // from one another.
On 2016/08/03 13:46:14, Sami wrote:
> nit: "them from one another"

Done.

[altimin, 2016-08-03 14:18:51]

PTAL

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:84: // We need to close all
WebContents here.
On 2016/08/03 13:46:14, Sami wrote:
> On 2016/08/03 11:54:10, altimin wrote:
> > On 2016/08/03 11:20:30, Sami wrote:
> > > Is it a problem if a HeadlessBrowserContext outlives the HeadlessBrowser?
I
> > > think it's fine to require folks to delete the browser contexts before
> > shutting
> > > down the browser if we can make that fail in an explicit way.
> > 
> > We can have weak pointers to browser contexts and check that they have gone
> > away.
> 
> If the user needs to delete their contexts before shutting down, let's mention
> this in a class comment somewhere.

Done.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:40:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 13:46:14, Sami wrote:
> On 2016/08/03 11:54:10, altimin wrote:
> > On 2016/08/03 11:20:31, Sami wrote:
> > > Should we change this to GetAllBrowserContexts to better reflect the
> > hierarchy?
> > > You could still use it to iterate over all tabs.
> > 
> > The problem is that we do not own browser contexts and so it can be tricky.
> Weak
> > pointers can solve the problem, however.
> 
> Sure, we don't own them but we can tell when they are created and deleted. I
> think we should either change this to GetAllBrowserContexts() or remove it
> completely since the user can iterate their own browser contexts. WDYT?

I was planning to do it in a next patch today or tomorrow. This one is already
too big.

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

https://codereview.chromium.org/2181413002/diff/100001/headless/public/headle...
headless/public/headless_browser.h:40: // from one another.
On 2016/08/03 13:46:14, Sami wrote:
> nit: "them from one another"

Done.

[Sami, 2016-08-03 14:25:49]

lgtm.

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2181413002/diff/40001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:40:
EXPECT_EQ(browser()->GetAllWebContents(),
On 2016/08/03 14:18:51, altimin wrote:
> On 2016/08/03 13:46:14, Sami wrote:
> > On 2016/08/03 11:54:10, altimin wrote:
> > > On 2016/08/03 11:20:31, Sami wrote:
> > > > Should we change this to GetAllBrowserContexts to better reflect the
> > > hierarchy?
> > > > You could still use it to iterate over all tabs.
> > > 
> > > The problem is that we do not own browser contexts and so it can be
tricky.
> > Weak
> > > pointers can solve the problem, however.
> > 
> > Sure, we don't own them but we can tell when they are created and deleted. I
> > think we should either change this to GetAllBrowserContexts() or remove it
> > completely since the user can iterate their own browser contexts. WDYT?
> 
> I was planning to do it in a next patch today or tomorrow. This one is already
> too big.

Sounds good.

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

https://codereview.chromium.org/2181413002/diff/120001/headless/public/headle...
headless/public/headless_browser.h:41: // User owns newly created
HeadlessBrowserContext and should delete them
s/them/it/

[altimin, 2016-08-03 14:27:45]

The CQ bit was checked by altimin@chromium.org

[commit-bot: I haz the power, 2016-08-03 14:28:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 16:27:03]

Description was changed from

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options. 
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.


BUG=546953,624837
==========

to

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options. 
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.


BUG=546953,624837
==========

[commit-bot: I haz the power, 2016-08-03 16:27:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-03 16:28:56]

Description was changed from

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options. 
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.


BUG=546953,624837
==========

to

==========
[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and
HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created
explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due
to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when
user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for
BrowserContext and for fallback to default browser-wide settings from
HeadlessBrowser::Options.
- Removed SetOptionsForTesting. Now BrowserContext should be created for this
purposes.

Misc:
- Made linter happy.

BUG=546953,624837

Committed: https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e
Cr-Commit-Position: refs/heads/master@{#409527}
==========

[commit-bot: I haz the power, 2016-08-03 16:28:57]

Patchset 7 (id:??) landed as
https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e
Cr-Commit-Position: refs/heads/master@{#409527}