Avoid visually transitioning on length CSS properties when zoom changes

Avoid visually transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

Transitions may still start due to floating point & integer truncation
approximations but the visual effects of the transitions will be negligible.

BUG=267316
Committed: https://crrev.com/0ef90d0b121f4da99b02804a4bc0f774a8254655
Cr-Commit-Position: refs/heads/master@{#438740}

[alancutter (OOO until 2018), 2016-12-09 08:04:22]

Description was changed from

==========
+zoomTransitions

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

to

==========
Avoid transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

BUG=267316
==========

[alancutter (OOO until 2018), 2016-12-09 08:04:22]

alancutter@chromium.org changed reviewers:
+ ericwilligers@chromium.org

[alancutter (OOO until 2018), 2016-12-12 00:28:58]

alancutter@chromium.org changed reviewers:
+ suzyh@chromium.org
- ericwilligers@chromium.org

[alancutter (OOO until 2018), 2016-12-12 00:29:08]

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

[commit-bot: I haz the power, 2016-12-12 00:29:13]

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-12-12 02:18:17]

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

[commit-bot: I haz the power, 2016-12-12 02:18:18]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[suzyh_UTC10 (ex-contributor), 2016-12-12 04:30:13]

In principle I'm happy with this change. I have several comments around making
the code and individual patches more skimmable and easier to maintain. Most of
these suggestions boil down to splitting the patch into multiple patches to
isolate the actual behaviour change from the renames or other refactorings that
it induces.

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html
(right):

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:14: var
lengthProperties = [
Is there a canonical source we can get this list of properties from, instead of
hard-coding it here?

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:69:
test(() => {
This is not a test. Why can this not be written without the "test(() => { ... },
'Test setup');" surrounds?

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:74: if
(window.internals) {
Please add a comment to explain what these two options are for. Is there a
reason not to use only "target.style.zoom = 2"?

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
(right):

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp:636:
return createFromPropertyLength(property, style);
I can see why we might want to group all of these together, but I'd prefer not
to reorder the case lines here. It creates extra churn and obscures the actual
change of this patch. Please leave the ordering of cases alone in this patch.

I'd be happy to see a separate patch that changes the case ordering without
changing the behaviour (either before or after this change), if that still seems
desirable. I would probably go for strict alphabetical order of the cases, even
if that involves duplicating the action for each case. However, I'd also be fine
with a different grouping to avoid duplicating the action as long as (a) the
properties within each group are in alphabetical order, and (b) blank
lines/comments/etc are used to make it easy to see how and why the groups are
formed.

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp
(right):

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:113: T
roundedClampTo(double value,
Is there any chance that the changes to function names and arguments (which is
kind of a refactoring) can be separated from the actual change to avoid
transitions? The function name/argument changes are driven by the behaviour
change, but doing both in a single CL means it's not obvious which parts of the
patch actually produce the behaviour change.

[alancutter (OOO until 2018), 2016-12-13 02:36:34]

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html
(right):

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:14: var
lengthProperties = [
On 2016/12/12 at 04:30:13, suzyh wrote:
> Is there a canonical source we can get this list of properties from, instead
of hard-coding it here?

Not one I would be comfortable relying on not changing unexpectedly.
Generally it's fine for tests to have hard coded values for the purpose of
regression testing.

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:69:
test(() => {
On 2016/12/12 at 04:30:13, suzyh wrote:
> This is not a test. Why can this not be written without the "test(() => { ...
}, 'Test setup');" surrounds?

Without the test() wrapper if this code throws an error it won't be caught by
the test harness and the test won't fail. Added comment.

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:74: if
(window.internals) {
On 2016/12/12 at 04:30:13, suzyh wrote:
> Please add a comment to explain what these two options are for. Is there a
reason not to use only "target.style.zoom = 2"?

The working group is trying to get rid of the zoom property so I don't want to
rely on it. I should probably just not use it at all and error if internals
doesn't exist.

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
(right):

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp:636:
return createFromPropertyLength(property, style);
On 2016/12/12 at 04:30:13, suzyh wrote:
> I can see why we might want to group all of these together, but I'd prefer not
to reorder the case lines here. It creates extra churn and obscures the actual
change of this patch. Please leave the ordering of cases alone in this patch.

Done.

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp
(right):

https://codereview.chromium.org/2562643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:113: T
roundedClampTo(double value,
On 2016/12/12 at 04:30:13, suzyh wrote:
> Is there any chance that the changes to function names and arguments (which is
kind of a refactoring) can be separated from the actual change to avoid
transitions? The function name/argument changes are driven by the behaviour
change, but doing both in a single CL means it's not obvious which parts of the
patch actually produce the behaviour change.

Done.

[suzyh_UTC10 (ex-contributor), 2016-12-14 05:53:30]

lgtm with one minor comment. You'll still need approval from one of the style
folks for core/css/.

https://codereview.chromium.org/2562643002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html
(right):

https://codereview.chromium.org/2562643002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:70: //
The test setup is wrapped in a test() call to make sure we fail if an error is
thrown.
It's just occurred to me that we need to be sure that this test setup runs
before the tests below. I don't know whether testharness guarantees that. In
looking at the testharness docs to try to answer that question, I found a
"setup" function:
https://github.com/w3c/testharness.js/blob/master/docs/api.md#setup
Useful here?

[alancutter (OOO until 2018), 2016-12-14 05:59:27]

alancutter@chromium.org changed reviewers:
+ timloh@chromium.org

[alancutter (OOO until 2018), 2016-12-14 05:59:28]

https://codereview.chromium.org/2562643002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html
(right):

https://codereview.chromium.org/2562643002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/transition-zoomed-length.html:70: //
The test setup is wrapped in a test() call to make sure we fail if an error is
thrown.
On 2016/12/14 at 05:53:30, suzyh wrote:
> It's just occurred to me that we need to be sure that this test setup runs
before the tests below. I don't know whether testharness guarantees that. In
looking at the testharness docs to try to answer that question, I found a
"setup" function:
> https://github.com/w3c/testharness.js/blob/master/docs/api.md#setup
> Useful here?

That's exactly what I should be using, thanks!

[alancutter (OOO until 2018), 2016-12-14 06:16:13]

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

[commit-bot: I haz the power, 2016-12-14 06:16:28]

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

[alancutter (OOO until 2018), 2016-12-14 06:19:44]

+timloh for core/css.

[commit-bot: I haz the power, 2016-12-14 08:09:21]

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

[commit-bot: I haz the power, 2016-12-14 08:09:22]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2016-12-14 23:30:45]

On 2016/12/14 06:19:44, alancutter wrote:
> +timloh for core/css.

Won't this end up transitioning between two really close values most of the time
instead of not transitioning? There's still a observable difference because
transition events fire for this, right?

[alancutter (OOO until 2018), 2016-12-15 00:25:53]

Description was changed from

==========
Avoid transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

BUG=267316
==========

to

==========
Avoid visually transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

Transitions may still start due to floating point & integer truncation
approximations but the visual effects of the transitions will be negligible.

BUG=267316
==========

[alancutter (OOO until 2018), 2016-12-15 00:26:06]

On 2016/12/14 at 23:30:45, timloh wrote:
> On 2016/12/14 06:19:44, alancutter wrote:
> > +timloh for core/css.
> 
> Won't this end up transitioning between two really close values most of the
time instead of not transitioning? There's still a observable difference because
transition events fire for this, right?

Updated description.

[Timothy Loh, 2016-12-15 04:15:48]

lgtm

[alancutter (OOO until 2018), 2016-12-15 04:25:00]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-12-15 04:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2562643002/#ps60001
(title: "Use setup()")

[commit-bot: I haz the power, 2016-12-15 04:25:30]

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-12-15 04:33:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481775900091650,
"parent_rev": "2146c4b5b7816f6639780bc2c1f53249a41446d8", "commit_rev":
"4728f7fdaeb65806d306fb1ae1398a9fce4e2108"}

[commit-bot: I haz the power, 2016-12-15 04:33:40]

Description was changed from

==========
Avoid visually transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

Transitions may still start due to floating point & integer truncation
approximations but the visual effects of the transitions will be negligible.

BUG=267316
==========

to

==========
Avoid visually transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

Transitions may still start due to floating point & integer truncation
approximations but the visual effects of the transitions will be negligible.

BUG=267316

Review-Url: https://codereview.chromium.org/2562643002
==========

[commit-bot: I haz the power, 2016-12-15 04:33:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-15 04:35:23]

Description was changed from

==========
Avoid visually transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

Transitions may still start due to floating point & integer truncation
approximations but the visual effects of the transitions will be negligible.

BUG=267316

Review-Url: https://codereview.chromium.org/2562643002
==========

to

==========
Avoid visually transitioning on length CSS properties when zoom changes

This change alters transition animation handling of length properties
that happen to be stored as non-length numeric fields in ComputedStyle.
Instead of treating them as AnimatableDoubles this patch treats them
as AnimatableLengths which can deal with changes in page zoom
correctly.

Transitions may still start due to floating point & integer truncation
approximations but the visual effects of the transitions will be negligible.

BUG=267316

Committed: https://crrev.com/0ef90d0b121f4da99b02804a4bc0f774a8254655
Cr-Commit-Position: refs/heads/master@{#438740}
==========

[commit-bot: I haz the power, 2016-12-15 04:35:25]

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