Don't restart the ImageQualityController timer unless it is already half over.

Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

We do this plumbing to avoid making lots of calls to query the system time,
which is expensive. The compositor has a similar optimization; see
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_layer_impl.cc&sq=package:chromium&l=452
for example.

BUG=605701
Committed: https://crrev.com/f79f4d6a1c52efbe06e30ace1ce882ac1d53aa40
Cr-Commit-Position: refs/heads/master@{#389394}

[chrishtr, 2016-04-22 15:41:46]

Description was changed from

==========
none


none


none

BUG=
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

BUG=605701
==========

[chrishtr, 2016-04-22 17:48:38]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug.

Instead, only restart the timer if it is more than half

BUG=605701
==========

[chrishtr, 2016-04-22 17:50:08]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug.

Instead, only restart the timer if it is more than half

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug.

Instead, only restart the timer if the current frame begin time is more than
half of the timer time later than the frame time at the start of the timer. To
accomplish
that, record the current frame time in WebViewImpl, so that it can be compared
with
a future frame time. We do all this plumbing to avoid making lots of calls to
query the system time, which is expensive.

BUG=605701
==========

[chrishtr, 2016-04-22 17:51:03]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug.

Instead, only restart the timer if the current frame begin time is more than
half of the timer time later than the frame time at the start of the timer. To
accomplish
that, record the current frame time in WebViewImpl, so that it can be compared
with
a future frame time. We do all this plumbing to avoid making lots of calls to
query the system time, which is expensive.

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time. We do all this plumbing to avoid
making
lots of calls to query the system time, which is expensive.

BUG=605701
==========

[chrishtr, 2016-04-22 17:51:56]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time. We do all this plumbing to avoid
making
lots of calls to query the system time, which is expensive.

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time. We do all this plumbing to avoid
making
lots of calls to query the system time, which is expensive.

BUG=605701
==========

[chrishtr, 2016-04-22 17:52:28]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time. We do all this plumbing to avoid
making
lots of calls to query the system time, which is expensive.

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time. (We do this plumbing to avoid making
lots of calls to query the system time, which is expensive.)

BUG=605701
==========

[chrishtr, 2016-04-22 17:53:00]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time. (We do this plumbing to avoid making
lots of calls to query the system time, which is expensive.)

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

BUG=605701
==========

[chrishtr, 2016-04-22 17:53:16]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

We do this plumbing to avoid making lots of calls to query the system time,
which is expensive. The compositor has a similar optimization; see
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...

BUG=605701
==========

[chrishtr, 2016-04-22 17:53:31]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

We do this plumbing to avoid making lots of calls to query the system time,
which is expensive. The compositor has a similar optimization; see
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

We do this plumbing to avoid making lots of calls to query the system time,
which is expensive. The compositor has a similar optimization; see
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
for example.

BUG=605701
==========

[chrishtr, 2016-04-22 18:01:00]

chrishtr@chromium.org changed reviewers:
+ esprehn@chromium.org, wangxianzhu@chromium.org

[Xianzhu, 2016-04-22 18:15:00]

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp
(right):

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp:125: void
startOneShot(double interval, const WebTraceLocation& caller) override
Can you use Timer::isActive() and Timer::cancel() instead of making this method
virtual?

[chrishtr, 2016-04-22 18:30:35]

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp
(right):

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp:125: void
startOneShot(double interval, const WebTraceLocation& caller) override
On 2016/04/22 at 18:14:59, Xianzhu wrote:
> Can you use Timer::isActive() and Timer::cancel() instead of making this
method virtual?

I'm not sure how to do that. Are you suggesting cancelling the timer in the test
and checking whether it was re-started? That won't work because
the code checks for it being inactive before the other logic.

I think I could use nextFireInterval() and mock out time...

[Xianzhu, 2016-04-22 19:39:17]

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp
(right):

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp:115: ,
started(false)
Nit: Indent 4 more spaces.

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp:125: void
startOneShot(double interval, const WebTraceLocation& caller) override
On 2016/04/22 18:30:35, chrishtr wrote:
> On 2016/04/22 at 18:14:59, Xianzhu wrote:
> > Can you use Timer::isActive() and Timer::cancel() instead of making this
> method virtual?
> 
> I'm not sure how to do that. Are you suggesting cancelling the timer in the
test
> and checking whether it was re-started? That won't work because
> the code checks for it being inactive before the other logic.
> 
> I think I could use nextFireInterval() and mock out time...

I see Timer::cancel() would affect what we want to test.

Would this work:
- Add double Timer::nextFireTime() const { return m_nextFireTime; }
- Check if nextFireTime() changed after calling shouldPaintAtLowQuality().

[Xianzhu, 2016-04-22 19:49:51]

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp
(right):

https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp:125: void
startOneShot(double interval, const WebTraceLocation& caller) override
On 2016/04/22 19:39:17, Xianzhu wrote:
> On 2016/04/22 18:30:35, chrishtr wrote:
> > On 2016/04/22 at 18:14:59, Xianzhu wrote:
> > > Can you use Timer::isActive() and Timer::cancel() instead of making this
> > method virtual?
> > 
> > I'm not sure how to do that. Are you suggesting cancelling the timer in the
> test
> > and checking whether it was re-started? That won't work because
> > the code checks for it being inactive before the other logic.
> > 
> > I think I could use nextFireInterval() and mock out time...
> 
> I see Timer::cancel() would affect what we want to test.
> 
> Would this work:
> - Add double Timer::nextFireTime() const { return m_nextFireTime; }
> - Check if nextFireTime() changed after calling shouldPaintAtLowQuality().

Just noticed that you would have to mock the time. I think this is worth doing
to avoid changing Timer::startOneShot().

[chrishtr, 2016-04-22 20:28:51]

On 2016/04/22 at 19:49:51, wangxianzhu wrote:
>
https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp
(right):
> 
>
https://codereview.chromium.org/1911273002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/ImageQualityControllerTest.cpp:125: void
startOneShot(double interval, const WebTraceLocation& caller) override
> On 2016/04/22 19:39:17, Xianzhu wrote:
> > On 2016/04/22 18:30:35, chrishtr wrote:
> > > On 2016/04/22 at 18:14:59, Xianzhu wrote:
> > > > Can you use Timer::isActive() and Timer::cancel() instead of making this
> > > method virtual?
> > > 
> > > I'm not sure how to do that. Are you suggesting cancelling the timer in
the
> > test
> > > and checking whether it was re-started? That won't work because
> > > the code checks for it being inactive before the other logic.
> > > 
> > > I think I could use nextFireInterval() and mock out time...
> > 
> > I see Timer::cancel() would affect what we want to test.
> > 
> > Would this work:
> > - Add double Timer::nextFireTime() const { return m_nextFireTime; }
> > - Check if nextFireTime() changed after calling shouldPaintAtLowQuality().
> 
> Just noticed that you would have to mock the time. I think this is worth doing
to avoid changing Timer::startOneShot().

Done.

[Xianzhu, 2016-04-22 20:38:29]

lgtm

[chrishtr, 2016-04-22 20:44:54]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2016-04-22 20:45:28]

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

[commit-bot: I haz the power, 2016-04-22 22:13:03]

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

[commit-bot: I haz the power, 2016-04-22 22:13:04]

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

[chrishtr, 2016-04-22 22:37:03]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-22 22:37:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1911273002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-04-22 22:37:50]

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

[commit-bot: I haz the power, 2016-04-23 00:14:23]

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

[commit-bot: I haz the power, 2016-04-23 00:14:24]

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

[chrishtr, 2016-04-23 04:13:13]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-23 04:13:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1911273002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-04-23 04:13:27]

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

[commit-bot: I haz the power, 2016-04-23 05:50:31]

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

[commit-bot: I haz the power, 2016-04-23 05:50:33]

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

[chrishtr, 2016-04-23 22:51:35]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-23 22:51:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1911273002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2016-04-23 22:51:49]

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

[commit-bot: I haz the power, 2016-04-24 00:02:44]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-24 00:04:27]

Description was changed from

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

We do this plumbing to avoid making lots of calls to query the system time,
which is expensive. The compositor has a similar optimization; see
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
for example.

BUG=605701
==========

to

==========
Don't restart the ImageQualityController timer unless it is already half over.

Restarting the timer every time an image continues to animate is very expensive,
because there is no way to actuually cancel the existing timer. Instead, the
timer
is neutered, so that when it fires it will do nothing. This results in a flood
of no-op timer callbacks in the referenced bug scenario.

Instead, only restart the timer if the current frame time is more than half
of the timer time later than the frame time at the start of the timer.
To accomplish that, record the current frame time in WebViewImpl, so that it
can be compared with a future frame time.

We do this plumbing to avoid making lots of calls to query the system time,
which is expensive. The compositor has a similar optimization; see
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
for example.

BUG=605701

Committed: https://crrev.com/f79f4d6a1c52efbe06e30ace1ce882ac1d53aa40
Cr-Commit-Position: refs/heads/master@{#389394}
==========

[commit-bot: I haz the power, 2016-04-24 00:04:28]

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