Headless (breaking change): A better GenericURLRequestJob::Delegate API

Headless: A better GenericURLRequestJob::Delegate API

The old Delegate API made it rather awkward to get at the full details of 
the request.  This new API fixes that, whilst allowing more flexible 
request modification.

BUG=546953
Review-Url: https://codereview.chromium.org/2815003003
Cr-Commit-Position: refs/heads/master@{#464382}
Committed: https://chromium.googlesource.com/chromium/src/+/a612e798921b4a374aafdb907ed9a38ed0a6c476

[alex clarke (OOO till 29th), 2017-04-13 09:21:53]

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

[commit-bot: I haz the power, 2017-04-13 09:22:14]

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

[alex clarke (OOO till 29th), 2017-04-13 09:26:27]

Description was changed from

==========
Headless: A better GenericURLRequestJob::Delegate API

BUG=
==========

to

==========
Headless: A better GenericURLRequestJob::Delegate API

The old Delegate API made it rather awkward to get at the full details of 
the request.  This new API fixes that, whilst allowing more flexible 
request modification.

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

[alex clarke (OOO till 29th), 2017-04-13 09:26:27]

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

[alex clarke (OOO till 29th), 2017-04-13 09:27:38]

PTAL

[commit-bot: I haz the power, 2017-04-13 09:37:50]

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

[commit-bot: I haz the power, 2017-04-13 09:37:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alex clarke (OOO till 29th), 2017-04-13 09:39:51]

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

[commit-bot: I haz the power, 2017-04-13 09:40:10]

Dry run: 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-04-13 10:58:12]

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

[commit-bot: I haz the power, 2017-04-13 10:58:13]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-04-13 11:06:32]

I think this looks like a great overall simplification!

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
File headless/public/util/generic_url_request_job.cc (right):

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:196: std::string
GenericURLRequestJob::GetDevtoolsAgentHostId() const {
nit: DevTools

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:241: DCHECK(false) <<
"Unrecognized resource type";
nit: NOTREACHED

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
File headless/public/util/generic_url_request_job.h (right):

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:45: virtual std::string
GetDevtoolsAgentHostId() const = 0;
nit: DevTools

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:47: enum ResourceType {
bikeshed: enum class?

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:81: class IncomingRequest :
public Request {
Let's split this into its own class and have Request as a member to make the
relationship a bit more clear.

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:97: int http_response_code;
nit: init to zero

[alex clarke (OOO till 29th), 2017-04-13 11:15:56]

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

[alex clarke (OOO till 29th), 2017-04-13 11:16:03]

PTAL

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
File headless/public/util/generic_url_request_job.cc (right):

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:196: std::string
GenericURLRequestJob::GetDevtoolsAgentHostId() const {
On 2017/04/13 11:06:31, Sami wrote:
> nit: DevTools

Done.

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:241: DCHECK(false) <<
"Unrecognized resource type";
On 2017/04/13 11:06:31, Sami wrote:
> nit: NOTREACHED

Done.

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
File headless/public/util/generic_url_request_job.h (right):

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:45: virtual std::string
GetDevtoolsAgentHostId() const = 0;
On 2017/04/13 11:06:31, Sami wrote:
> nit: DevTools

Done.

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:47: enum ResourceType {
On 2017/04/13 11:06:31, Sami wrote:
> bikeshed: enum class?

Done.

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:81: class IncomingRequest :
public Request {
On 2017/04/13 11:06:31, Sami wrote:
> Let's split this into its own class and have Request as a member to make the
> relationship a bit more clear.

Done.

https://codereview.chromium.org/2815003003/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:97: int http_response_code;
On 2017/04/13 11:06:31, Sami wrote:
> nit: init to zero

Done.

[commit-bot: I haz the power, 2017-04-13 11:16:10]

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

[Sami, 2017-04-13 11:56:59]

Great, lgtm!

[commit-bot: I haz the power, 2017-04-13 12:12:25]

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

[commit-bot: I haz the power, 2017-04-13 12:12:26]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-04-13 12:13:47]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2017-04-13 12:13:55]

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-04-13 12:18:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492085627324960,
"parent_rev": "30b894d4f8a1c58183749fb7e0982f53dba0ba40", "commit_rev":
"a612e798921b4a374aafdb907ed9a38ed0a6c476"}

[commit-bot: I haz the power, 2017-04-13 12:19:10]

Description was changed from

==========
Headless: A better GenericURLRequestJob::Delegate API

The old Delegate API made it rather awkward to get at the full details of 
the request.  This new API fixes that, whilst allowing more flexible 
request modification.

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

to

==========
Headless: A better GenericURLRequestJob::Delegate API

The old Delegate API made it rather awkward to get at the full details of 
the request.  This new API fixes that, whilst allowing more flexible 
request modification.

BUG=546953

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

[commit-bot: I haz the power, 2017-04-13 12:19:10]

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