[headless] Add default browser context to HeadlessBrowser.

[headless] Add default browser context to HeadlessBrowser.

This also modifies HeadlessDevToolsManagerDelegate to send error
responses if required parameters are missing.

BUG=679343, 546953
Review-Url: https://codereview.chromium.org/2626823003
Cr-Commit-Position: refs/heads/master@{#443538}
Committed: https://chromium.googlesource.com/chromium/src/+/c3ead867afb360569076daac6ed5563405f395b4

[Eric Seckler, 2017-01-11 09:54:12]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 09:54:41]

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

[Eric Seckler, 2017-01-11 09:55:27]

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

[Eric Seckler, 2017-01-11 09:55:27]

Alex, PTAL :)

[alex clarke (OOO till 29th), 2017-01-11 10:06:11]

Generally seems fine to me, but I'm not really up to speed with browser contexts
so best wait for Sami's review.

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_browser_impl.h (right):

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_browser_impl.h:88: HeadlessBrowserContext*
default_browser_context_;
Nit add // NOT OWNED

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_devtools_manager_delegate.cc:52:
response->Set(kErrorParam, error_object);
This method is depreciated.  It will probably make the code clearer if you move
in a std::unique_ptr<base::DictionaryValue>, you'd need to set the int and
string before you do that ofc :)

[Eric Seckler, 2017-01-11 10:14:42]

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

[Eric Seckler, 2017-01-11 10:14:43]

On 2017/01/11 10:06:11, alex clarke wrote:
> Generally seems fine to me, but I'm not really up to speed with browser
contexts
> so best wait for Sami's review.

No worries! +Sami for review then :)

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_browser_impl.h (right):

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_browser_impl.h:88: HeadlessBrowserContext*
default_browser_context_;
On 2017/01/11 10:06:11, alex clarke wrote:
> Nit add // NOT OWNED

Done.

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2626823003/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_devtools_manager_delegate.cc:52:
response->Set(kErrorParam, error_object);
On 2017/01/11 10:06:11, alex clarke wrote:
> This method is depreciated.  It will probably make the code clearer if you
move
> in a std::unique_ptr<base::DictionaryValue>, you'd need to set the int and
> string before you do that ofc :)

Sure thing!

[Eric Seckler, 2017-01-11 10:26:10]

Patchset #2 (id:20001) has been deleted

[Sami, 2017-01-11 10:54:21]

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

https://codereview.chromium.org/2626823003/diff/40001/headless/public/headles...
headless/public/headless_browser.h:60: // new targets in by default.
One side effect of this change is that now you must call
SetDefaultBrowserContext to make createTarget (without an explicit context id)
work. I wonder if we should be a little more helpful and set the first created
context as the default? On one hand I like that you need to say explicitly what
you want, but on the other hand pretty much everyone is going to create a single
context which should be the default. WDYT?

[Eric Seckler, 2017-01-11 11:37:13]

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

https://codereview.chromium.org/2626823003/diff/40001/headless/public/headles...
headless/public/headless_browser.h:60: // new targets in by default.
On 2017/01/11 10:54:21, Sami wrote:
> One side effect of this change is that now you must call
> SetDefaultBrowserContext to make createTarget (without an explicit context id)
> work. I wonder if we should be a little more helpful and set the first created
> context as the default? On one hand I like that you need to say explicitly
what
> you want, but on the other hand pretty much everyone is going to create a
single
> context which should be the default. WDYT?

Hehe :) I pondered this a bit as well and went with explicitness in the end.

I think a problem with implicitly making the first context the default one is
that we'd then have to think about how we handle the destruction of that context
-- Would we reset the default context to null, or choose another existing
context (which) as default one? If null, do we set the next context that is
created afterwards to be the default one again?

Whatever we'd choose, it might confuse people that do something more advanced
with the contexts... But I can also see how it streamlines the simple
single-context use case.

Maybe an alternative would be to add a more descriptive error message for
Target.createTarget if no default context is set, which tells them to set one?

[Sami, 2017-01-11 18:20:30]

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

https://codereview.chromium.org/2626823003/diff/40001/headless/public/headles...
headless/public/headless_browser.h:60: // new targets in by default.
On 2017/01/11 11:37:13, Eric Seckler wrote:
> On 2017/01/11 10:54:21, Sami wrote:
> > One side effect of this change is that now you must call
> > SetDefaultBrowserContext to make createTarget (without an explicit context
id)
> > work. I wonder if we should be a little more helpful and set the first
created
> > context as the default? On one hand I like that you need to say explicitly
> what
> > you want, but on the other hand pretty much everyone is going to create a
> single
> > context which should be the default. WDYT?
> 
> Hehe :) I pondered this a bit as well and went with explicitness in the end.
> 
> I think a problem with implicitly making the first context the default one is
> that we'd then have to think about how we handle the destruction of that
context
> -- Would we reset the default context to null, or choose another existing
> context (which) as default one? If null, do we set the next context that is
> created afterwards to be the default one again?
> 
> Whatever we'd choose, it might confuse people that do something more advanced
> with the contexts... But I can also see how it streamlines the simple
> single-context use case.
> 
> Maybe an alternative would be to add a more descriptive error message for
> Target.createTarget if no default context is set, which tells them to set one?

All good points. I think we should go with making the error message point the
user to the right direction.

[Sami, 2017-01-12 17:25:12]

Oh yeah, lgtm with the error message change

[Eric Seckler, 2017-01-13 11:51:42]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-01-13 11:51:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2626823003/#ps60001
(title: "More explicit error message.")

[commit-bot: I haz the power, 2017-01-13 11:51:53]

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, 2017-01-13 12:21:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484308302033500,
"parent_rev": "6d2c4d88c7d326cd5e711b2d9710e9c4ebc19cf0", "commit_rev":
"c3ead867afb360569076daac6ed5563405f395b4"}

[commit-bot: I haz the power, 2017-01-13 12:22:10]

Description was changed from

==========
[headless] Add default browser context to HeadlessBrowser.

This also modifies HeadlessDevToolsManagerDelegate to send error
responses if required parameters are missing.

BUG=679343,546953
==========

to

==========
[headless] Add default browser context to HeadlessBrowser.

This also modifies HeadlessDevToolsManagerDelegate to send error
responses if required parameters are missing.

BUG=679343,546953

Review-Url: https://codereview.chromium.org/2626823003
Cr-Commit-Position: refs/heads/master@{#443538}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3ead867afb360569076daac6ed5...
==========

[commit-bot: I haz the power, 2017-01-13 12:22:11]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c3ead867afb360569076daac6ed5...