Web Animations CSS: Fix crash when transitioning background or mask properties

Web Animations CSS: Fix crash when transitioning background or mask properties

This patch ensures the first layer for background and mask declares that
their values are set. This has no visible effect on the way fill layers
work, this just ensures an assertion made by the Web Animations engine
about fill layer properties is correct.

The current animation implementation did not make use of the flags on a fill layer
that indicate whether a value was set or not and will be agnostic to this change.
In order to implement correct interpolation behaviour[1] for many fill layer
properties these flags need to be used.
The existing FillLayer implementation was not setting these flags correctly for
the initial values, this patch fixes that.

[1] http://www.w3.org/TR/css3-transitions/#animtype-repeatable-list

Fixes crashing tests:
virtual/web-animations-css/transitions/multiple-background-size-transitions.html
virtual/web-animations-css/transitions/shorthand-transitions.html

BUG=257591
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161438

[alancutter (OOO until 2018), 2013-10-24 06:20:39]

This patch depends on https://codereview.chromium.org/39353003/ and
https://codereview.chromium.org/38573003/ landing first.

[dstockwell, 2013-10-24 12:42:27]

Can you expand on the "assumption made by the Web Animations engine" ?

Why doesn't the existing implementation have this problem?

[alancutter (OOO until 2018), 2013-10-24 14:16:00]

On 2013/10/24 12:42:27, dstockwell wrote:
> Can you expand on the "assumption made by the Web Animations engine" ?
> 
> Why doesn't the existing implementation have this problem?

Updated description with comparison with existing implementation.

[alancutter (OOO until 2018), 2013-11-02 01:21:08]

Dependent patches have now landed, updated description s/assumptions/assertions,
crashing tests now passing.

dstockwell: ptal

[Steve Block, 2013-11-03 10:16:47]

I'm not sure I understand this. It seems odd to add logic just to satisfy an
assert in the Web Animations logic.  Or are you suggesting that the existing
implementation was wrong to allow behavior which violates this assertion? If the
latter, can we add a test for this?

https://codereview.chromium.org/38573005/diff/180001/Source/core/rendering/st...
File Source/core/rendering/style/FillLayer.cpp (right):

https://codereview.chromium.org/38573005/diff/180001/Source/core/rendering/st...
Source/core/rendering/style/FillLayer.cpp:43:
FillLayer::FillLayer(EFillLayerType type, bool isInitiallySet)
Would something like useInitialSize be a better name? This seems like an
instruction about initialization, but the name sounds like a current state.

[alancutter (OOO until 2018), 2013-11-04 00:45:24]

On 2013/11/03 10:16:47, Steve Block wrote:
> I'm not sure I understand this. It seems odd to add logic just to satisfy an
> assert in the Web Animations logic.  Or are you suggesting that the existing
> implementation was wrong to allow behavior which violates this assertion? If
the
> latter, can we add a test for this?

The current implementation did not make use of flags on a fill layer indicating
whether a value was set or not and will be agnostic to this change.
In order to implement correct interpolation behaviour [1] for many fill layer
properties these flags need to be used.
The existing FillLayer implementation was not setting these flags correctly for
the initial values, this patch fixes that.

[1] http://www.w3.org/TR/css3-transitions/#animtype-repeatable-list


Perhaps I ought to make this clarification part of the patch description.

https://codereview.chromium.org/38573005/diff/180001/Source/core/rendering/st...
File Source/core/rendering/style/FillLayer.cpp (right):

https://codereview.chromium.org/38573005/diff/180001/Source/core/rendering/st...
Source/core/rendering/style/FillLayer.cpp:43:
FillLayer::FillLayer(EFillLayerType type, bool isInitiallySet)
On 2013/11/03 10:16:48, Steve Block wrote:
> Would something like useInitialSize be a better name? This seems like an
> instruction about initialization, but the name sounds like a current state.

Renamed to useInitialValues.

[Steve Block, 2013-11-05 04:20:48]

lgtm

Thanks for the explanation.

[commit-bot: I haz the power, 2013-11-05 04:27:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/38573005/280001

[commit-bot: I haz the power, 2013-11-05 04:27:51]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 94.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
078820867aba7156265db8e176f26b8a07c5cdb4..4d5dcbef49150e65d73b45076cd32181dfd719ee
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -94,8 +94,6 @@ crbug.com/303430
virtual/web-animations-css/transitions/retargetted-transition.h
 crbug.com/303430
virtual/web-animations-css/transitions/transition-end-event-all-properties.html
[ Crash ]
 crbug.com/303430
virtual/web-animations-css/transitions/transitions-parsing.html [ Crash ]
 crbug.com/303430 virtual/web-animations-css/transitions/visited-link-color.html
[ Crash ]
-crbug.com/303430 [ Debug ]
virtual/web-animations-css/transitions/multiple-background-size-transitions.html
[ Crash ]
-crbug.com/303430 [ Debug ]
virtual/web-animations-css/transitions/shorthand-transitions.html [ Crash ]
 # These can't be rebaselined because the failure involves flaky output.
 crbug.com/258896
virtual/web-animations-css/animations/transition-and-animation-1.html [ Failure
]
 crbug.com/258896
virtual/web-animations-css/animations/transition-and-animation-2.html [ Failure
]

[commit-bot: I haz the power, 2013-11-06 06:24:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/38573005/350001

[commit-bot: I haz the power, 2013-11-06 08:52:20]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-11-06 08:53:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/38573005/350001

[commit-bot: I haz the power, 2013-11-06 09:30:17]

Change committed as 161438