Don't throttle web views until they've been in the background for 10s

Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied immediately 
and budget based throttling after 10s.  It looks like even the timer alignment throttling
on it's own is causing problems for new tabs where the visibility isn't initially known.
To fix this we're changing the policy so we don't do any throttling at all until the web
view has been in the background for 10s and then we turn budget based throttling &
alignment on.

Note this patch removes fast/dom/timer-throttling-hidden-page.html
because the 10s grace period means this test would be too slow. 

Virtual time doesn't help because Date.now() (currently) isn't
overridden. There's no support for using a mock time source in 
layout tests.  In any event there's plenty of C++ unit test coverage
for this so it's not too bad to remove the layout test.

BUG=649942
Review-Url: https://codereview.chromium.org/2620743002
Cr-Commit-Position: refs/heads/master@{#442989}
Committed: https://chromium.googlesource.com/chromium/src/+/368911e4215596d9e75fe64f54c9accd14b27ae5

[alex clarke (OOO till 29th), 2017-01-09 17:52:21]

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

[commit-bot: I haz the power, 2017-01-09 17:52: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), 2017-01-09 17:52:46]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediatly and budget based throttling after 10s.  It looks like even
the timer alignment throttling is causing problems for new tabs where
the visibility isn't initially known.  To fix this we're changing the
policy so we don't do any throttling at all until the web view has
been in the background for 10s and then we turn budget based
throlling & alignment on.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediatly and budget based throttling after 10s.  It looks like even
the timer alignment throttling is causing problems for new tabs where
the visibility isn't initially known.  To fix this we're changing the
policy so we don't do any throttling at all until the web view has
been in the background for 10s and then we turn budget based
throlling & alignment on.

BUG=649942
==========

[alex clarke (OOO till 29th), 2017-01-09 17:52:46]

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

[alex clarke (OOO till 29th), 2017-01-09 17:53:52]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediatly and budget based throttling after 10s.  It looks like even
the timer alignment throttling is causing problems for new tabs where
the visibility isn't initially known.  To fix this we're changing the
policy so we don't do any throttling at all until the web view has
been in the background for 10s and then we turn budget based
throlling & alignment on.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling on it's own
is causing problems for new tabs where the visibility isn't initially known.  To
fix this we're changing 
the policy so we don't do any throttling at all until the web view has
been in the 
background for 10s and then we turn budget based throttling & alignment on.

BUG=649942
==========

[alex clarke (OOO till 29th), 2017-01-09 17:54:39]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling on it's own
is causing problems for new tabs where the visibility isn't initially known.  To
fix this we're changing 
the policy so we don't do any throttling at all until the web view has
been in the 
background for 10s and then we turn budget based throttling & alignment on.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately and budget based throttling after 10s.  It looks like even the timer
alignment throttling on it's own is causing problems for new tabs where the
visibility isn't initially known.  To fix this we're changing the policy so we
don't do any throttling at all until the web view has been in the background for
10s and then we turn budget based throttling & alignment on.

BUG=649942
==========

[alex clarke (OOO till 29th), 2017-01-09 17:54:54]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately and budget based throttling after 10s.  It looks like even the timer
alignment throttling on it's own is causing problems for new tabs where the
visibility isn't initially known.  To fix this we're changing the policy so we
don't do any throttling at all until the web view has been in the background for
10s and then we turn budget based throttling & alignment on.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately and budget based throttling after 10s.  It looks like even the timer
alignment throttling on it's own is causing problems for new tabs where the
visibility isn't initially known.  To fix this we're changing the policy so we
don't do any throttling at all until the web view has been in the background for
10s and then we turn budget based throttling & alignment on.

BUG=649942
==========

[alex clarke (OOO till 29th), 2017-01-09 17:55:18]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately and budget based throttling after 10s.  It looks like even the timer
alignment throttling on it's own is causing problems for new tabs where the
visibility isn't initially known.  To fix this we're changing the policy so we
don't do any throttling at all until the web view has been in the background for
10s and then we turn budget based throttling & alignment on.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling
on it's own is causing problems for new tabs where the visibility isn't
initially known.
To fix this we're changing the policy so we don't do any throttling at all until
the web
view has been in the background for 10s and then we turn budget based throttling
&
alignment on.

BUG=649942
==========

[commit-bot: I haz the power, 2017-01-09 19:53:02]

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

[commit-bot: I haz the power, 2017-01-09 19:53:03]

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

[alex clarke (OOO till 29th), 2017-01-10 12:13:25]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling
on it's own is causing problems for new tabs where the visibility isn't
initially known.
To fix this we're changing the policy so we don't do any throttling at all until
the web
view has been in the background for 10s and then we turn budget based throttling
&
alignment on.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling
on it's own is causing problems for new tabs where the visibility isn't
initially known.
To fix this we're changing the policy so we don't do any throttling at all until
the web
view has been in the background for 10s and then we turn budget based throttling
&
alignment on.

Note this patch removes fast/dom/timer-throttling-hidden-page.html
because the 10s grace period means this test would be too slow. 

Virtual time doesn't help because Date.now() (currently) isn't
overridden. There's no support for using a mock time source in 
layout tests.  In any event there's plenty of C++ unit test coverage
for this so it's not too bad to remove the layout test.

BUG=649942
==========

[alex clarke (OOO till 29th), 2017-01-10 12:21:40]

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

[commit-bot: I haz the power, 2017-01-10 12:21:59]

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-01-10 13:38:41]

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

[commit-bot: I haz the power, 2017-01-10 13:38:43]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-01-10 14:44:22]

Thanks, the fix looks good. One question below.

Were you able to repro the bug directly by the way?

https://codereview.chromium.org/2620743002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2620743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:334:
frame_scheduler->setPageVisible(false);
Should we rename |setPageVisible| to |setThrottlingAllowed| or something like
that since it won't strictly be tracking visibility anymore?

[alex clarke (OOO till 29th), 2017-01-11 10:08:49]

On 2017/01/10 14:44:22, Sami wrote:
> Thanks, the fix looks good. One question below.
> 
> Were you able to repro the bug directly by the way?
I think so yes. I should be able to demonstrate this - sometimes the Feedly
Notifier takes a good few seconds to load with a ToT build. It seems
consistently fast with this patched in.  Might be interesting to get a couple of
traces and verify we're seeing what we think we're seeing.

> 
>
https://codereview.chromium.org/2620743002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2620743002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:334:
> frame_scheduler->setPageVisible(false);
> Should we rename |setPageVisible| to |setThrottlingAllowed| or something like
> that since it won't strictly be tracking visibility anymore?

[alex clarke (OOO till 29th), 2017-01-11 10:54:47]

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

[alex clarke (OOO till 29th), 2017-01-11 10:54:47]

+haraken for third_party/WebKit/Source/platform/WebFrameScheduler.h

https://codereview.chromium.org/2620743002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2620743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:334:
frame_scheduler->setPageVisible(false);
On 2017/01/10 14:44:22, Sami wrote:
> Should we rename |setPageVisible| to |setThrottlingAllowed| or something like
> that since it won't strictly be tracking visibility anymore?
You're probably right.  After this patch the WebFrameSchedulerImpl's knowledge
of visibility has a 10s delay.  Lets call it setPageThrottled()

[alex clarke (OOO till 29th), 2017-01-11 10:54:55]

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

[commit-bot: I haz the power, 2017-01-11 10:55:12]

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-01-11 11:16:47]

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

[commit-bot: I haz the power, 2017-01-11 11:16:48]

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_...)

[haraken, 2017-01-11 11:20:52]

This change sounds very reasonable.

On 2017/01/11 10:54:47, alex clarke wrote:
> +haraken for third_party/WebKit/Source/platform/WebFrameScheduler.h

platform/WebFrameScheduler.h LGTM.

[Sami, 2017-01-11 11:55:40]

lgtm, thanks!

[alex clarke (OOO till 29th), 2017-01-11 12:05:43]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-01-11 12:05:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2620743002/#ps60001
(title: "Fix test compile")

[commit-bot: I haz the power, 2017-01-11 12:05:58]

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-01-11 12:31:27]

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

[commit-bot: I haz the power, 2017-01-11 12:31:28]

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

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

The CQ bit was checked by alexclarke@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2620743002/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2017-01-11 13:16:08]

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-01-11 14:40:28]

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

[commit-bot: I haz the power, 2017-01-11 14:40:30]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[alex clarke (OOO till 29th), 2017-01-11 15:33:32]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-01-11 15:33:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2620743002/#ps100001
(title: "Speculative asan fix")

[commit-bot: I haz the power, 2017-01-11 15:33:54]

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-01-11 15:51:29]

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

[commit-bot: I haz the power, 2017-01-11 15:51:31]

Try jobs failed on following builders:
  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-01-11 17:07:57]

Patchset #6 (id:100001) has been deleted

[alex clarke (OOO till 29th), 2017-01-11 17:08:01]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-01-11 17:08:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2620743002/#ps120001
(title: "Fix asan test issue")

[commit-bot: I haz the power, 2017-01-11 17:08:22]

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-01-11 20:45:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484154481349380,
"parent_rev": "b61b264655a67b06dd3937ba02e7b487cc35aacd", "commit_rev":
"368911e4215596d9e75fe64f54c9accd14b27ae5"}

[commit-bot: I haz the power, 2017-01-11 20:45:43]

Description was changed from

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling
on it's own is causing problems for new tabs where the visibility isn't
initially known.
To fix this we're changing the policy so we don't do any throttling at all until
the web
view has been in the background for 10s and then we turn budget based throttling
&
alignment on.

Note this patch removes fast/dom/timer-throttling-hidden-page.html
because the 10s grace period means this test would be too slow. 

Virtual time doesn't help because Date.now() (currently) isn't
overridden. There's no support for using a mock time source in 
layout tests.  In any event there's plenty of C++ unit test coverage
for this so it's not too bad to remove the layout test.

BUG=649942
==========

to

==========
Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied
immediately 
and budget based throttling after 10s.  It looks like even the timer alignment
throttling
on it's own is causing problems for new tabs where the visibility isn't
initially known.
To fix this we're changing the policy so we don't do any throttling at all until
the web
view has been in the background for 10s and then we turn budget based throttling
&
alignment on.

Note this patch removes fast/dom/timer-throttling-hidden-page.html
because the 10s grace period means this test would be too slow. 

Virtual time doesn't help because Date.now() (currently) isn't
overridden. There's no support for using a mock time source in 
layout tests.  In any event there's plenty of C++ unit test coverage
for this so it's not too bad to remove the layout test.

BUG=649942

Review-Url: https://codereview.chromium.org/2620743002
Cr-Commit-Position: refs/heads/master@{#442989}
Committed:
https://chromium.googlesource.com/chromium/src/+/368911e4215596d9e75fe64f54c9...
==========

[commit-bot: I haz the power, 2017-01-11 20:45:45]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/368911e4215596d9e75fe64f54c9...

[altimin, 2017-01-12 11:11:13]

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

[altimin, 2017-01-12 11:11:14]

lgtm