Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it and
Virtual Time is applied to all relevant queues.

Finally remove unused WebViewSchedulerTest.cpp

BUG=546953
Committed: https://crrev.com/1c815652bc24a83da99ca473d489accdeeafa435
Cr-Commit-Position: refs/heads/master@{#408944}

[alex clarke (OOO till 29th), 2016-07-27 12:08:32]

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

[commit-bot: I haz the power, 2016-07-27 12:08:42]

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-07-27 12:08:49]

Description was changed from

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitarially far before the
first fetch occurred.

Also apply virtual time to per thread task queues.

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

to

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitarially far before the
first fetch occurred.

Also apply virtual time to per thread task queues.

Finally remove unused WebViewSchedulerTest.cpp

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

[alex clarke (OOO till 29th), 2016-07-27 12:09:04]

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

[commit-bot: I haz the power, 2016-07-27 13:30:15]

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

[commit-bot: I haz the power, 2016-07-27 13:30:15]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-07-28 10:46:34]

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

https://codereview.chromium.org/2184123002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:1412: void
RendererSchedulerImpl::SetPerThreadTaskRunnerTimeDomain(
The next policy update can overwrite these settings. Should we instead make
virtual time be more of a global mode for the entire scheduler so it knows how
to set the appropriate time domains?

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

https://codereview.chromium.org/2184123002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:149: //
TimerTaskRunner).  If the lifetime of |time_domain| is shorter than the
nit: How about (less cryptically :) "|time_domain| must outlive the
RendererSchedulerImpl" (implying that it needs to be unregistered if that's not
the case)

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

https://codereview.chromium.org/2184123002/diff/1/components/scheduler/render...
components/scheduler/renderer/web_view_scheduler_impl_unittest.cc:509: //
Initially virtual time is not allowed to advance until we have seen at
This comment could be useful in the virtual time API header too.

[jochen (gone - plz use gerrit), 2016-07-29 09:42:16]

waiting for skyostil to finish his review

[alex clarke (OOO till 29th), 2016-07-29 13:35:02]

PTAL

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

https://codereview.chromium.org/2184123002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:1412: void
RendererSchedulerImpl::SetPerThreadTaskRunnerTimeDomain(
On 2016/07/28 10:46:34, Sami wrote:
> The next policy update can overwrite these settings. Should we instead make
> virtual time be more of a global mode for the entire scheduler so it knows how
> to set the appropriate time domains?

Done.

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

https://codereview.chromium.org/2184123002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:149: //
TimerTaskRunner).  If the lifetime of |time_domain| is shorter than the
On 2016/07/28 10:46:34, Sami wrote:
> nit: How about (less cryptically :) "|time_domain| must outlive the
> RendererSchedulerImpl" (implying that it needs to be unregistered if that's
not
> the case)

Done.

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

https://codereview.chromium.org/2184123002/diff/1/components/scheduler/render...
components/scheduler/renderer/web_view_scheduler_impl_unittest.cc:509: //
Initially virtual time is not allowed to advance until we have seen at
On 2016/07/28 10:46:34, Sami wrote:
> This comment could be useful in the virtual time API header too.

Done.

[alex clarke (OOO till 29th), 2016-07-29 13:35:12]

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

[commit-bot: I haz the power, 2016-07-29 13:35:27]

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-07-29 13:36:16]

Description was changed from

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitarially far before the
first fetch occurred.

Also apply virtual time to per thread task queues.

Finally remove unused WebViewSchedulerTest.cpp

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

to

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it.

Also apply virtual time to per thread task queues.

Finally remove unused WebViewSchedulerTest.cpp

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

[alex clarke (OOO till 29th), 2016-07-29 13:36:49]

Description was changed from

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it.

Also apply virtual time to per thread task queues.

Finally remove unused WebViewSchedulerTest.cpp

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

to

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it and
Virtual Time is applied to all relevant queues.

Finally remove unused WebViewSchedulerTest.cpp

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

[Sami, 2016-07-29 14:05:51]

Otherwise looks good, just one question about timer suspension.

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

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:233:
.SetTimeDomain(MainThreadOnly().use_virtual_time
Could you add a TODO here to make these use ApplyTaskQueuePolicy() so we don't
need keep repeating everything?

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:895:
new_policy.timer_queue_policy.time_domain_type = TimeDomainType::REAL;
Is it okay to go back to real time here?

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:1440:
AutoAdvancingVirtualTimeDomain* RendererSchedulerImpl::virtual_time_domain() {
nit: This should be called VirtualTimeDomain() since does a bit more than
returning a member.

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl.cc (right):

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:150: // otherwise we
could advance virtual time arbitarially far before the
typo: arbitrarily

https://codereview.chromium.org/2184123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp (right):

https://codereview.chromium.org/2184123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp:51: // Some task queues
may have repeating tasks that run forever so we impose a hard time limit.
"repeating v8 tasks" to make it clear it's not the test itself generating these
tasks?

[alex clarke (OOO till 29th), 2016-07-29 14:42:09]

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

[alex clarke (OOO till 29th), 2016-07-29 14:42:16]

PTAL

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

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:233:
.SetTimeDomain(MainThreadOnly().use_virtual_time
On 2016/07/29 14:05:50, Sami wrote:
> Could you add a TODO here to make these use ApplyTaskQueuePolicy() so we don't
> need keep repeating everything?

Done.

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:895:
new_policy.timer_queue_policy.time_domain_type = TimeDomainType::REAL;
On 2016/07/29 14:05:50, Sami wrote:
> Is it okay to go back to real time here?

Huh, no.  Lets fix that.

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:1440:
AutoAdvancingVirtualTimeDomain* RendererSchedulerImpl::virtual_time_domain() {
On 2016/07/29 14:05:50, Sami wrote:
> nit: This should be called VirtualTimeDomain() since does a bit more than
> returning a member.

Done.

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl.cc (right):

https://codereview.chromium.org/2184123002/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:150: // otherwise we
could advance virtual time arbitarially far before the
On 2016/07/29 14:05:51, Sami wrote:
> typo: arbitrarily

Done.

https://codereview.chromium.org/2184123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp (right):

https://codereview.chromium.org/2184123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp:51: // Some task queues
may have repeating tasks that run forever so we impose a hard time limit.
On 2016/07/29 14:05:51, Sami wrote:
> "repeating v8 tasks" to make it clear it's not the test itself generating
these
> tasks?

Done.

[alex clarke (OOO till 29th), 2016-07-29 14:42:17]

PTAL

[commit-bot: I haz the power, 2016-07-29 14:42:26]

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

[Sami, 2016-07-29 14:48:40]

Thanks, lgtm.

[commit-bot: I haz the power, 2016-07-29 16:46:16]

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

[commit-bot: I haz the power, 2016-07-29 16:46:19]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-07-30 13:34:10]

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

[commit-bot: I haz the power, 2016-07-30 13:34:19]

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, 2016-07-30 15:38:39]

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

[commit-bot: I haz the power, 2016-07-30 15:38:40]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-08-01 10:39:26]

lgtm

[alex clarke (OOO till 29th), 2016-08-01 10:39:50]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-08-01 10:39:51]

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/2184123002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-01 10:39:59]

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-01 12:20:48]

Description was changed from

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it and
Virtual Time is applied to all relevant queues.

Finally remove unused WebViewSchedulerTest.cpp

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

to

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it and
Virtual Time is applied to all relevant queues.

Finally remove unused WebViewSchedulerTest.cpp

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

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

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-01 12:23:41]

Description was changed from

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it and
Virtual Time is applied to all relevant queues.

Finally remove unused WebViewSchedulerTest.cpp

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

to

==========
Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING

Change VirtualTimePolicy::PAUSE_IF_NETWORK_FETCHES_PENDING to initially
pause virtual time until at least one loading task has run.  This fixes
a problem where virtual time could advance arbitrarily far before the
first fetch occurred.

Also refactor the way virtual time is dealt with so that
RendererSchedulerImpl and ThrottlingHelper know about it and
Virtual Time is applied to all relevant queues.

Finally remove unused WebViewSchedulerTest.cpp

BUG=546953

Committed: https://crrev.com/1c815652bc24a83da99ca473d489accdeeafa435
Cr-Commit-Position: refs/heads/master@{#408944}
==========

[commit-bot: I haz the power, 2016-08-01 12:23:42]

Patchset 6 (id:??) landed as
https://crrev.com/1c815652bc24a83da99ca473d489accdeeafa435
Cr-Commit-Position: refs/heads/master@{#408944}

[kolos1, 2016-08-12 12:51:29]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2246493002/ by kolos@chromium.org.

The reason for reverting is: This CL caused failures of WebKit Android (Nexus4).


See crbug.com/633321 for details. .