Allow interpolation of background-size values with keywords in CSS Animations

Allow interpolation of background-size values with keywords in CSS Animations

This change allows for positionally matched keywords in background-size CSS
Animations. This brings it in line with CSS Transitions behaviour.

Previously CSSSizeListInterpolationType inherited from CSSLengthListInterpolationType,
due to the handling of keywords this is no longer suitable.

BUG=640450
Committed: https://crrev.com/07b28f4faed3dba2c80a864a61e8af46bdf5d50a
Cr-Commit-Position: refs/heads/master@{#417134}

[alancutter (OOO until 2018), 2016-08-25 06:08:16]

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

[alancutter (OOO until 2018), 2016-08-25 06:08:16]

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/interpolation/background-size-interpolation.html
(left):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/interpolation/background-size-interpolation.html:67:
// TODO(alancutter): Fix transitions: https://crbug.com/513127
These are stale TODOs, bug has been fixed a while ago.

[suzyh_UTC10 (ex-contributor), 2016-08-26 04:14:32]

suzyh@chromium.org changed reviewers:
+ suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-08-26 04:14:34]

My main concern with this patch is how difficult it is to read
CSSSizeListInterpolationType.cpp (although I don't think this is unique to this
particular file). The issues are long names (which look similar and as such are
hard to skim read), and long functions which start to look much the same very
quickly.

Since this is also a factor in other parts of the InterpolationType scheme, I
don't propose blocking this patch on a complete fix. I'd like to see some
changes before this is submitted, but I'm open to a discussion of which changes
are reasonable in this patch vs done later vs not done.

I think the class would mostly benefit from some of the code being factored out
into one or more separate util libraries. Even though the functions wouldn't
necessarily be used by other code, I think it would make it easier to read by
dividing the code into SizeList-stuff and InterpolationType-stuff.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:12:
#include "core/css/resolver/StyleResolverState.h"
Consider also explicitly including the headers were other classes like FillSize,
Length and LengthSize are declared, to make the dependencies explicit rather
than relying on transitive includes.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:37:
CSSSizeNonInterpolableValue(CSSValueID keyword)
Is there a reason to have two constructors mirroring the two ::create functions,
or can we have the two ::create functions call one constructor with different
arguments?

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:56:
static bool sizesAreCompatible(const NonInterpolableValue* a, const
NonInterpolableValue* b)
Does this function belong in a separate helper library?

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:197:
return ListInterpolationFunctions::createList(list->length() * 2, [list](size_t
index) -> InterpolationValue {
I think I would prefer to see a separate library of helper functions than an
anonymous function defined inline like this.

The code would also be simpler and easier to read with a few parts factored out
into separate functions.

Likewise in convertSizeList above.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:264:
return CSSLengthInterpolationType::resolveInterpolableLength(interpolableValue,
nonInterpolableValue.lengthNonInterpolableValue(), conversionData,
ValueRangeNonNegative);
As a reader of this code it is surprising to have a reference to
CSSLengthInterpolationType here. I think that indicates an opportunity to move
some code to a helper function/library used here and by
CSSLengthInterpolationType.

Ditto above.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.h
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.h:12:
class CSSSizeListInterpolationType : public CSSInterpolationType {
In the CL description I think it would be useful to mention that the extra stuff
needed meant that this class had to be moved up in the hierarchy.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/LengthListPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/LengthListPropertyFunctions.cpp:50:
switch (property) {
After this change, the four CSSPropertyIDs that are valid arguments populate the
same four functions: isSet, getLength, setLength, clear. Is there now a clearer
way to write this struct? I think that might also lead to simplifications in the
rest of the file.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h:16: using
SizeList = Vector<FillSize, 1>;
"using" statements are discouraged in header files:
https://www.chromium.org/blink/coding-style#TOC-using-Statements

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/FillLayer.h (right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/FillLayer.h:40:
DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
Can you elaborate on the consequences of making this change?

[alancutter (OOO until 2018), 2016-08-26 04:43:46]

Description was changed from

==========
Allow interpolation of background-size values with keywords in CSS Animations

This change allows for positionally matched keywords in background-size CSS
Animations. This brings it in line with CSS Transitions behaviour.

BUG=640450
==========

to

==========
Allow interpolation of background-size values with keywords in CSS Animations

This change allows for positionally matched keywords in background-size CSS
Animations. This brings it in line with CSS Transitions behaviour.

Previously CSSSizeListInterpolationType inherited from
CSSLengthListInterpolationType,
due to the handling of keywords this is no longer suitable.

BUG=640450
==========

[alancutter (OOO until 2018), 2016-08-26 13:31:48]

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:12:
#include "core/css/resolver/StyleResolverState.h"
On 2016/08/26 at 04:14:34, suzyh wrote:
> Consider also explicitly including the headers were other classes like
FillSize, Length and LengthSize are declared, to make the dependencies explicit
rather than relying on transitive includes.

I think the policy is only to not include things that you don't use. I'm less
inclined to strictly include everything that is used as it becomes easy to
accidentally break the first rule as the code changes.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:37:
CSSSizeNonInterpolableValue(CSSValueID keyword)
On 2016/08/26 at 04:14:34, suzyh wrote:
> Is there a reason to have two constructors mirroring the two ::create
functions, or can we have the two ::create functions call one constructor with
different arguments?

I think it's better for assertions to be in constructors than create functions.
IMO this is neater WRT the DCHECK but I'm not fussed if you want it changed.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:56:
static bool sizesAreCompatible(const NonInterpolableValue* a, const
NonInterpolableValue* b)
On 2016/08/26 at 04:14:34, suzyh wrote:
> Does this function belong in a separate helper library?

Moved into SizeInterpolationFunctions.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:197:
return ListInterpolationFunctions::createList(list->length() * 2, [list](size_t
index) -> InterpolationValue {
On 2016/08/26 at 04:14:34, suzyh wrote:
> I think I would prefer to see a separate library of helper functions than an
anonymous function defined inline like this.
> 
> The code would also be simpler and easier to read with a few parts factored
out into separate functions.
> 
> Likewise in convertSizeList above.

This is a lambda due to the closure of list (and the method signature). I am
certain that avoiding the lambda would result in more convoluted code to deal
with the memory management involved. Most likely it will turn into what the
lambda is syntactic sugar for.

The inner parts of the lambda now live in SizeInterpolationFunctions.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:264:
return CSSLengthInterpolationType::resolveInterpolableLength(interpolableValue,
nonInterpolableValue.lengthNonInterpolableValue(), conversionData,
ValueRangeNonNegative);
On 2016/08/26 at 04:14:34, suzyh wrote:
> As a reader of this code it is surprising to have a reference to
CSSLengthInterpolationType here. I think that indicates an opportunity to move
some code to a helper function/library used here and by
CSSLengthInterpolationType.
> 
> Ditto above.

Agreed, CSSLengthInterpolationType should have its static helper functions split
off into a LengthInterpolationFunctions class similar to
ShadowInterpolationFunctions and others.
Created dependant patch.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.h
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.h:12:
class CSSSizeListInterpolationType : public CSSInterpolationType {
On 2016/08/26 at 04:14:34, suzyh wrote:
> In the CL description I think it would be useful to mention that the extra
stuff needed meant that this class had to be moved up in the hierarchy.

Done.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/LengthListPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/LengthListPropertyFunctions.cpp:50:
switch (property) {
On 2016/08/26 at 04:14:34, suzyh wrote:
> After this change, the four CSSPropertyIDs that are valid arguments populate
the same four functions: isSet, getLength, setLength, clear. Is there now a
clearer way to write this struct? I think that might also lead to
simplifications in the rest of the file.

We could inline the specific method calls (like setXPosition()) and copy paste
the iteration logic. That would kill this struct and the ugly member function
pointer dereferences.
I would recommend that for a separate change though, removing the size methods
doesn't really change whether it's a good idea or not.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h:16: using
SizeList = Vector<FillSize, 1>;
On 2016/08/26 at 04:14:34, suzyh wrote:
> "using" statements are discouraged in header files:
https://www.chromium.org/blink/coding-style#TOC-using-Statements

I think that's referring to the other form of "using" that hides namespaces,
this is a more readable version of typedef which is preferred:
http://chromium-cpp.appspot.com/#core-whitelist

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/FillLayer.h (right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/FillLayer.h:40:
DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
On 2016/08/26 at 04:14:34, suzyh wrote:
> Can you elaborate on the consequences of making this change?

It's necessary for Vector<FillSize> which does placement new.

[alancutter (OOO until 2018), 2016-08-26 13:36:16]

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

[commit-bot: I haz the power, 2016-08-26 13:36:45]

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-08-26 13:50:40]

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

[commit-bot: I haz the power, 2016-08-26 13:50:42]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alancutter (OOO until 2018), 2016-08-29 01:56:33]

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

[commit-bot: I haz the power, 2016-08-29 01:56:51]

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-08-29 03:19:48]

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

[commit-bot: I haz the power, 2016-08-29 03:19:49]

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-08-30 08:01:07]

I'm more or less happy with this, but I'll give it another pass with fresh eyes
(and maybe wait for the patches it depends on, if that might trigger changes)
before stamping.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:37:
CSSSizeNonInterpolableValue(CSSValueID keyword)
On 2016/08/26 at 13:31:48, alancutter wrote:
> On 2016/08/26 at 04:14:34, suzyh wrote:
> > Is there a reason to have two constructors mirroring the two ::create
functions, or can we have the two ::create functions call one constructor with
different arguments?
> 
> I think it's better for assertions to be in constructors than create
functions. IMO this is neater WRT the DCHECK but I'm not fussed if you want it
changed.

I think this is a style thing, and I am undecided. My context is a discussion
about what happens in error conditions for constructors, a topic which I am not
yet comfortably familiar with:
https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
I haven't found anything in the Chromium/Blink style guides that express a
position on this. If you can find a precedent in docs or code that would set out
a preferred approach for this, let's go with that. Otherwise, I think it's
clearer and conceptually simpler to have a single constructor, and in that case
would probably put the DCHECK in the create function.

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h:16: using
SizeList = Vector<FillSize, 1>;
On 2016/08/26 at 13:31:48, alancutter wrote:
> On 2016/08/26 at 04:14:34, suzyh wrote:
> > "using" statements are discouraged in header files:
https://www.chromium.org/blink/coding-style#TOC-using-Statements
> 
> I think that's referring to the other form of "using" that hides namespaces,
this is a more readable version of typedef which is preferred:
http://chromium-cpp.appspot.com/#core-whitelist

Oh, right. I missed that distinction. All good, then.

[Eric Willigers, 2016-08-31 01:47:06]

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/SizeInterpolationFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/SizeInterpolationFunctions.cpp:45: { }
DCHECK(m_lengthNonInterpolableValue);

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.cpp:45: for
(const FillLayer* fillLayer = getFillLayer(property, style); fillLayer &&
fillLayer->isSizeSet(); fillLayer = fillLayer->next())
We stop when we hit a fillLayer with size not set, instead of skipping over. Is
that intentional?

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h
(right):

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.h:23: static
void setSizeList(CSSPropertyID, ComputedStyle&, SizeList&);
const SizeList&

[alancutter (OOO until 2018), 2016-09-05 07:44:27]

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
(right):

https://codereview.chromium.org/2280553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp:37:
CSSSizeNonInterpolableValue(CSSValueID keyword)
On 2016/08/30 at 08:01:07, suzyh wrote:
> On 2016/08/26 at 13:31:48, alancutter wrote:
> > On 2016/08/26 at 04:14:34, suzyh wrote:
> > > Is there a reason to have two constructors mirroring the two ::create
functions, or can we have the two ::create functions call one constructor with
different arguments?
> > 
> > I think it's better for assertions to be in constructors than create
functions. IMO this is neater WRT the DCHECK but I'm not fussed if you want it
changed.
> 
> I think this is a style thing, and I am undecided. My context is a discussion
about what happens in error conditions for constructors, a topic which I am not
yet comfortably familiar with:
https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
> I haven't found anything in the Chromium/Blink style guides that express a
position on this. If you can find a precedent in docs or code that would set out
a preferred approach for this, let's go with that. Otherwise, I think it's
clearer and conceptually simpler to have a single constructor, and in that case
would probably put the DCHECK in the create function.

There is precedence for checking in the constructor in Blink. See: CSSValuePair,
CSSVariableData, MediaQueryMatcher
I don't think the code here is breaking the linked style guide since the check
is not a virtual method call nor is the failure without a signal (the process
crashes).
Personally it makes more sense to check in the constructor, if you put it in the
create() then there's a possibility the object may be constructed internally
with incorrect parameters. Eliminating these possibilities is a key benefit of
using assertions.

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/SizeInterpolationFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/SizeInterpolationFunctions.cpp:45: { }
On 2016/08/31 at 01:47:06, Eric Willigers wrote:
> DCHECK(m_lengthNonInterpolableValue);

Done.

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/SizeListPropertyFunctions.cpp:45: for
(const FillLayer* fillLayer = getFillLayer(property, style); fillLayer &&
fillLayer->isSizeSet(); fillLayer = fillLayer->next())
On 2016/08/31 at 01:47:06, Eric Willigers wrote:
> We stop when we hit a fillLayer with size not set, instead of skipping over.
Is that intentional?

Yes. Unset/nullptr indicates the end of the list.

[alancutter (OOO until 2018), 2016-09-06 00:28:06]

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

[commit-bot: I haz the power, 2016-09-06 00:28:21]

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-09-06 02:13:37]

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

[commit-bot: I haz the power, 2016-09-06 02:13:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alancutter (OOO until 2018), 2016-09-06 02:27:31]

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/SizeInterpolationFunctions.cpp
(right):

https://codereview.chromium.org/2280553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/SizeInterpolationFunctions.cpp:45: { }
On 2016/09/05 at 07:44:27, alancutter wrote:
> On 2016/08/31 at 01:47:06, Eric Willigers wrote:
> > DCHECK(m_lengthNonInterpolableValue);
> 
> Done.

Whoops, that's actually allowed to be nullptr. Removed check.

[alancutter (OOO until 2018), 2016-09-06 02:27:45]

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

[commit-bot: I haz the power, 2016-09-06 02:27:57]

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

[suzyh_UTC10 (ex-contributor), 2016-09-06 03:23:08]

I share your frustration at how much code this required, but have no further
useful suggestions at this point. lgtm.

[commit-bot: I haz the power, 2016-09-06 04:03:43]

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

[commit-bot: I haz the power, 2016-09-06 04:03:44]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2016-09-07 08:01:32]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-09-07 08:01:45]

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-09-07 08:08:14]

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

[commit-bot: I haz the power, 2016-09-07 08:08:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alancutter (OOO until 2018), 2016-09-07 08:29:18]

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

[alancutter (OOO until 2018), 2016-09-07 08:29:19]

+timloh for change to core/style/FillLayer.h

[alancutter (OOO until 2018), 2016-09-07 08:30:23]

https://codereview.chromium.org/2280553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/FillLayer.h (right):

https://codereview.chromium.org/2280553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/FillLayer.h:40:
DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
As discussed on previous patchsets this is due to Vector<FillSize, 1> in
SizeListPropertyFunctions.h.

[Timothy Loh, 2016-09-07 09:53:51]

On 2016/09/07 08:30:23, alancutter wrote:
>
https://codereview.chromium.org/2280553002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/style/FillLayer.h (right):
> 
>
https://codereview.chromium.org/2280553002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/style/FillLayer.h:40:
> DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
> As discussed on previous patchsets this is due to Vector<FillSize, 1> in
> SizeListPropertyFunctions.h.

rs-lgtm

[alancutter (OOO until 2018), 2016-09-08 00:37:00]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-09-08 00:37:17]

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-09-08 00:42:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-08 00:45:51]

Description was changed from

==========
Allow interpolation of background-size values with keywords in CSS Animations

This change allows for positionally matched keywords in background-size CSS
Animations. This brings it in line with CSS Transitions behaviour.

Previously CSSSizeListInterpolationType inherited from
CSSLengthListInterpolationType,
due to the handling of keywords this is no longer suitable.

BUG=640450
==========

to

==========
Allow interpolation of background-size values with keywords in CSS Animations

This change allows for positionally matched keywords in background-size CSS
Animations. This brings it in line with CSS Transitions behaviour.

Previously CSSSizeListInterpolationType inherited from
CSSLengthListInterpolationType,
due to the handling of keywords this is no longer suitable.

BUG=640450

Committed: https://crrev.com/07b28f4faed3dba2c80a864a61e8af46bdf5d50a
Cr-Commit-Position: refs/heads/master@{#417134}
==========

[commit-bot: I haz the power, 2016-09-08 00:45:53]

Patchset 6 (id:??) landed as
https://crrev.com/07b28f4faed3dba2c80a864a61e8af46bdf5d50a
Cr-Commit-Position: refs/heads/master@{#417134}