[svg2] Make SVGLength wrap a CSSPrimitiveValue

[svg2] Make SVGLength wrap a CSSPrimitiveValue.

BUG=467464

[Erik Dahlström, 2015-05-28 15:19:55]

ed@chromium.org changed reviewers:
+ fs@opera.com, pdr@chromium.org

[Erik Dahlström, 2015-05-28 15:19:56]

The remaining test failures seem to overlap with  http://crbug.com/463358, so
opening this for review.

The performance tryrun (see below) shows some regressions, and I haven't yet
looked into that in detail, but I'm guessing that it might be either the css
parser (setup overhead, or that it hits a slowpath), or that we use more
temporary CSSPrimitiveValue objects with this approach.

http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-05...

Anyhow, curious to hear your thoughts on this approach.

[pdr., 2015-05-28 21:25:08]

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

[pdr., 2015-05-28 21:28:19]

I think this is clever, clean, and a nice perf win too.

+cc timloh, WDYT about this approach?

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:53: CSSPrimitiveValue* getCSSPrimitiveValue() {
return m_value.get(); }
It looks like this is only used to access the primitiveType. Can you just expose
"UnitType primitiveType() { return m_value->primitiveType(); } const"?

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:154: RefPtrWillBeMember<CSSPrimitiveValue> m_value;
Does this need to be a pointer? I wonder if it can just be a member like
"CSSPrimitiveValue m_value";

Otherwise, can it just be an OwnPtr to avoid some overhead of ref counting?

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:155: unsigned m_unitMode : 2;
Can we use CSSPrimitiveValue's LengthUnitType and get rid of this entirely?

[fs, 2015-05-29 08:09:42]

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.cpp:42: , m_value(CSSPrimitiveValue::create(0,
CSSPrimitiveValue::CSS_NUMBER))
I wonder if we could have this initially value statically (shared). (At least
until, and if, CSSValue is refactored in which case there would be no allocation
here again.)

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:154: RefPtrWillBeMember<CSSPrimitiveValue> m_value;
On 2015/05/28 21:28:18, pdr wrote:
> Does this need to be a pointer? I wonder if it can just be a member like
> "CSSPrimitiveValue m_value";
> 
> Otherwise, can it just be an OwnPtr to avoid some overhead of ref counting?

Then it'd need to be copied when it's used as a presentation attribute. (I
notice that that simplification/optimization is not in this CL though, but I'd
expect that to be a close follower.)

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:155: unsigned m_unitMode : 2;
On 2015/05/28 21:28:18, pdr wrote:
> Can we use CSSPrimitiveValue's LengthUnitType and get rid of this entirely?

That's a different thing. In most cases it should be possible to trivially get
rid of this by specifying it at the callsite. The exception I can think of is
SVGLengthTearOff (the DOM interface for .value etc)

[fs, 2015-05-29 13:49:07]

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:154: RefPtrWillBeMember<CSSPrimitiveValue> m_value;
On 2015/05/29 08:09:42, fs wrote:
> On 2015/05/28 21:28:18, pdr wrote:
> > Does this need to be a pointer? I wonder if it can just be a member like
> > "CSSPrimitiveValue m_value";
> > 
> > Otherwise, can it just be an OwnPtr to avoid some overhead of ref counting?
> 
> Then it'd need to be copied when it's used as a presentation attribute. (I
> notice that that simplification/optimization is not in this CL though, but I'd
> expect that to be a close follower.)

Oh, yeah, almost forgot, Oilpan. That also means that a trace() definition is
missing.

[Erik Dahlström, 2015-06-02 11:57:45]

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:53: CSSPrimitiveValue* getCSSPrimitiveValue() {
return m_value.get(); }
On 2015/05/28 21:28:19, pdr wrote:
> It looks like this is only used to access the primitiveType. Can you just
expose
> "UnitType primitiveType() { return m_value->primitiveType(); } const"?

Sure.

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:154: RefPtrWillBeMember<CSSPrimitiveValue> m_value;
On 2015/05/29 13:49:06, fs wrote:
> On 2015/05/29 08:09:42, fs wrote:
> > On 2015/05/28 21:28:18, pdr wrote:
> > > Does this need to be a pointer? I wonder if it can just be a member like
> > > "CSSPrimitiveValue m_value";
> > > 
> > > Otherwise, can it just be an OwnPtr to avoid some overhead of ref
counting?
> > 
> > Then it'd need to be copied when it's used as a presentation attribute. (I
> > notice that that simplification/optimization is not in this CL though, but
I'd
> > expect that to be a close follower.)
> 
> Oh, yeah, almost forgot, Oilpan. That also means that a trace() definition is
> missing.

Ok, will add that.

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:154: RefPtrWillBeMember<CSSPrimitiveValue> m_value;
On 2015/05/28 21:28:18, pdr wrote:
> Does this need to be a pointer? I wonder if it can just be a member like
> "CSSPrimitiveValue m_value";
> 
> Otherwise, can it just be an OwnPtr to avoid some overhead of ref counting?

It can't be a simple member, since CSSPrimitiveValue is immutable.

https://codereview.chromium.org/1162453002/diff/20001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:155: unsigned m_unitMode : 2;
On 2015/05/29 08:09:42, fs wrote:
> On 2015/05/28 21:28:18, pdr wrote:
> > Can we use CSSPrimitiveValue's LengthUnitType and get rid of this entirely?
> 
> That's a different thing. In most cases it should be possible to trivially get
> rid of this by specifying it at the callsite. The exception I can think of is
> SVGLengthTearOff (the DOM interface for .value etc)

Yes, it's not quite the same as LengthUnitType I think. Attempting to get rid of
this should be done separately.

[pdr., 2015-06-02 17:06:40]

LGTM

Could you please wait before committing for Fredrik's comments?

[fs, 2015-06-02 18:20:09]

LGTM w/ small, almost nitish, issue.

I'm going to start an Oilpan trybot for PS3 just so we have that covered.

https://codereview.chromium.org/1162453002/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1162453002/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:171: if (unitType ==
CSSPrimitiveValue::CSS_UNKNOWN || unitType > CSSPrimitiveValue::CSS_PC) {
This is a bit theoretical perhaps, but this should really compare the "DOM
exposed" values (like before.) It just happens that they are the same I suppose.
Could we add a helper isValidLengthUnit and use here, below and in the assert in
toCSSUnitType? Just in case the CSS types change in the future (and to discern
one from the other.)

[Timothy Loh, 2015-06-03 01:31:08]

https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:702: if (isNumber())
Is this something that needs to be here? We currently only use the number type
to represent things which are <number>s in CSS, while unitless lengths (quirks
mode / attributes / zero) will still generate length primitive values.

[Erik Dahlström, 2015-06-03 14:51:30]

https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:702: if (isNumber())
On 2015/06/03 01:31:08, Timothy Loh wrote:
> Is this something that needs to be here? We currently only use the number type
> to represent things which are <number>s in CSS, while unitless lengths (quirks
> mode / attributes / zero) will still generate length primitive values.

This is needed due to
https://codereview.chromium.org/1162453002/patch/40001/50012.

If this is not here, we hit the assert one line down, which causes a bunch of
svg LayoutTest failures. Unitless length values are allowed in svg, and we get
another set of test failures if the parsing of svg lengths allows unitless
values but then represents them as values that had a px unit specified. If the
value was unitless the expectation is that it's preserved that way.

https://codereview.chromium.org/1162453002/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1162453002/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:171: if (unitType ==
CSSPrimitiveValue::CSS_UNKNOWN || unitType > CSSPrimitiveValue::CSS_PC) {
On 2015/06/02 18:20:08, fs wrote:
> This is a bit theoretical perhaps, but this should really compare the "DOM
> exposed" values (like before.) It just happens that they are the same I
suppose.
> Could we add a helper isValidLengthUnit and use here, below and in the assert
in
> toCSSUnitType? Just in case the CSS types change in the future (and to discern
> one from the other.)

The unknown unit is a "valid" unit, however one is not allowed to convert or
create new such values. I've added a helper function.

Or did you mean that it should more carefully check each possible unit to ensure
that the ordering doesn't change?

[fs, 2015-06-03 15:15:10]

https://codereview.chromium.org/1162453002/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1162453002/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:171: if (unitType ==
CSSPrimitiveValue::CSS_UNKNOWN || unitType > CSSPrimitiveValue::CSS_PC) {
On 2015/06/03 14:51:30, Erik Dahlström wrote:
> On 2015/06/02 18:20:08, fs wrote:
> > This is a bit theoretical perhaps, but this should really compare the "DOM
> > exposed" values (like before.) It just happens that they are the same I
> suppose.
> > Could we add a helper isValidLengthUnit and use here, below and in the
assert
> in
> > toCSSUnitType? Just in case the CSS types change in the future (and to
discern
> > one from the other.)
> 
> The unknown unit is a "valid" unit, however one is not allowed to convert or
> create new such values. I've added a helper function.
> 
> Or did you mean that it should more carefully check each possible unit to
ensure
> that the ordering doesn't change?

More to check against what is considered the "external" type, so essentially
symmetry with the toSVGLengthType(...) in SVGLengthTearOff::unitType().

So in the new helper it'd just be:

return type != LengthTypeUnknown && type <= LengthTypePC;

[Timothy Loh, 2015-06-04 02:10:16]

https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:702: if (isNumber())
On 2015/06/03 14:51:30, Erik Dahlström wrote:
> On 2015/06/03 01:31:08, Timothy Loh wrote:
> > Is this something that needs to be here? We currently only use the number
type
> > to represent things which are <number>s in CSS, while unitless lengths
(quirks
> > mode / attributes / zero) will still generate length primitive values.
> 
> This is needed due to
> https://codereview.chromium.org/1162453002/patch/40001/50012.
> 
> If this is not here, we hit the assert one line down, which causes a bunch of
> svg LayoutTest failures. Unitless length values are allowed in svg, and we get
> another set of test failures if the parsing of svg lengths allows unitless
> values but then represents them as values that had a px unit specified. If the
> value was unitless the expectation is that it's preserved that way.
> 

How are we getting into here though? I only see the CSSPrimitiveValue being used
for storage in SVGLength. If the parser change is making other things change, we
should work out a different way to do this part of the patch.

[Erik Dahlström, 2015-06-04 12:00:31]

On 2015/06/04 02:10:16, Timothy Loh wrote:
>
https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
> File Source/core/css/CSSPrimitiveValue.cpp (right):
> 
>
https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
> Source/core/css/CSSPrimitiveValue.cpp:702: if (isNumber())
> On 2015/06/03 14:51:30, Erik Dahlström wrote:
> > On 2015/06/03 01:31:08, Timothy Loh wrote:
> > > Is this something that needs to be here? We currently only use the number
> type
> > > to represent things which are <number>s in CSS, while unitless lengths
> (quirks
> > > mode / attributes / zero) will still generate length primitive values.
> > 
> > This is needed due to
> > https://codereview.chromium.org/1162453002/patch/40001/50012.
> > 
> > If this is not here, we hit the assert one line down, which causes a bunch
of
> > svg LayoutTest failures. Unitless length values are allowed in svg, and we
get
> > another set of test failures if the parsing of svg lengths allows unitless
> > values but then represents them as values that had a px unit specified. If
the
> > value was unitless the expectation is that it's preserved that way.
> > 
> 
> How are we getting into here though? I only see the CSSPrimitiveValue being
used
> for storage in SVGLength. If the parser change is making other things change,
we
> should work out a different way to do this part of the patch.

Previously, all parsed CSSPrimitiveValues get the px unit forced onto them if
there was no specified unit. With the change in CSSParserFastPaths we allow
unitless CSSPrimitiveValues through, and so need to deal with the resulting
values - I guess I don't see that as strange, I expected that. I should say that
it's certainly possible to restrict the change in CSSParserFastPaths to just
SVGAttributeMode, but even if that change is made the convertLength calls would
still happen for some svg elements.

Anyway, the CSSPrimitiveValues get handled via recalcStyle, here's a partial
callstack that hits the convertLength if-statement for number (this is from
running LayoutTests/svg/dom/animated-reference-in-shadow-tree.html):

0	blink::CSSPrimitiveValue::convertToLength	CSSPrimitiveValue.cpp	703	0x2d99328	
1	blink::StyleBuilderConverter::convertLength	StyleBuilderConverter.cpp	536	0x2e8fb04	
2	blink::StyleBuilderConverter::convertLengthSizing	StyleBuilderConverter.cpp	557	0x2e8fcac	
3	blink::StyleBuilderFunctions::applyValueCSSPropertyHeight	StyleBuilderFunctions.cpp	251	0x3c1e06e	
4	blink::StyleBuilder::applyProperty	StyleBuilder.cpp	259	0x3c18664	
5	blink::StyleBuilder::applyProperty	StyleBuilderCustom.cpp	133	0x3288bbb	
6	blink::StyleResolver::applyProperties<(blink::CSSPropertyPriority)1>	StyleResolver.cpp	1256	0x2ea93e6	
7	blink::StyleResolver::applyMatchedProperties<(blink::CSSPropertyPriority)1>	StyleResolver.cpp	1282	0x2ea31a5	
8	blink::StyleResolver::applyMatchedProperties	StyleResolver.cpp	1376	0x2e9e325	
9	blink::StyleResolver::styleForElement	StyleResolver.cpp	611	0x2e9d9a0	
10	blink::SVGElement::customStyleForLayoutObject	SVGElement.cpp	894	0x3599f42	
11	blink::Element::styleForLayoutObject	Element.cpp	1578	0x28dfcbd	
12	blink::Element::recalcOwnStyle	Element.cpp	1672	0x28e04d1	
13	blink::Element::recalcStyle	Element.cpp	1627	0x28e0168	
14	blink::ContainerNode::recalcChildStyle	ContainerNode.cpp	1296	0x286d0cb	
15	blink::Element::recalcStyle	Element.cpp	1642	0x28e024e	
16	blink::ContainerNode::recalcChildStyle	ContainerNode.cpp	1296	0x286d0cb	
17	blink::ShadowRoot::recalcStyle	ShadowRoot.cpp	148	0x29aa39a	
18	blink::Element::recalcStyle	Element.cpp	1640	0x28e0228	
19	blink::ContainerNode::recalcChildStyle	ContainerNode.cpp	1296	0x286d0cb	
20	blink::Element::recalcStyle	Element.cpp	1642	0x28e024e	
...	<More>

[fs, 2015-07-06 11:44:48]

On 2015/06/04 12:00:31, Erik Dahlström wrote:
> On 2015/06/04 02:10:16, Timothy Loh wrote:
> >
>
https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
> > File Source/core/css/CSSPrimitiveValue.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1162453002/diff/40001/Source/core/css/CSSPrim...
> > Source/core/css/CSSPrimitiveValue.cpp:702: if (isNumber())
> > On 2015/06/03 14:51:30, Erik Dahlström wrote:
> > > On 2015/06/03 01:31:08, Timothy Loh wrote:
> > > > Is this something that needs to be here? We currently only use the
number
> > type
> > > > to represent things which are <number>s in CSS, while unitless lengths
> > (quirks
> > > > mode / attributes / zero) will still generate length primitive values.
> > > 
> > > This is needed due to
> > > https://codereview.chromium.org/1162453002/patch/40001/50012.
> > > 
> > > If this is not here, we hit the assert one line down, which causes a bunch
> of
> > > svg LayoutTest failures. Unitless length values are allowed in svg, and we
> get
> > > another set of test failures if the parsing of svg lengths allows unitless
> > > values but then represents them as values that had a px unit specified. If
> the
> > > value was unitless the expectation is that it's preserved that way.
> > > 
> > 
> > How are we getting into here though? I only see the CSSPrimitiveValue being
> used
> > for storage in SVGLength. If the parser change is making other things
change,
> we
> > should work out a different way to do this part of the patch.
> 
> Previously, all parsed CSSPrimitiveValues get the px unit forced onto them if
> there was no specified unit. With the change in CSSParserFastPaths we allow
> unitless CSSPrimitiveValues through, and so need to deal with the resulting
> values - I guess I don't see that as strange, I expected that. I should say
that
> it's certainly possible to restrict the change in CSSParserFastPaths to just
> SVGAttributeMode, but even if that change is made the convertLength calls
would
> still happen for some svg elements.
> 
> Anyway, the CSSPrimitiveValues get handled via recalcStyle, here's a partial
> callstack that hits the convertLength if-statement for number (this is from
> running LayoutTests/svg/dom/animated-reference-in-shadow-tree.html):

In particular TC, is this caused by the fact that there's an animation of
'height'? AFAICS, SVGLength::cssUnitTypeQuirk would do the <number> -> 'px'
coercion for regular presentation attribute, while the SMIL "CSS" animation will
just valueToString and set that (which won't include a unit naturally). Dropping
out of the fast-path for SMIL animations doesn't seem like a major issue. Were
there any more issues/fails/tests difference with just the coercion?

As for slow-path vs. fast-path, it ought to be ok to do something like:

bool allowUnitless = isQuirksModeBehavior(cssParserMode) ||
isUnitLessLengthParsingEnabledForMode(cssParserMode);
if (number && !allowUnitless)
    return nullptr;

to get the fast-path to allow these. Could go in independently from what it
looks like. If it's only for the SMIL code-path though...

So, are there other issues that need to be considered wrt unit coercion?

[Stephen Chennney, 2015-10-22 16:11:32]

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

[Stephen Chennney, 2015-10-22 16:11:33]

Looks like this got dropped, close to landing, when Erik left us. I'll look into
understanding the patch and moving it forward.

[fs, 2015-10-22 16:22:47]

On 2015/10/22 at 16:11:33, schenney wrote:
> Looks like this got dropped, close to landing, when Erik left us. I'll look
into understanding the patch and moving it forward.

I had plans for picking this up, but certainly don't mind if you do. I think all
that's needed is to resolve how to handle unitless SVGLengths.

[Stephen Chennney, 2015-10-23 19:16:25]

Moved to https://codereview.chromium.org/1421533006

[Stephen Chennney, 2015-10-23 19:16:54]

Description was changed from

==========
[svg2] Make SVGLength wrap a CSSPrimitiveValue.

BUG=467464
==========

to

==========
[svg2] Make SVGLength wrap a CSSPrimitiveValue.

BUG=467464
==========