Don't animate a progress bar if it isn't animated

Don't animate a progress bar if it isn't animated

The paint path for HTML Progress elements only uses animated
values if the element is "indeterminant", having no "value"
attribute. Yet the LayoutProgress code was always setting up and
using an animation timer, and requesting paint invalidation
repeatedly, even if the value never changed.

Re-enables a test on Mac that should now pass.

Adds a unit test to verify the timer starts and stops as required.

R=wangxianzhu@chromium.org
BUG=570756
Committed: https://crrev.com/4be952b08a2b6634a810432cb0fd501607853164
Cr-Commit-Position: refs/heads/master@{#377405}

[Stephen Chennney, 2016-02-22 22:06:22]

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

[commit-bot: I haz the power, 2016-02-22 22:08:31]

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

[Stephen Chennney, 2016-02-22 22:08:43]

I expect to have to revisit this on Mac, to either re-disable the layout test or
set NeedsExpectations. I want to see the results first, however.

Meanwhile, does it seem right to you? To understand it, you also need to look at
ThemePainterDefault::paintProgressBar and ThemePainterMac::paintProgressBar. I'm
not entirely sure the latter will still work, but I think it will.

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

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  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_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Xianzhu, 2016-02-22 23:17:27]

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/NeverFixTests (left):

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/NeverFixTests:45: [ Mac ]
fast/dom/HTMLProgressElement/progress-element.html [ WontFix ]
How did this test animate on Mac? Why did it animate on Mac only?

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutProgress.cpp (right):

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutProgress.cpp:79: return
m_animationTimer.isActive();
Nit: Can you use m_animating (and rename this function to isAnimating())?

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutProgressTest.cpp (right):

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutProgressTest.cpp:23:
document().view()->updateAllLifecyclePhases();
Nits:
- You can use RenderingTest::setBodyInnerHTML() instead of the above 2 lines.
- String::fromUTF8() is unnecessary.

[Stephen Chennney, 2016-02-23 19:24:02]

I've updated the test to reflect the comments.

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/NeverFixTests (left):

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/NeverFixTests:45: [ Mac ]
fast/dom/HTMLProgressElement/progress-element.html [ WontFix ]
On 2016/02/22 23:17:27, Xianzhu wrote:
> How did this test animate on Mac? Why did it animate on Mac only?

It animated on all platforms, in the sense that we repainted it. But on Mac it
paints using an OS control, hence paints differently. I speculate that the
control painting was unstable for some reason. I want to see if it is fixed now.

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutProgress.cpp (right):

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutProgress.cpp:79: return
m_animationTimer.isActive();
On 2016/02/22 23:17:27, Xianzhu wrote:
> Nit: Can you use m_animating (and rename this function to isAnimating())?

I would rather check the timer itself, as that will catch bugs with both
m_animating being set and the timer being set (because the timer can only be
right is m_animating is right).

Better yet is to check both the flag and the timer, which will test that the
code keeps them correctly in sync.

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutProgressTest.cpp (right):

https://codereview.chromium.org/1721943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutProgressTest.cpp:23:
document().view()->updateAllLifecyclePhases();
On 2016/02/22 23:17:27, Xianzhu wrote:
> Nits:
> - You can use RenderingTest::setBodyInnerHTML() instead of the above 2 lines.
> - String::fromUTF8() is unnecessary.

Acknowledged.

[Stephen Chennney, 2016-02-23 19:24:45]

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

[commit-bot: I haz the power, 2016-02-23 19:29:02]

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

[Xianzhu, 2016-02-23 19:47:47]

lgtm

[commit-bot: I haz the power, 2016-02-23 20:57:01]

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

[commit-bot: I haz the power, 2016-02-23 20:57:02]

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

[Stephen Chennney, 2016-02-23 21:50:30]

On 2016/02/23 19:47:47, Xianzhu wrote:
> lgtm

I'll verify that the Mac test is no longer flakey, update expectations
appropriately and submit tomorrow.

[Stephen Chennney, 2016-02-24 21:00:33]

New mac expectation is stable on my machine, so re-enabling that test.
Committing with just the expectations update from the lgtm.

[Stephen Chennney, 2016-02-24 21:00:38]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-02-24 21:00:39]

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

[commit-bot: I haz the power, 2016-02-24 21:06:10]

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

[commit-bot: I haz the power, 2016-02-24 22:15:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-24 22:18:07]

Description was changed from

==========
Don't animate a progress bar if it isn't animated

The paint path for HTML Progress elements only uses animated
values if the element is "indeterminant", having no "value"
attribute. Yet the LayoutProgress code was always setting up and
using an animation timer, and requesting paint invalidation
repeatedly, even if the value never changed.

Re-enables a test on Mac that should now pass.

Adds a unit test to verify the timer starts and stops as required.

R=wangxianzhu@chromium.org
BUG=570756
==========

to

==========
Don't animate a progress bar if it isn't animated

The paint path for HTML Progress elements only uses animated
values if the element is "indeterminant", having no "value"
attribute. Yet the LayoutProgress code was always setting up and
using an animation timer, and requesting paint invalidation
repeatedly, even if the value never changed.

Re-enables a test on Mac that should now pass.

Adds a unit test to verify the timer starts and stops as required.

R=wangxianzhu@chromium.org
BUG=570756

Committed: https://crrev.com/4be952b08a2b6634a810432cb0fd501607853164
Cr-Commit-Position: refs/heads/master@{#377405}
==========

[commit-bot: I haz the power, 2016-02-24 22:18:08]

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