Add a signal to the scheduler that a navigation is expected

Add a signal to the scheduler that a navigation is expected

Under some circumstances, the scheduler temporarily blocks
expensive tasks to improve responsiveness to user input.  We
don't want to delay navigations and the signal this patch
adds prevents any scheduler imposed delay.

Also we rename the OnPageLoad to OnNavigate for consistency.

BUG=497761, 510398
Committed: https://crrev.com/c403394390922bcaab4318a50a6b405439d7ca7d
Cr-Commit-Position: refs/heads/master@{#351052}

[alex clarke (OOO till 29th), 2015-09-24 11:35:21]

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

[alex clarke (OOO till 29th), 2015-09-24 11:41:02]

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

[commit-bot: I haz the power, 2015-09-24 11:41:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/1

[commit-bot: I haz the power, 2015-09-24 12:50:00]

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

[commit-bot: I haz the power, 2015-09-24 12:50:01]

Dry run: This issue passed the CQ dry run.

[Sami, 2015-09-24 14:15:05]

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:821: base::AutoLock
lock(any_thread_lock_);
nit: helper_.CheckOnValidThread();

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:827: base::AutoLock
lock(any_thread_lock_);
Ditto.

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:139: //
OnPageLoadExpected.
s/PageLoad/Navigation/ (in the variable name too)

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:256: bool
navigation_expected;
Should this on the main thread struct instead? I think we're a bit inconsistent
about which thread owns which state, but navigation feels like it should be a
main thread only thing.

https://codereview.chromium.org/1368643002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebScheduler.h (right):

https://codereview.chromium.org/1368643002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebScheduler.h:87: // Tells the scheduler
that a navigation is expected. This temporarially
nit: I don't think we need to specify here how this will be affect the policy --
will definitely forget to update it :)

https://codereview.chromium.org/1368643002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebScheduler.h:89: virtual void
navigationExpected() { }
nit: should all of these be onNavigation[...] to match content/?

[alex clarke (OOO till 29th), 2015-09-24 14:53:10]

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:821: base::AutoLock
lock(any_thread_lock_);
On 2015/09/24 14:15:04, Sami wrote:
> nit: helper_.CheckOnValidThread();

Done.

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:827: base::AutoLock
lock(any_thread_lock_);
On 2015/09/24 14:15:04, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:139: //
OnPageLoadExpected.
On 2015/09/24 14:15:04, Sami wrote:
> s/PageLoad/Navigation/ (in the variable name too)

I don't need this constant anymore.

https://codereview.chromium.org/1368643002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:256: bool
navigation_expected;
On 2015/09/24 14:15:04, Sami wrote:
> Should this on the main thread struct instead? I think we're a bit
inconsistent
> about which thread owns which state, but navigation feels like it should be a
> main thread only thing.

Done.

https://codereview.chromium.org/1368643002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebScheduler.h (right):

https://codereview.chromium.org/1368643002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebScheduler.h:87: // Tells the scheduler
that a navigation is expected. This temporarially
On 2015/09/24 14:15:04, Sami wrote:
> nit: I don't think we need to specify here how this will be affect the policy
--
> will definitely forget to update it :)

Done.

https://codereview.chromium.org/1368643002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebScheduler.h:89: virtual void
navigationExpected() { }
On 2015/09/24 14:15:04, Sami wrote:
> nit: should all of these be onNavigation[...] to match content/?

Done.

[Sami, 2015-09-24 15:16:15]

Great, components/scheduler lgtm with a nit.

https://codereview.chromium.org/1368643002/diff/20001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1368643002/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:823: base::AutoLock
lock(any_thread_lock_);
nit: Just call UpdatePolicy() to avoid some boilerplate?

https://codereview.chromium.org/1368643002/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:830: base::AutoLock
lock(any_thread_lock_);
Ditto.

[alex clarke (OOO till 29th), 2015-09-24 15:36:30]

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

[alex clarke (OOO till 29th), 2015-09-24 15:37:15]

+jochen@ can you please review the content/* and WebKit changes?

https://codereview.chromium.org/1368643002/diff/20001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1368643002/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:823: base::AutoLock
lock(any_thread_lock_);
On 2015/09/24 15:16:15, Sami wrote:
> nit: Just call UpdatePolicy() to avoid some boilerplate?

Done.

https://codereview.chromium.org/1368643002/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:830: base::AutoLock
lock(any_thread_lock_);
On 2015/09/24 15:16:15, Sami wrote:
> Ditto.

Done.

[alex clarke (OOO till 29th), 2015-09-25 09:39:04]

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

[commit-bot: I haz the power, 2015-09-25 09:39:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/60001

[commit-bot: I haz the power, 2015-09-25 10:52:29]

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

[commit-bot: I haz the power, 2015-09-25 10:52:29]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2015-09-25 11:21:52]

what if two navigations get scheduled, and one is cancelled?

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

On 2015/09/25 11:21:52, jochen wrote:
> what if two navigations get scheduled, and one is cancelled?

Looking at NavigationScheduler::schedule it always calls cancel() before
startTimer() so that can't happen there.  However it could happen if the API was
used elsewhere, so lets add an assert to check for double navigation.

[jochen (gone - plz use gerrit), 2015-09-25 12:17:24]

lgtm

[alex clarke (OOO till 29th), 2015-09-25 12:37:05]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-09-25 12:37:05]

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/1368643002/#ps80001
(title: "assert to check for double navigation")

[commit-bot: I haz the power, 2015-09-25 12:37:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/80001

[commit-bot: I haz the power, 2015-09-25 13:04:15]

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

[commit-bot: I haz the power, 2015-09-25 13:04:16]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex clarke (OOO till 29th), 2015-09-25 13:34:24]

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

[commit-bot: I haz the power, 2015-09-25 13:34:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/100001

[commit-bot: I haz the power, 2015-09-25 13:36:20]

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

[commit-bot: I haz the power, 2015-09-25 13:36:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex clarke (OOO till 29th), 2015-09-25 13:37:34]

PTAL

So that assert showed a bug.  If NavigationScheduler::navigateTask fires but the
chromium side OnNavigationStarted never gets called, we end up with the bit
still being set.

That's undesirable because we'd end up preventing the blocking of expensive
tasks until a navigation either succeeded or was cancelled.

Calling OnNavigationStarted multiple times is harmless so I call that from
within NavigationScheduler::navigateTask.

[alex clarke (OOO till 29th), 2015-09-25 13:37:35]

PTAL

So that assert showed a bug.  If NavigationScheduler::navigateTask fires but the
chromium side OnNavigationStarted never gets called, we end up with the bit
still being set.

That's undesirable because we'd end up preventing the blocking of expensive
tasks until a navigation either succeeded or was cancelled.

Calling OnNavigationStarted multiple times is harmless so I call that from
within NavigationScheduler::navigateTask.

[alex clarke (OOO till 29th), 2015-09-25 13:41:34]

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

[commit-bot: I haz the power, 2015-09-25 13:42:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/100002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/100002

[commit-bot: I haz the power, 2015-09-25 14:29:16]

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

[commit-bot: I haz the power, 2015-09-25 14:29:17]

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

[alex clarke (OOO till 29th), 2015-09-25 15:49:08]

So we do need ref counts!  The reason is it's possible for there to be multiple
NavigationSchedulers in existence at the same time for a particular renderer.

PTAL

[alex clarke (OOO till 29th), 2015-09-25 15:49:25]

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

[commit-bot: I haz the power, 2015-09-25 15:50:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/130001

[Sami, 2015-09-25 16:18:38]

Logic looks alright -- just one bikesheddy comment.

https://codereview.chromium.org/1368643002/diff/130001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler.h (right):

https://codereview.chromium.org/1368643002/diff/130001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler.h:120: virtual void
NavigationTaskPendingIncRef() = 0;
nit: I think the "reference" part here is a little misleading since nothing is
really being referenced. How about just
AddPendingNavigation/RemovePendingNavigation?

[alex clarke (OOO till 29th), 2015-09-25 16:57:28]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-09-25 16:57:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1368643002/#ps150001
(title: "Rename for Sami")

[alex clarke (OOO till 29th), 2015-09-25 16:57:38]

https://codereview.chromium.org/1368643002/diff/130001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler.h (right):

https://codereview.chromium.org/1368643002/diff/130001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler.h:120: virtual void
NavigationTaskPendingIncRef() = 0;
On 2015/09/25 16:18:38, Sami wrote:
> nit: I think the "reference" part here is a little misleading since nothing is
> really being referenced. How about just
> AddPendingNavigation/RemovePendingNavigation?

Done.

[commit-bot: I haz the power, 2015-09-25 16:58:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/150001

[Sami, 2015-09-25 17:09:01]

Thanks, lgtm.

[commit-bot: I haz the power, 2015-09-25 18:43:39]

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

[commit-bot: I haz the power, 2015-09-25 18:43:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2015-09-25 19:25:56]

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

[commit-bot: I haz the power, 2015-09-25 19:27:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/150001

[commit-bot: I haz the power, 2015-09-25 20:26:23]

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

[commit-bot: I haz the power, 2015-09-25 20:26:24]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-09-28 09:19:00]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-09-28 09:19:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368643002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368643002/150001

[commit-bot: I haz the power, 2015-09-28 10:27:05]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-09-28 10:27:43]

Patchset 9 (id:??) landed as
https://crrev.com/c403394390922bcaab4318a50a6b405439d7ca7d
Cr-Commit-Position: refs/heads/master@{#351052}