Issue 1317523002: Changed Pair to be a CSSValue

Issue 1317523002: Changed Pair to be a CSSValue (Closed)

Created:
5 years, 4 months ago by sashab

Modified:
5 years, 3 months ago

Reviewers:
Timothy Loh

CC:
blink-reviews, shans, rjwright, blink-reviews-style_chromium.org, blink-reviews-animation_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, rwlbuis, Eric Willigers, esprehn

Base URL:
https://chromium.googlesource.com/chromium/blink.git@add_const_to_primvalue

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Changed Pair to be a CSSValue Changed Pair to be a CSSValue called CSSValuePair (consistent naming with CSSValueList), and removed all references to Pair from CSSPrimitiveValue. BUG=523893 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201469

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Small test fix #

Patch Set 4 : Fixed some callsites from bad rebase #

Total comments: 20

Patch Set 5 : Review feedback #

Patch Set 6 : Added assert to CSSValuePair::equals #

Patch Set 7 : Removed dependent patchset #

Patch Set 8 : Rebase #

Created: 5 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+335 lines, -387 lines)			Patch
M	Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl	View	1	2 chunks	+4 lines, -5 lines	0 comments	Download
M	Source/core/animation/DeferredLegacyStyleInterpolation.h	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	Source/core/animation/DeferredLegacyStyleInterpolation.cpp	View	1 2 3 4	5 chunks	+9 lines, -7 lines	0 comments	Download
M	Source/core/animation/LengthPairStyleInterpolation.cpp	View		2 chunks	+6 lines, -9 lines	0 comments	Download
M	Source/core/animation/LengthPairStyleInterpolationTest.cpp	View	1 2	4 chunks	+19 lines, -19 lines	0 comments	Download
M	Source/core/core.gypi	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/css/BasicShapeFunctions.cpp	View		4 chunks	+16 lines, -17 lines	0 comments	Download
M	Source/core/css/CSSBasicShapes.h	View	1 2 3 4 5 6 7	8 chunks	+25 lines, -24 lines	0 comments	Download
M	Source/core/css/CSSBasicShapes.cpp	View	1 2 3 4 5 6 7	6 chunks	+25 lines, -26 lines	0 comments	Download
M	Source/core/css/CSSCalculationValue.cpp	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/css/CSSGradientValue.h	View		2 chunks	+9 lines, -9 lines	0 comments	Download
M	Source/core/css/CSSGradientValue.cpp	View		7 chunks	+19 lines, -16 lines	0 comments	Download
M	Source/core/css/CSSPrimitiveValue.h	View	2 3 4	7 chunks	+0 lines, -14 lines	0 comments	Download
M	Source/core/css/CSSPrimitiveValue.cpp	View	2 3 4	8 chunks	+0 lines, -36 lines	0 comments	Download
M	Source/core/css/CSSValue.h	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	Source/core/css/CSSValue.cpp	View	1	6 chunks	+14 lines, -0 lines	0 comments	Download
A	Source/core/css/CSSValuePair.h	View	1 2 3 4 5	1 chunk	+87 lines, -0 lines	0 comments	Download
A +	Source/core/css/CSSValuePair.cpp	View		1 chunk	+3 lines, -2 lines	0 comments	Download
M	Source/core/css/ComputedStyleCSSValueMapping.cpp	View	1 2 3 4	3 chunks	+6 lines, -7 lines	0 comments	Download
D	Source/core/css/Pair.h	View		1 chunk	+0 lines, -83 lines	0 comments	Download
D	Source/core/css/Pair.cpp	View		1 chunk	+0 lines, -16 lines	0 comments	Download
M	Source/core/css/parser/CSSPropertyParser.cpp	View	1 2 3 4	18 chunks	+38 lines, -46 lines	0 comments	Download
M	Source/core/css/resolver/CSSToStyleMap.cpp	View	1	7 chunks	+30 lines, -28 lines	0 comments	Download
M	Source/core/css/resolver/StyleBuilderConverter.cpp	View	1 2 3 4	4 chunks	+18 lines, -19 lines	0 comments	Download
M	Source/core/css/resolver/StyleBuilderCustom.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download

Dependent Patchsets:

Issue 1318543010 Patch 20001

Issue 1325783004 Patch 1

Issue 1325783004 Patch 20001

Issue 1318543010 Patch 40001

Messages

Total messages: 13 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Timothy Loh

https://codereview.chromium.org/1317523002/diff/50001/Source/core/animation/DeferredLegacyStyleInterpolation.cpp File Source/core/animation/DeferredLegacyStyleInterpolation.cpp (right): https://codereview.chromium.org/1317523002/diff/50001/Source/core/animation/DeferredLegacyStyleInterpolation.cpp#newcode136 Source/core/animation/DeferredLegacyStyleInterpolation.cpp:136: extra blank line https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValuePair.h File Source/core/css/CSSValuePair.h (right): https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValuePair.h#newcode35 Source/core/css/CSSValuePair.h:35: ...

5 years, 3 months ago (2015-08-28 05:51:46 UTC) #5

sashab

5 years, 3 months ago (2015-08-31 00:33:51 UTC) #6

https://codereview.chromium.org/1317523002/diff/50001/Source/core/animation/D...
File Source/core/animation/DeferredLegacyStyleInterpolation.cpp (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/animation/D...
Source/core/animation/DeferredLegacyStyleInterpolation.cpp:136: 
On 2015/08/28 05:51:46, Timothy Loh wrote:
> extra blank line

Thank you, got rid of one later too (unrelated to this patch but whatever)

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
File Source/core/css/CSSValuePair.h (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:35: // A primitive value representing a pair. 
This is useful for properties like border-radius, background-size/position,
On 2015/08/28 05:51:46, Timothy Loh wrote:
> This comment is super outdated, can we drop it?

Sure, gone.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:54: CSSValue* first() const { return
m_first.get(); }
On 2015/08/28 05:51:46, Timothy Loh wrote:
> can these be const references?

Sure, but this is kind of a big change -- I'll do it in another patch.

Is this something we want to try and do in general (make functions take const
CSSFooValue&s instead of CSSFooValue*s, like in StyleBuilderConverter,
CSSToStyleMap)? There are lots of callsites that do the equivalent of
*toCSSFooValue(foo) (storing intermediary pointers in variables/as function
arguments), maybe we should change them to dereference earlier (i.e.
toCSSFooValue(*foo)).

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:70: && m_identicalValuesPolicy ==
other.m_identicalValuesPolicy;
On 2015/08/28 05:51:46, Timothy Loh wrote:
> probably we can even assert these are the same.

The equals() method should work on all CSSValuePairs, whether they have the same
identical values policy or not. Is it really invalid to compare two Pairs that
have different identical values policies? I think it should return false but not
error.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:78: static
PassRefPtrWillBeRawPtr<CSSValuePair>
createCSSValuePair(PassRefPtrWillBeRawPtr<CSSValue> first,
PassRefPtrWillBeRawPtr<CSSValue> second, CSSValuePair::IdenticalValuesPolicy
identicalValuesPolicy = CSSValuePair::DropIdenticalValues)
On 2015/08/28 05:51:46, Timothy Loh wrote:
> I would probably just drop this function, all it does now is provide a default
> value which would probably be better if it were explicit.

Was thinking the same thing actually. Done.

Should I add 'using CSSValuePair::DropIdenticalValues,
CSSValuePair::KeepIdenticalValues' so we don't have to use the prefix
everywhere?

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:348:
ASSERT(value->isPrimitiveValue());
On 2015/08/28 05:51:46, Timothy Loh wrote:
> toCSSPrimitiveValue will do this assertion

Just wanted to make it explicit, but you're right. Done.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:649:
ASSERT(value->isPrimitiveValue());
On 2015/08/28 05:51:46, Timothy Loh wrote:
> convertLength will do all the relevant assertions

Done.

Timothy Loh

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValuePair.h File Source/core/css/CSSValuePair.h (right): https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValuePair.h#newcode54 Source/core/css/CSSValuePair.h:54: CSSValue* first() const { return m_first.get(); } On 2015/08/31 ...

5 years, 3 months ago (2015-08-31 01:08:04 UTC) #7

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
File Source/core/css/CSSValuePair.h (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:54: CSSValue* first() const { return
m_first.get(); }
On 2015/08/31 00:33:51, sashab wrote:
> On 2015/08/28 05:51:46, Timothy Loh wrote:
> > can these be const references?
> 
> Sure, but this is kind of a big change -- I'll do it in another patch.
> 
> Is this something we want to try and do in general (make functions take const
> CSSFooValue&s instead of CSSFooValue*s, like in StyleBuilderConverter,
> CSSToStyleMap)? There are lots of callsites that do the equivalent of
> *toCSSFooValue(foo) (storing intermediary pointers in variables/as function
> arguments), maybe we should change them to dereference earlier (i.e.
> toCSSFooValue(*foo)).

We discussed a while ago that most usages of CSSValue are with non-null values,
so short of some big overhaul we should at some point change all the pointers to
const refs.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:70: && m_identicalValuesPolicy ==
other.m_identicalValuesPolicy;
On 2015/08/31 00:33:51, sashab wrote:
> On 2015/08/28 05:51:46, Timothy Loh wrote:
> > probably we can even assert these are the same.
> 
> The equals() method should work on all CSSValuePairs, whether they have the
same
> identical values policy or not. Is it really invalid to compare two Pairs that
> have different identical values policies? I think it should return false but
not
> error.

We should only be comparing values that were parsed for the same property, I
think these will always the same serialization strategy.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:78: static
PassRefPtrWillBeRawPtr<CSSValuePair>
createCSSValuePair(PassRefPtrWillBeRawPtr<CSSValue> first,
PassRefPtrWillBeRawPtr<CSSValue> second, CSSValuePair::IdenticalValuesPolicy
identicalValuesPolicy = CSSValuePair::DropIdenticalValues)
On 2015/08/31 00:33:51, sashab wrote:
> On 2015/08/28 05:51:46, Timothy Loh wrote:
> > I would probably just drop this function, all it does now is provide a
default
> > value which would probably be better if it were explicit.
> 
> Was thinking the same thing actually. Done.
> 
> Should I add 'using CSSValuePair::DropIdenticalValues,
> CSSValuePair::KeepIdenticalValues' so we don't have to use the prefix
> everywhere?

It's probably fine to add the using statement to this file, but I think it's
slightly clearer without it.

sashab

5 years, 3 months ago (2015-08-31 01:29:47 UTC) #8

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
File Source/core/css/CSSValuePair.h (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:54: CSSValue* first() const { return
m_first.get(); }
On 2015/08/31 01:08:04, Timothy Loh wrote:
> On 2015/08/31 00:33:51, sashab wrote:
> > On 2015/08/28 05:51:46, Timothy Loh wrote:
> > > can these be const references?
> > 
> > Sure, but this is kind of a big change -- I'll do it in another patch.
> > 
> > Is this something we want to try and do in general (make functions take
const
> > CSSFooValue&s instead of CSSFooValue*s, like in StyleBuilderConverter,
> > CSSToStyleMap)? There are lots of callsites that do the equivalent of
> > *toCSSFooValue(foo) (storing intermediary pointers in variables/as function
> > arguments), maybe we should change them to dereference earlier (i.e.
> > toCSSFooValue(*foo)).
> 
> We discussed a while ago that most usages of CSSValue are with non-null
values,
> so short of some big overhaul we should at some point change all the pointers
to
> const refs.

I'll upload a patch in a minute that changes pair to use const refs. Also happy
to do the overhaul at some point, maybe when I get some spare time.

Would be a giant review though, might be nice if there was a way to split it up.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/CSSValu...
Source/core/css/CSSValuePair.h:70: && m_identicalValuesPolicy ==
other.m_identicalValuesPolicy;
On 2015/08/31 01:08:04, Timothy Loh wrote:
> On 2015/08/31 00:33:51, sashab wrote:
> > On 2015/08/28 05:51:46, Timothy Loh wrote:
> > > probably we can even assert these are the same.
> > 
> > The equals() method should work on all CSSValuePairs, whether they have the
> same
> > identical values policy or not. Is it really invalid to compare two Pairs
that
> > have different identical values policies? I think it should return false but
> not
> > error.
> 
> We should only be comparing values that were parsed for the same property, I
> think these will always the same serialization strategy.

Ok, added the assert.

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1317523002/diff/50001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:78: static
PassRefPtrWillBeRawPtr<CSSValuePair>
createCSSValuePair(PassRefPtrWillBeRawPtr<CSSValue> first,
PassRefPtrWillBeRawPtr<CSSValue> second, CSSValuePair::IdenticalValuesPolicy
identicalValuesPolicy = CSSValuePair::DropIdenticalValues)
On 2015/08/31 01:08:04, Timothy Loh wrote:
> On 2015/08/31 00:33:51, sashab wrote:
> > On 2015/08/28 05:51:46, Timothy Loh wrote:
> > > I would probably just drop this function, all it does now is provide a
> default
> > > value which would probably be better if it were explicit.
> > 
> > Was thinking the same thing actually. Done.
> > 
> > Should I add 'using CSSValuePair::DropIdenticalValues,
> > CSSValuePair::KeepIdenticalValues' so we don't have to use the prefix
> > everywhere?
> 
> It's probably fine to add the using statement to this file, but I think it's
> slightly clearer without it.

Sure, just left it. Makes for some long lines in the file but there are some
ridiculously long lines in it anyway

commit-bot: I haz the power

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

5 years, 3 months ago (2015-08-31 05:01:17 UTC) #12

commit-bot: I haz the power

5 years, 3 months ago (2015-08-31 06:45:32 UTC) #13

Message was sent while issue was closed.

Committed patchset #8 (id:130001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201469

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages