Headless utility classes for making deterministic protocol handlers

Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

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

[alex clarke (OOO till 29th), 2016-08-19 10:32:30]

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

[alex clarke (OOO till 29th), 2016-08-19 10:32:40]

Description was changed from

==========
Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

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

to

==========
Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

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

[alex clarke (OOO till 29th), 2016-08-19 10:32:40]

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

[commit-bot: I haz the power, 2016-08-19 10:32:43]

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), 2016-08-19 10:32:48]

PTAL :)

[Sami, 2016-08-19 11:54:01]

Thanks, these will be useful. Couple of small nits.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ex...
File headless/public/util/expedited_dispatcher.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ex...
headless/public/util/expedited_dispatcher.h:19: // immediatly sacrificing
determinism for performance.
typo: immediately

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

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:154: // TODO(skyostil): Once we
have working tests, add coverage for all the cases
nit: We do have working tests so maybe reword this a bit :)

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

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:31: /*
nit: Could we standardize on // style comment blocks to keep things consistent?

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
File headless/public/util/generic_url_request_job_test.cc (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
headless/public/util/generic_url_request_job_test.cc:184: R"({"url":
"https://example.com/",
Raw string literals are still banned :( Maybe we should propose them again?

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ma...
File headless/public/util/managed_dispatch_url_request_job.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ma...
headless/public/util/managed_dispatch_url_request_job.h:18: // fetching
deterministic. NOTE UrlRequestDispatcher sub classes use
nit: URLRequestDispatcher

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/te...
File headless/public/util/testing/generic_url_request_mocks.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/te...
headless/public/util/testing/generic_url_request_mocks.h:51: class
MockCookieStore : public net::CookieStore {
Could you add a TODO to replace this with the actual CookieMonster? Looks like
it can be used as a mock directly and I think that would simplify our
dependencies a bit.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ur...
File headless/public/util/url_fetcher.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ur...
headless/public/util/url_fetcher.h:44: const std::string& final_url,
Could we use GURL for URLs everywhere?

[commit-bot: I haz the power, 2016-08-19 12:21:39]

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

[commit-bot: I haz the power, 2016-08-19 12:21:40]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-08-19 12:48:58]

All comments addressed, PTAL.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ex...
File headless/public/util/expedited_dispatcher.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ex...
headless/public/util/expedited_dispatcher.h:19: // immediatly sacrificing
determinism for performance.
On 2016/08/19 11:54:00, Sami wrote:
> typo: immediately

Done.

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

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:154: // TODO(skyostil): Once we
have working tests, add coverage for all the cases
On 2016/08/19 11:54:00, Sami wrote:
> nit: We do have working tests so maybe reword this a bit :)

Done.

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

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
headless/public/util/generic_url_request_job.h:31: /*
On 2016/08/19 11:54:00, Sami wrote:
> nit: Could we standardize on // style comment blocks to keep things
consistent?

Done.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
File headless/public/util/generic_url_request_job_test.cc (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ge...
headless/public/util/generic_url_request_job_test.cc:184: R"({"url":
"https://example.com/",
On 2016/08/19 11:54:00, Sami wrote:
> Raw string literals are still banned :( Maybe we should propose them again?

Rats, OK I'll reformat.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ma...
File headless/public/util/managed_dispatch_url_request_job.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ma...
headless/public/util/managed_dispatch_url_request_job.h:18: // fetching
deterministic. NOTE UrlRequestDispatcher sub classes use
On 2016/08/19 11:54:00, Sami wrote:
> nit: URLRequestDispatcher

Done.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/te...
File headless/public/util/testing/generic_url_request_mocks.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/te...
headless/public/util/testing/generic_url_request_mocks.h:51: class
MockCookieStore : public net::CookieStore {
On 2016/08/19 11:54:00, Sami wrote:
> Could you add a TODO to replace this with the actual CookieMonster? Looks like
> it can be used as a mock directly and I think that would simplify our
> dependencies a bit.

Done.

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ur...
File headless/public/util/url_fetcher.h (right):

https://codereview.chromium.org/2260793002/diff/20001/headless/public/util/ur...
headless/public/util/url_fetcher.h:44: const std::string& final_url,
On 2016/08/19 11:54:00, Sami wrote:
> Could we use GURL for URLs everywhere?

Done.

[alex clarke (OOO till 29th), 2016-08-19 12:49:02]

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

[commit-bot: I haz the power, 2016-08-19 12:49:19]

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

[Sami, 2016-08-19 12:53:23]

lgtm.

[alex clarke (OOO till 29th), 2016-08-19 12:55:33]

The CQ bit was unchecked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-08-19 12:55:43]

Thanks :)

[alex clarke (OOO till 29th), 2016-08-19 12:55:48]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2016-08-19 12:56:18]

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-19 14:03:06]

Description was changed from

==========
Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

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

to

==========
Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

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

[commit-bot: I haz the power, 2016-08-19 14:03:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-19 14:04:52]

Description was changed from

==========
Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

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

to

==========
Headless utility classes for making deterministic protocol handlers

In follow on patches I hope to wire up a URLFetcher backending on
URLRequestHttpJob and adding a --deterministic-network-fetch option
to headless_shell

BUG=546953

Committed: https://crrev.com/1660df547f70a27ca4b6360845febfd3adb1273f
Cr-Commit-Position: refs/heads/master@{#413130}
==========

[commit-bot: I haz the power, 2016-08-19 14:04:53]

Patchset 3 (id:??) landed as
https://crrev.com/1660df547f70a27ca4b6360845febfd3adb1273f
Cr-Commit-Position: refs/heads/master@{#413130}