Web Animations - responsive interpolation

Web Animations - responsive interpolation

Whenever inheriting to and from CSS values like inherit or currentColor, or compound values containing font-dependent lengths, we avoid capturing the styles up-front.

Instead, we only obtain the start and end styles when it is time to interpolate between them.

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

[dstockwell, 2014-05-26 01:16:03]

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
return false;
Shouldn't the default be true?

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:43:
return false;
Doesn't this depend on what the initial value is?

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:46:
return false;
Shouldn't the default be true?

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:98:
return false;
Expand these cases and make the default true?

[shans, 2014-05-26 03:31:01]

Overall approach looks good.

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
Source/core/animation/StringKeyframe.cpp:123: m_animatableValueCache =
StyleResolver::applyAndSnapshotAnimatableValue(*element, property,
*fromCSSValue);
This is a bad name. Can we go back to createAnimatableValueSnapshot?

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.h (left):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
Source/core/animation/StringKeyframe.h:39: ASSERT(m_animatableValueCache);
Why did we remove this assert?

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:22:
StyleResolver::applyAndSnapshotAnimatableValue(*state.element(), m_id,
*m_startCSSValue, state.style()),
Have two different functions. I think a function that applies to the style
depending on whether it is provided is crazyTown.

https://codereview.chromium.org/292173009/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:763: style = RenderStyle::create();
This looks like a recipe for introducing bugs in the future. Please provide
separate code paths!!

[alancutter (OOO until 2018), 2014-05-26 05:22:41]

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.h (left):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
Source/core/animation/StringKeyframe.h:39: ASSERT(m_animatableValueCache);
On 2014/05/26 03:31:01, shans wrote:
> Why did we remove this assert?

This may return nullptr for transform/opacity now if style dependent values are
used in the keyframe, see the change in CompositorAnimations.cpp.
The sooner we kill this function the better.

https://codereview.chromium.org/292173009/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:763: style = RenderStyle::create();
On 2014/05/26 03:31:01, shans wrote:
> This looks like a recipe for introducing bugs in the future. Please provide
> separate code paths!!

Perhaps the style parameter should be non-optional (a reference) and the check
should be reworked into the callsites.

[Eric Willigers, 2014-05-26 06:32:47]

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
Source/core/animation/StringKeyframe.cpp:123: m_animatableValueCache =
StyleResolver::applyAndSnapshotAnimatableValue(*element, property,
*fromCSSValue);
On 2014/05/26 03:31:01, shans wrote:
> This is a bad name. Can we go back to createAnimatableValueSnapshot?

Done.

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.h (left):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/St...
Source/core/animation/StringKeyframe.h:39: ASSERT(m_animatableValueCache);
On 2014/05/26 03:31:01, shans wrote:
> Why did we remove this assert?

With deferred style resolving, the keyframe's cached value can be null.

CompositorAnimations::isCandidateForAnimationOnCompositor now explicitly checks
for null.

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:22:
StyleResolver::applyAndSnapshotAnimatableValue(*state.element(), m_id,
*m_startCSSValue, state.style()),
On 2014/05/26 03:31:01, shans wrote:
> Have two different functions. I think a function that applies to the style
> depending on whether it is provided is crazyTown.

Agreed, done.

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
return false;
On 2014/05/26 01:16:04, dstockwell wrote:
> Shouldn't the default be true?

Done. Note this leads to a couple of extra failures in
LayoutTests/animations/interpolation/background-image-interpolation-expected.txt

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:43:
return false;
On 2014/05/26 01:16:04, dstockwell wrote:
> Doesn't this depend on what the initial value is?

Done.

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:46:
return false;
On 2014/05/26 01:16:04, dstockwell wrote:
> Shouldn't the default be true?

Done.

https://codereview.chromium.org/292173009/diff/20001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:98:
return false;
On 2014/05/26 01:16:04, dstockwell wrote:
> Expand these cases and make the default true?

I have made the color constants false and default true. For unlisted values, we
can risk being slow.

[alancutter (OOO until 2018), 2014-05-26 10:12:08]

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:235:
case CSSValueYellowgreen:
This makes me sad. ):
I personally think the smaller negated switch statement is more reasonable,
especially if this is inaccurate for the sake of simplification.

[shans, 2014-05-26 10:47:48]

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:235:
case CSSValueYellowgreen:
On 2014/05/26 10:12:10, alancutter wrote:
> This makes me sad. ):
> I personally think the smaller negated switch statement is more reasonable,
> especially if this is inaccurate for the sake of simplification.

Are these contiguous? Comparisons with the min and max might keep everyone
happy?

[dstockwell, 2014-05-26 11:20:40]

https://codereview.chromium.org/292173009/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/292173009/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:758: if (element.renderStyle())
Do we ever need to use the element's render style now?

Can we just use a singleton RenderStyle for this purpose? (can defer this to a
later change)

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
return true;
I think we need to handle transform here now.

+fixme, for everything else.

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:235:
case CSSValueYellowgreen:
On 2014/05/26 10:47:48, shans wrote:
> On 2014/05/26 10:12:10, alancutter wrote:
> > This makes me sad. ):
> > I personally think the smaller negated switch statement is more reasonable,
> > especially if this is inaccurate for the sake of simplification.
> 
> Are these contiguous? Comparisons with the min and max might keep everyone
> happy?

I suppose we could just return true for all IDs for now, is there any particular
case that we need to optimize now?

[alancutter (OOO until 2018), 2014-05-26 15:51:32]

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
return true;
On 2014/05/26 11:20:40, dstockwell wrote:
> I think we need to handle transform here now.
> 
> +fixme, for everything else.

Transforms are handled by the CSS_VALUE_LIST branch.

[dstockwell, 2014-05-26 23:40:14]

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
return true;
On 2014/05/26 15:51:32, alancutter wrote:
> On 2014/05/26 11:20:40, dstockwell wrote:
> > I think we need to handle transform here now.
> > 
> > +fixme, for everything else.
> 
> Transforms are handled by the CSS_VALUE_LIST branch.

What does the list contain?

[alancutter (OOO until 2018), 2014-05-26 23:48:25]

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
(right):

https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
return true;
On 2014/05/26 23:40:15, dstockwell wrote:
> On 2014/05/26 15:51:32, alancutter wrote:
> > On 2014/05/26 11:20:40, dstockwell wrote:
> > > I think we need to handle transform here now.
> > > 
> > > +fixme, for everything else.
> > 
> > Transforms are handled by the CSS_VALUE_LIST branch.
> 
> What does the list contain?

The transform arguments.
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...

[dstockwell, 2014-05-26 23:50:41]

On 2014/05/26 23:48:25, alancutter wrote:
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
> (right):
> 
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
> return true;
> On 2014/05/26 23:40:15, dstockwell wrote:
> > On 2014/05/26 15:51:32, alancutter wrote:
> > > On 2014/05/26 11:20:40, dstockwell wrote:
> > > > I think we need to handle transform here now.
> > > > 
> > > > +fixme, for everything else.
> > > 
> > > Transforms are handled by the CSS_VALUE_LIST branch.
> > 
> > What does the list contain?
> 
> The transform arguments.
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...

So, don't we need to handle CSSTransformValue?

[alancutter (OOO until 2018), 2014-05-26 23:53:24]

On 2014/05/26 23:50:41, dstockwell wrote:
> On 2014/05/26 23:48:25, alancutter wrote:
> >
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> > File
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> > Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
> > return true;
> > On 2014/05/26 23:40:15, dstockwell wrote:
> > > On 2014/05/26 15:51:32, alancutter wrote:
> > > > On 2014/05/26 11:20:40, dstockwell wrote:
> > > > > I think we need to handle transform here now.
> > > > > 
> > > > > +fixme, for everything else.
> > > > 
> > > > Transforms are handled by the CSS_VALUE_LIST branch.
> > > 
> > > What does the list contain?
> > 
> > The transform arguments.
> >
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
> >
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
> 
> So, don't we need to handle CSSTransformValue?

Not specifically just incidentally.

[dstockwell, 2014-05-27 00:01:07]

On 2014/05/26 23:53:24, alancutter wrote:
> On 2014/05/26 23:50:41, dstockwell wrote:
> > On 2014/05/26 23:48:25, alancutter wrote:
> > >
> >
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> > > File
> Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> > >
Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
> > > return true;
> > > On 2014/05/26 23:40:15, dstockwell wrote:
> > > > On 2014/05/26 15:51:32, alancutter wrote:
> > > > > On 2014/05/26 11:20:40, dstockwell wrote:
> > > > > > I think we need to handle transform here now.
> > > > > > 
> > > > > > +fixme, for everything else.
> > > > > 
> > > > > Transforms are handled by the CSS_VALUE_LIST branch.
> > > > 
> > > > What does the list contain?
> > > 
> > > The transform arguments.
> > >
> >
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
> > >
> >
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
> > 
> > So, don't we need to handle CSSTransformValue?
> 
> Not specifically just incidentally.

Wont the code as written always use the deferred path for transform?

[Eric Willigers, 2014-05-27 00:05:14]

On 2014/05/26 10:47:48, shans wrote:
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> File Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
> (right):
> 
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:235:
> case CSSValueYellowgreen:
> On 2014/05/26 10:12:10, alancutter wrote:
> > This makes me sad. ):
> > I personally think the smaller negated switch statement is more reasonable,
> > especially if this is inaccurate for the sake of simplification.
> 
> Are these contiguous? Comparisons with the min and max might keep everyone
> happy?

Changed to check if id is in color ranges.

[alancutter (OOO until 2018), 2014-05-27 00:11:19]

On 2014/05/27 00:01:07, dstockwell wrote:
> On 2014/05/26 23:53:24, alancutter wrote:
> > On 2014/05/26 23:50:41, dstockwell wrote:
> > > On 2014/05/26 23:48:25, alancutter wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> > > > File
> > Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/292173009/diff/60001/Source/core/animation/in...
> > > >
> Source/core/animation/interpolation/DeferredLegacyStyleInterpolation.cpp:41:
> > > > return true;
> > > > On 2014/05/26 23:40:15, dstockwell wrote:
> > > > > On 2014/05/26 15:51:32, alancutter wrote:
> > > > > > On 2014/05/26 11:20:40, dstockwell wrote:
> > > > > > > I think we need to handle transform here now.
> > > > > > > 
> > > > > > > +fixme, for everything else.
> > > > > > 
> > > > > > Transforms are handled by the CSS_VALUE_LIST branch.
> > > > > 
> > > > > What does the list contain?
> > > > 
> > > > The transform arguments.
> > > >
> > >
> >
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
> > > >
> > >
> >
>
https://chromium.googlesource.com/experimental/chromium/blink/+/master/Source...
> > > 
> > > So, don't we need to handle CSSTransformValue?
> > 
> > Not specifically just incidentally.
> 
> Wont the code as written always use the deferred path for transform?

This should probably be one of the unit test cases.

https://codereview.chromium.org/292173009/diff/80001/Source/core/animation/in...
File
Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp
(right):

https://codereview.chromium.org/292173009/diff/80001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp:89:

I recommend adding a helper function like "bool test(CSSPropertyID, const char*
value);" that returns the result of interpolationRequiresStyleResolve() on the
parsed string.

[Eric Willigers, 2014-05-27 04:20:34]

Unit test for transform added.

https://codereview.chromium.org/292173009/diff/80001/Source/core/animation/in...
File
Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp
(right):

https://codereview.chromium.org/292173009/diff/80001/Source/core/animation/in...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp:89:

On 2014/05/27 00:11:19, alancutter wrote:
> I recommend adding a helper function like "bool test(CSSPropertyID, const
char*
> value);" that returns the result of interpolationRequiresStyleResolve() on the
> parsed string.

Done.

[dstockwell, 2014-05-27 04:32:03]

lgtm

[Eric Willigers, 2014-05-27 05:21:19]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2014-05-27 05:21:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ericwilligers@chromium.org/292173009/1...

[dstockwell, 2014-05-27 06:45:53]

This seems to be failing or timing out in unit tests:
AnimationDeferredLegacyStyleInterpolationTest.Inherit

[commit-bot: I haz the power, 2014-05-27 06:52:56]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9749)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9568)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9022)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9204)

[commit-bot: I haz the power, 2014-05-27 07:09:29]

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

[commit-bot: I haz the power, 2014-05-27 07:09:29]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9029)

[Eric Willigers, 2014-05-27 07:29:25]

On 2014/05/27 06:45:53, dstockwell wrote:
> This seems to be failing or timing out in unit tests:
> AnimationDeferredLegacyStyleInterpolationTest.Inherit

ASSERT parseValue should have been RELEASE_ASSERT

[Eric Willigers, 2014-05-27 07:29:35]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2014-05-27 07:29:53]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ericwilligers@chromium.org/292173009/1...

[dstockwell, 2014-05-27 07:39:54]

https://codereview.chromium.org/292173009/diff/140001/Source/core/animation/i...
File
Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp
(right):

https://codereview.chromium.org/292173009/diff/140001/Source/core/animation/i...
Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp:26:
RELEASE_ASSERT(BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string,
false, parserMode, 0));
It's probably clearer to write this as:
result = parseValue(...)
ASSERT(result);

(or gtest's assert_true)

[commit-bot: I haz the power, 2014-05-27 10:58:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9780)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9599)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9218)

[commit-bot: I haz the power, 2014-05-27 11:46:31]

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

[commit-bot: I haz the power, 2014-05-27 11:46:31]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9604)

[alancutter (OOO until 2018), 2014-05-27 12:22:43]

On 2014/05/27 11:46:31, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9604)

shape-outside interpolation is failing.
It looks like the style builders and the parsers are mismatched on auto/none
keywords for this property.

[Eric Willigers, 2014-05-27 18:16:29]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2014-05-27 18:16:50]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ericwilligers@chromium.org/292173009/1...

[commit-bot: I haz the power, 2014-05-27 19:56:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9854)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9318)

[commit-bot: I haz the power, 2014-05-27 21:58:13]

Change committed as 174900

[dstockwell, 2014-05-28 00:46:21]

Alan has a fix for the shape issue, but I'm not sure whats going on with the svg
test that was failing.

These changes shouldn't affect CSS Animation. It's also not clear why it was
only failing on linux. Was it just a flake?

[Nils Barth (inactive), 2014-05-28 01:33:20]

On 2014/05/27 21:58:13, I haz the power (commit-bot) wrote:
> Change committed as 174900

This changes breaks the build, with
../../third_party/WebKit/Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp:26:14:
error: unused variable 'ok' [-Werror=unused-variable]

This is due to the line:
bool ok = BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string,
false, parserMode, 0);
ASSERT(ok);
...as the variable is unused if asserts are turned off.

I think this can be fixed by simply removing the local variable:
ASSERT(BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string, false,
parserMode, 0));

[kouhei (in TOK), 2014-05-28 01:35:24]

On 2014/05/28 01:33:20, Nils Barth wrote:
> On 2014/05/27 21:58:13, I haz the power (commit-bot) wrote:
> > Change committed as 174900
> 
> This changes breaks the build, with
>
../../third_party/WebKit/Source/core/animation/interpolation/DeferredLegacyStyleInterpolationTest.cpp:26:14:
> error: unused variable 'ok' [-Werror=unused-variable]
> 
> This is due to the line:
> bool ok = BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string,
> false, parserMode, 0);
> ASSERT(ok);
> ...as the variable is unused if asserts are turned off.
> 
> I think this can be fixed by simply removing the local variable:
> ASSERT(BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string, false,
> parserMode, 0));

or ASSERT_UNUSED(ok, ok)

[Eric Willigers, 2014-05-28 01:36:24]

Some new SVG tests were added just before my change.

The very last version of my change is for them but I uploaded it too late
and the second last version was committed.
 On 28 May 2014 10:46, <dstockwell@chromium.org> wrote:

Alan has a fix for the shape issue, but I'm not sure whats going on with
the svg
test that was failing.

These changes shouldn't affect CSS Animation. It's also not clear why it was
only failing on linux. Was it just a flake?

https://codereview.chromium.org/292173009/

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[kouhei (in TOK), 2014-05-28 01:37:26]

> I think this can be fixed by simply removing the local variable:
> ASSERT(BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string, false,
> parserMode, 0));

Actually this would prevent running parseValue on release builds so we do need
ASSERT_UNUSED

[Timothy Loh, 2014-05-28 01:39:32]

On 2014/05/28 01:37:26, kouhei wrote:
> > I think this can be fixed by simply removing the local variable:
> > ASSERT(BisonCSSParser::parseValue(dummyStyle.get(), propertyID, string,
false,
> > parserMode, 0));
> 
> Actually this would prevent running parseValue on release builds so we do need
> ASSERT_UNUSED

alancutter is compiling a quick fix. I think he's changing this to ASSERT_TRUE,
which is from gtest.