Schedule bitmap animation timers on the compositor task runner.

Schedule bitmap animation timers on the compositor task runner.

GIF animation timers will now run on the compositor task runner (retrievable via TaskRunnerHelper::Get(kAnimationTimer)), which
is now prioritised in UseCase::NONE over the main task runner. This
allows animated GIFs to maintain their frame rate even in the presence
of timer storms like:
https://people-mozilla.org/~bkelly/timer-flood/index.html

BUG=703608
Review-Url: https://codereview.chromium.org/2810423003
Cr-Commit-Position: refs/heads/master@{#472427}
Committed: https://chromium.googlesource.com/chromium/src/+/f5065232010251bad1cccddcc2a0d27ac2232f67

[Dan Elphick, 2017-04-27 14:09:14]

Description was changed from

==========
Schedule bitmap animation timers on the compositor task runner.

GIF animation timers will now run on the compositor task runner, which
is now prioritised in UseCase::NONE over the main task runner. This
allows animated GIFs to maintain their frame rate even in the presence
of timer storms like:
https://people-mozilla.org/~bkelly/timer-flood/index.html

BUG=703608
==========

to

==========
Schedule bitmap animation timers on the compositor task runner.

GIF animation timers will now run on the compositor task runner (retrievable via
TaskRunnerHelper::Get(kAnimationTimer)), which
is now prioritised in UseCase::NONE over the main task runner. This
allows animated GIFs to maintain their frame rate even in the presence
of timer storms like:
https://people-mozilla.org/~bkelly/timer-flood/index.html

BUG=703608
==========

[Dan Elphick, 2017-04-27 14:09:39]

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

[Dan Elphick, 2017-04-27 14:41:39]

WDYT of the general pattern?

There's a couple of const_casts that need removing, which might be tricky.

https://codereview.chromium.org/2810423003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/2810423003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:129:
TaskType::kAnimationTimer, const_cast<Document*>(&document));
Presumably const_cast is bad, so I guess I could remove the const from the
method signature. Although that looks like having a large knock on effect.

https://codereview.chromium.org/2810423003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/2810423003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSImageValue.cpp:77:
TaskType::kAnimationTimer, const_cast<Document*>(&document));
more const_cast here:(

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:73:
BitmapImage::BitmapImage() : BitmapImage(TimerBase::GetTimerTaskRunner()) {}
I guess I could just inline that function here:

Platform::Current()->CurrentThread()->Scheduler()->TimerTaskRunner()

and can then remove the friend BitmapImage in Timer.h.

[Sami, 2017-04-27 17:38:17]

Generally looks great. I'm wondering if we should put a scarier warning on the
animation task queue since it's a bit of a footgun if used incorrectly.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:128:
RefPtr<WebTaskRunner> task_runner = TaskRunnerHelper::Get(
Interesting, can TaskRunnerHelper be changed to take a const instead?

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:127: // Tasks related to
animating images should be posted here.
nit: No need to limit this to image animations -- could just say animations in
general.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:197: } break;
nit: keep the break inside the block.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:85: // TODO: temporary hack
What's this for? :)

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebFrameScheduler.h:55: // WebFrameScheduler
owns the returned WebTaskRunner.
Hmm, this comment about ownership doesn't make sense since the return value is
refcounted :P

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:73:
BitmapImage::BitmapImage() : BitmapImage(TimerBase::GetTimerTaskRunner()) {}
On 2017/04/27 14:41:39, Dan Elphick wrote:
> I guess I could just inline that function here:
> 
> Platform::Current()->CurrentThread()->Scheduler()->TimerTaskRunner()
> 
> and can then remove the friend BitmapImage in Timer.h.

So this is the case where we can't tie the image to a frame/document? I hope
this is very rare, so it doesn't much matter which task runner we use here.
Getting the global one like you said seems fine.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:118:
BitmapImage(RefPtr<WebTaskRunner>, const SkBitmap&, ImageObserver* = 0);
Should we make these two explicit?

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1268:
// Note compositor tasks are not prioritized.
It *should* be fine to prioritize compositor tasks if they're fast, but this
might trigger some surprises especially with high refresh rate mice. Let's try
landing to see what comes up.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1524:
// TODO(delphick): this comment suggests we've got this wrong?
Similarly this test predates the "compositor is fast" check, but let's see what
happens.

[fs, 2017-04-27 17:42:54]

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:73:
BitmapImage::BitmapImage() : BitmapImage(TimerBase::GetTimerTaskRunner()) {}
On 2017/04/27 at 17:38:17, Sami wrote:
> On 2017/04/27 14:41:39, Dan Elphick wrote:
> > I guess I could just inline that function here:
> > 
> > Platform::Current()->CurrentThread()->Scheduler()->TimerTaskRunner()
> > 
> > and can then remove the friend BitmapImage in Timer.h.
> 
> So this is the case where we can't tie the image to a frame/document? I hope
this is very rare, so it doesn't much matter which task runner we use here.
Getting the global one like you said seems fine.

Sorry to say, but Images don't have a 1:1 relation to a frame (or a document for
that matter), so while this might work in many cases (to some degree probably
accidentally), it won't work in general. (See for instance the animation policy
irkiness for an example of this.)

[Sami, 2017-04-27 18:09:31]

On 2017/04/27 17:42:54, fs wrote:
> Sorry to say, but Images don't have a 1:1 relation to a frame (or a document
for
> that matter), so while this might work in many cases (to some degree probably
> accidentally), it won't work in general. (See for instance the animation
policy
> irkiness for an example of this.)

Interesting, do you have more details about this? I wonder if we could somehow
dynamically keep the frame/document association correct? Or are you saying it
just isn't possible in general?

[fs, 2017-04-27 18:23:15]

On 2017/04/27 at 18:09:31, skyostil wrote:
> On 2017/04/27 17:42:54, fs wrote:
> > Sorry to say, but Images don't have a 1:1 relation to a frame (or a document
for
> > that matter), so while this might work in many cases (to some degree
probably
> > accidentally), it won't work in general. (See for instance the animation
policy
> > irkiness for an example of this.)
> 
> Interesting, do you have more details about this? I wonder if we could somehow
dynamically keep the frame/document association correct? Or are you saying it
just isn't possible in general?

I believe the main point is that ImageResources are shared through the
(process-global) MemoryCache, so despite all the plumbing there's no guarantee
that the (Image)Resource you get from a fetch is using the TaskRunner you
plumbed through. You could probably manage to get "a TaskRunner" dynamically
(like the one returned by the first still-active client) - and most of the time
that would probably work too (and potentially with a shorter window of possible
badness due to the TaskRunner.)
A while back I was toying with moving frame-scheduling (==the timer) out of
Image and instead drive that via rAF. It's pretty old and bit-rotted by now
though, and it too had some potential gotchas (although not TaskRunner-like
AFAIK.) Using the registered clients in some form is the best way forward I can
think of ATM.

(When I say "client" I mean ImageResourceObserver, which is implemented by for
instance LayoutObject, and related to Image via the
ImageObserver/ImageResourceContent IIRC after all the renamings/factorings...)

[Sami, 2017-04-27 18:26:19]

On 2017/04/27 18:23:15, fs wrote:
> On 2017/04/27 at 18:09:31, skyostil wrote:
> > On 2017/04/27 17:42:54, fs wrote:
> > > Sorry to say, but Images don't have a 1:1 relation to a frame (or a
document
> for
> > > that matter), so while this might work in many cases (to some degree
> probably
> > > accidentally), it won't work in general. (See for instance the animation
> policy
> > > irkiness for an example of this.)
> > 
> > Interesting, do you have more details about this? I wonder if we could
somehow
> dynamically keep the frame/document association correct? Or are you saying it
> just isn't possible in general?
> 
> I believe the main point is that ImageResources are shared through the
> (process-global) MemoryCache, so despite all the plumbing there's no guarantee
> that the (Image)Resource you get from a fetch is using the TaskRunner you
> plumbed through. You could probably manage to get "a TaskRunner" dynamically
> (like the one returned by the first still-active client) - and most of the
time
> that would probably work too (and potentially with a shorter window of
possible
> badness due to the TaskRunner.)

I see. Looks like we're going to have to try something else -- or just use a
global (non-frame-specific) compositing queue for all image decodes. That would
still be an improvement scheduling-wise.

> A while back I was toying with moving frame-scheduling (==the timer) out of
> Image and instead drive that via rAF. It's pretty old and bit-rotted by now
> though, and it too had some potential gotchas (although not TaskRunner-like
> AFAIK.) Using the registered clients in some form is the best way forward I
can
> think of ATM.
> 
> (When I say "client" I mean ImageResourceObserver, which is implemented by for
> instance LayoutObject, and related to Image via the
> ImageObserver/ImageResourceContent IIRC after all the renamings/factorings...)

Driving animations by rAF would definitely be ideal! If you have that patch
lying around somewhere we might be interested in taking a peek.

[Dan Elphick, 2017-05-03 09:41:06]

PTAL.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:128:
RefPtr<WebTaskRunner> task_runner = TaskRunnerHelper::Get(
On 2017/04/27 17:38:17, Sami wrote:
> Interesting, can TaskRunnerHelper be changed to take a const instead?

not required now we're not using getting the scheduler via the frame

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:127: // Tasks related to
animating images should be posted here.
On 2017/04/27 17:38:17, Sami wrote:
> nit: No need to limit this to image animations -- could just say animations in
> general.

no longer required

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:197: } break;
On 2017/04/27 17:38:17, Sami wrote:
> nit: keep the break inside the block.

Acknowledged.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:85: // TODO: temporary hack
On 2017/04/27 17:38:17, Sami wrote:
> What's this for? :)

no longer required

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebFrameScheduler.h:55: // WebFrameScheduler
owns the returned WebTaskRunner.
On 2017/04/27 17:38:17, Sami wrote:
> Hmm, this comment about ownership doesn't make sense since the return value is
> refcounted :P

no longer required

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:73:
BitmapImage::BitmapImage() : BitmapImage(TimerBase::GetTimerTaskRunner()) {}
On 2017/04/27 17:42:54, fs wrote:
> On 2017/04/27 at 17:38:17, Sami wrote:
> > On 2017/04/27 14:41:39, Dan Elphick wrote:
> > > I guess I could just inline that function here:
> > > 
> > > Platform::Current()->CurrentThread()->Scheduler()->TimerTaskRunner()
> > > 
> > > and can then remove the friend BitmapImage in Timer.h.
> > 
> > So this is the case where we can't tie the image to a frame/document? I hope
> this is very rare, so it doesn't much matter which task runner we use here.
> Getting the global one like you said seems fine.
> 
> Sorry to say, but Images don't have a 1:1 relation to a frame (or a document
for
> that matter), so while this might work in many cases (to some degree probably
> accidentally), it won't work in general. (See for instance the animation
policy
> irkiness for an example of this.)

I've reworked it all now to get the Compositor task runner with
Platform::Current()->CurrentThread()->Scheduler()->CompositorTaskRunner()
explicitly acknowledging that there is only one Compositor task runner per
process.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:118:
BitmapImage(RefPtr<WebTaskRunner>, const SkBitmap&, ImageObserver* = 0);
On 2017/04/27 17:38:17, Sami wrote:
> Should we make these two explicit?

no longer required

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1268:
// Note compositor tasks are not prioritized.
On 2017/04/27 17:38:17, Sami wrote:
> It *should* be fine to prioritize compositor tasks if they're fast, but this
> might trigger some surprises especially with high refresh rate mice. Let's try
> landing to see what comes up.

Done.

https://codereview.chromium.org/2810423003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1524:
// TODO(delphick): this comment suggests we've got this wrong?
On 2017/04/27 17:38:17, Sami wrote:
> Similarly this test predates the "compositor is fast" check, but let's see
what
> happens.

Done.

[Dan Elphick, 2017-05-03 13:58:32]

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

[commit-bot: I haz the power, 2017-05-03 13:58:44]

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-05-03 15:02:23]

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

[commit-bot: I haz the power, 2017-05-03 15:02:27]

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

[Sami, 2017-05-04 00:33:38]

Great, looks much simpler now!

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h (right):

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h:75: //
Returns a WebTaskRunner for compositor tasks.
Could add a bit more scary explanation here how this is only meant for specific
animation and rendering related tasks and should not be used generally?

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1267:
// Note compositor tasks are not prioritized.
This test is a little useless now, so let's change it to simulate a bunch of
slow compositor tasks so that it verifies the previous behavior. We can do the
same for the other two below that also say "compositor tasks are not
prioritized"

[Dan Elphick, 2017-05-04 16:47:15]

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h (right):

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h:75: //
Returns a WebTaskRunner for compositor tasks.
On 2017/05/04 00:33:38, Sami wrote:
> Could add a bit more scary explanation here how this is only meant for
specific
> animation and rendering related tasks and should not be used generally?

Done.

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2810423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1267:
// Note compositor tasks are not prioritized.
On 2017/05/04 00:33:38, Sami wrote:
> This test is a little useless now, so let's change it to simulate a bunch of
> slow compositor tasks so that it verifies the previous behavior. We can do the
> same for the other two below that also say "compositor tasks are not
> prioritized"

Added a new RunSlowCompositorTask method to RendererSchedulerImplTest which this
and similar methods now call to force into the mode where compositor tasks are
not prioritized.

TestDefaultPolicy doesn't use it which contrasts with
TestDefaultPolicyWithSlowCompositor to show the difference in behaviours.

[Dan Elphick, 2017-05-05 08:28:22]

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

[commit-bot: I haz the power, 2017-05-05 08:28:34]

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-05-05 09:24:33]

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

[commit-bot: I haz the power, 2017-05-05 09:24:34]

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

[Dan Elphick, 2017-05-10 09:04:15]

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

[commit-bot: I haz the power, 2017-05-10 09:05:09]

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-05-10 10:33:13]

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

[commit-bot: I haz the power, 2017-05-10 10:33:14]

Dry run: 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_...)

[Dan Elphick, 2017-05-10 10:38:37]

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

[commit-bot: I haz the power, 2017-05-10 10:39:00]

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-05-10 12:20:44]

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

[commit-bot: I haz the power, 2017-05-10 12:20:45]

Dry run: 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_...)

[Dan Elphick, 2017-05-11 09:27:18]

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

[commit-bot: I haz the power, 2017-05-11 09:27:55]

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-05-11 13:01:18]

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

[commit-bot: I haz the power, 2017-05-11 13:01:19]

Dry run: This issue passed the CQ dry run.

[Dan Elphick, 2017-05-11 13:53:46]

PTAL

[Sami, 2017-05-16 13:51:46]

lgtm, thanks for the optimization!

https://codereview.chromium.org/2810423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h (right):

https://codereview.chromium.org/2810423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h:76: // used
for animation and rendering related tasks (e.g. animated GIFS) and
s/for/by specific/

[Dan Elphick, 2017-05-16 14:09:52]

https://codereview.chromium.org/2810423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h (right):

https://codereview.chromium.org/2810423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/web_scheduler.h:76: // used
for animation and rendering related tasks (e.g. animated GIFS) and
On 2017/05/16 13:51:45, Sami wrote:
> s/for/by specific/

Done.

[Dan Elphick, 2017-05-16 14:16:16]

delphick@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Dan Elphick, 2017-05-16 14:16:42]

Added Chris for BitmapImage changes.

[chrishtr, 2017-05-17 00:37:40]

lgtm

[Dan Elphick, 2017-05-17 08:29:12]

The CQ bit was checked by delphick@chromium.org

[Dan Elphick, 2017-05-17 08:29:12]

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/2810423003/#ps180001
(title: "Fix comment.")

[commit-bot: I haz the power, 2017-05-17 08:30:01]

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-05-17 12:01:03]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495009752551960,
"parent_rev": "659885eebc27bf7a49b806d2f8b104b547b36605", "commit_rev":
"f5065232010251bad1cccddcc2a0d27ac2232f67"}

[commit-bot: I haz the power, 2017-05-17 12:01:20]

Description was changed from

==========
Schedule bitmap animation timers on the compositor task runner.

GIF animation timers will now run on the compositor task runner (retrievable via
TaskRunnerHelper::Get(kAnimationTimer)), which
is now prioritised in UseCase::NONE over the main task runner. This
allows animated GIFs to maintain their frame rate even in the presence
of timer storms like:
https://people-mozilla.org/~bkelly/timer-flood/index.html

BUG=703608
==========

to

==========
Schedule bitmap animation timers on the compositor task runner.

GIF animation timers will now run on the compositor task runner (retrievable via
TaskRunnerHelper::Get(kAnimationTimer)), which
is now prioritised in UseCase::NONE over the main task runner. This
allows animated GIFs to maintain their frame rate even in the presence
of timer storms like:
https://people-mozilla.org/~bkelly/timer-flood/index.html

BUG=703608

Review-Url: https://codereview.chromium.org/2810423003
Cr-Commit-Position: refs/heads/master@{#472427}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5065232010251bad1cccddcc2a0...
==========

[commit-bot: I haz the power, 2017-05-17 12:01:23]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/f5065232010251bad1cccddcc2a0...

[jbroman, 2017-05-18 19:07:38]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2892053002/ by jbroman@chromium.org.

The reason for reverting is: Makes CrSettingsRouteDynamicParametersTest.All fail
most of the time (requiring retries). Exact reason isn't clear, but local bisect
on Linux consistently points to this CL.

See also:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType....