Headless: make URLRequestDispatcher aware of navigations

Headless: make URLRequestDispatcher aware of navigations

DeterministicDispatcher needs to know about pending navigations in
order to ensure determinism.

headless_shell now uses this in --deterministic-fetch mode.

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

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

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

[commit-bot: I haz the power, 2017-02-09 11:21:26]

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-02-09 11:22:27]

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

[alex clarke (OOO till 29th), 2017-02-09 11:22:28]

PTAL

[alex clarke (OOO till 29th), 2017-02-09 11:39:37]

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

[commit-bot: I haz the power, 2017-02-09 11:39:44]

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

[altimin, 2017-02-09 12:01:51]

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

[altimin, 2017-02-09 12:01:52]

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.cc (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:69: void
DeterministicDispatcher::MaybeDispatchNavigationJobLocked() {
I don't like two similar methods with subtle difference. Should we refactor it
into one method, which checks first Request in the queue and add comment?

Like:

if (dispatch_pending_ || navigation_in_progress_)
  return;

if (pending_requests_.empty())
  return;

// If first url request is not ready, bail out.
if (ready_status_map_.find(pending_requests_.front().url_request &&
ready_status_map_.find(pending_requests_.front().url_request) ==
ready_status_map_.end())
  return;

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:129:
request.navigation_request->StartProcessing(
If we bailed in navigation_in_progress_, request.navigation_request could be
empty. (also: add test for this case?)

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.h (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.h:53: // TODO(alexclarke): Use
std::varient when c++17 is allowed in chromium.
nit: std::variant?

[Sami, 2017-02-09 12:03:26]

lgtm.

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.cc (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:117: navigation_in_progress_ =
true;
DCHECK(!navigation_in_progress_)?

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:138: navigation_in_progress_ =
false;
DCHECK(navigation_in_progress_)?

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.h (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.h:25: // helps make renders
deterministic at the cost of slower page loads.
Could you also add a brief description here how navigations and resource
requests interact?

[alex clarke (OOO till 29th), 2017-02-09 12:44:18]

Description was changed from

==========
Headless: make URLRequestDispatcher aware of navigations

DeterministicDispatcher needs to know about pending navigations in
order to ensure determinism.

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

to

==========
Headless: make URLRequestDispatcher aware of navigations

DeterministicDispatcher needs to know about pending navigations in
order to ensure determinism.

headless_shell now uses this in --deterministic-fetch mode.

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

[alex clarke (OOO till 29th), 2017-02-09 12:44:25]

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

[commit-bot: I haz the power, 2017-02-09 12:44:39]

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-02-09 12:44:44]

PTAL

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.cc (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:69: void
DeterministicDispatcher::MaybeDispatchNavigationJobLocked() {
On 2017/02/09 12:01:52, altimin wrote:
> I don't like two similar methods with subtle difference. Should we refactor it
> into one method, which checks first Request in the queue and add comment?
> 
> Like:
> 
> if (dispatch_pending_ || navigation_in_progress_)
>   return;
> 
> if (pending_requests_.empty())
>   return;
> 
> // If first url request is not ready, bail out.
> if (ready_status_map_.find(pending_requests_.front().url_request &&
> ready_status_map_.find(pending_requests_.front().url_request) ==
> ready_status_map_.end())
>   return;

OK I found a way of merging them.

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:117: navigation_in_progress_ =
true;
On 2017/02/09 12:03:26, Sami wrote:
> DCHECK(!navigation_in_progress_)?

In theory this is guaranteed by line 103, but I suppose it makes the intent
clearer.

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:129:
request.navigation_request->StartProcessing(
On 2017/02/09 12:01:52, altimin wrote:
> If we bailed in navigation_in_progress_, request.navigation_request could be
> empty. (also: add test for this case?)

Not sure I follow.  If we bail out, we exit the function (without poping
anything from |pending_requests|.

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:138: navigation_in_progress_ =
false;
On 2017/02/09 12:03:26, Sami wrote:
> DCHECK(navigation_in_progress_)?

Done.

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.h (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.h:25: // helps make renders
deterministic at the cost of slower page loads.
On 2017/02/09 12:03:26, Sami wrote:
> Could you also add a brief description here how navigations and resource
> requests interact?

Done.

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.h:53: // TODO(alexclarke): Use
std::varient when c++17 is allowed in chromium.
On 2017/02/09 12:01:52, altimin wrote:
> nit: std::variant?

Done.

[altimin, 2017-02-09 12:51:58]

lgtm

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.cc (right):

https://codereview.chromium.org/2687083002/diff/20001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:129:
request.navigation_request->StartProcessing(
On 2017/02/09 12:44:43, alex clarke wrote:
> On 2017/02/09 12:01:52, altimin wrote:
> > If we bailed in navigation_in_progress_, request.navigation_request could be
> > empty. (also: add test for this case?)
> 
> Not sure I follow.  If we bail out, we exit the function (without poping
> anything from |pending_requests|.

Sorry, got confused.

https://codereview.chromium.org/2687083002/diff/60001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.cc (right):

https://codereview.chromium.org/2687083002/diff/60001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:77: // Don't post a DoWork if
the first job is a url request (which isn't ready
nit: that's no DoWork :)

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

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

[commit-bot: I haz the power, 2017-02-09 13:27:28]

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

[Sami, 2017-02-09 14:05:11]

https://codereview.chromium.org/2687083002/diff/80001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2687083002/diff/80001/headless/app/headless_s...
headless/app/headless_shell.cc:62: class SimpleNavigationRequest : public
NavigationRequest {
s/Simple/Shell/? Not sure what a complex navigation request would be :)

Also would you mind moving this to a new file? It's now kind of awkwardly
sandwiched around HeadlessShell.

[commit-bot: I haz the power, 2017-02-09 14:16:24]

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

[commit-bot: I haz the power, 2017-02-09 14:16:29]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-02-09 15:08:33]

PTAL

https://codereview.chromium.org/2687083002/diff/60001/headless/public/util/de...
File headless/public/util/deterministic_dispatcher.cc (right):

https://codereview.chromium.org/2687083002/diff/60001/headless/public/util/de...
headless/public/util/deterministic_dispatcher.cc:77: // Don't post a DoWork if
the first job is a url request (which isn't ready
On 2017/02/09 12:51:58, altimin wrote:
> nit: that's no DoWork :)

Done.

https://codereview.chromium.org/2687083002/diff/80001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2687083002/diff/80001/headless/app/headless_s...
headless/app/headless_shell.cc:62: class SimpleNavigationRequest : public
NavigationRequest {
On 2017/02/09 14:05:11, Sami wrote:
> s/Simple/Shell/? Not sure what a complex navigation request would be :)
> 
> Also would you mind moving this to a new file? It's now kind of awkwardly
> sandwiched around HeadlessShell.

Done.

[alex clarke (OOO till 29th), 2017-02-09 15:08:38]

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

[commit-bot: I haz the power, 2017-02-09 15:09:07]

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

[Sami, 2017-02-09 15:12:52]

lgtm, thanks for cleaning up HeadlessShell too.

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

https://codereview.chromium.org/2687083002/diff/100001/headless/app/headless_...
headless/app/headless_shell.cc:29: class HeadlessShell;
Unneeded?

https://codereview.chromium.org/2687083002/diff/100001/headless/app/shell_nav...
File headless/app/shell_navigation_request.h (right):

https://codereview.chromium.org/2687083002/diff/100001/headless/app/shell_nav...
headless/app/shell_navigation_request.h:30: static void ProcessNavigationResult(
nit: Make private?

[alex clarke (OOO till 29th), 2017-02-09 15:55:45]

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

https://codereview.chromium.org/2687083002/diff/100001/headless/app/headless_...
headless/app/headless_shell.cc:29: class HeadlessShell;
On 2017/02/09 15:12:52, Sami wrote:
> Unneeded?

Done.

https://codereview.chromium.org/2687083002/diff/100001/headless/app/shell_nav...
File headless/app/shell_navigation_request.h (right):

https://codereview.chromium.org/2687083002/diff/100001/headless/app/shell_nav...
headless/app/shell_navigation_request.h:30: static void ProcessNavigationResult(
On 2017/02/09 15:12:52, Sami wrote:
> nit: Make private?

Done.

[alex clarke (OOO till 29th), 2017-02-09 15:55:53]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-02-09 15:55:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
altimin@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2687083002/#ps120001
(title: "Address nits")

[alex clarke (OOO till 29th), 2017-02-09 15:56:06]

Thanks all!

[commit-bot: I haz the power, 2017-02-09 15:56:16]

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-02-09 17:06:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486655753534240,
"parent_rev": "8e58f4c8544e779e5a80da2dc7321128b3de6b1d", "commit_rev":
"4cf935f5263a8ba2f4f69341085ef268b6c00617"}

[commit-bot: I haz the power, 2017-02-09 17:07:01]

Description was changed from

==========
Headless: make URLRequestDispatcher aware of navigations

DeterministicDispatcher needs to know about pending navigations in
order to ensure determinism.

headless_shell now uses this in --deterministic-fetch mode.

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

to

==========
Headless: make URLRequestDispatcher aware of navigations

DeterministicDispatcher needs to know about pending navigations in
order to ensure determinism.

headless_shell now uses this in --deterministic-fetch mode.

BUG=546953

Review-Url: https://codereview.chromium.org/2687083002
Cr-Commit-Position: refs/heads/master@{#449329}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cf935f5263a8ba2f4f69341085e...
==========

[commit-bot: I haz the power, 2017-02-09 17:07:02]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/4cf935f5263a8ba2f4f69341085e...