Make SVGLength wrap a CSSPrimitiveValue.

Make SVGLength wrap a CSSPrimitiveValue.

This patch is needed to facilitate improvements to SVG attribute parsing, in turn demanded by the reconciliation of CSS and SVG attributes and properties from the SVG 2.0 spec. One parser to rule them all is a better approach than writing SVG specific parsers for what are essentially the same types as CSS.

The only significant difference between the primitive types in CSS and SVG are related to dimensionless lengths, those for which the user defines no units, just a number. In CSS these are disallowed except when in quirks mode, while for SVG a pure number is considered in "user units" (see http://www.w3.org/TR/SVG/types.html#InterfaceSVGLength). Basically, the user could be using their own dimensioning system and rather than defining some kind of customization method, the spec just assumes things without units are the user's responsibility. This works because the SVG viewport defines the mapping from SVG user units into document units.

To address this difference, we add a new CSSPrimitive UnitType for SVG user units and assign it to lengths created in SVG parsing mode that have no other unit.

R=fs@opera.com,pdr@chromium.org,timloh@chromium.org
BUG=467464
Committed: https://crrev.com/13b78a5ac7b5c84c3cc12e7b1a1b07620f0a3f98
Cr-Commit-Position: refs/heads/master@{#358153}

[Stephen Chennney, 2015-10-23 19:15:51]

This is an update of https://codereview.chromium.org/1162453002 after 6 months
or so of inactivity.

In response to the comments about handling unitless numbers, I've removed the
changes to the CSS parser, so I would expect failures again. I'll pursue the
proposed fix from fs@, which would involve slowing SMIL, which is deprecated
anyway (unless the community wins).

I was also seeing new test failures that need investigating.

So overall, it's WIP.

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

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

[commit-bot: I haz the power, 2015-10-23 19:16:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/1

[commit-bot: I haz the power, 2015-10-23 19:23:08]

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

[commit-bot: I haz the power, 2015-10-23 19:23:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2015-10-23 20:28:59]

On 2015/10/23 at 19:15:51, schenney wrote:
> This is an update of https://codereview.chromium.org/1162453002 after 6 months
or so of inactivity.
> 
> In response to the comments about handling unitless numbers, I've removed the
changes to the CSS parser, so I would expect failures again. I'll pursue the
proposed fix from fs@, which would involve slowing SMIL, which is deprecated
anyway (unless the community wins).
> 
> I was also seeing new test failures that need investigating.

Ok, sounds good, hope it pans out. I think there was an issue with valueAsString
as well (and it looks that way based on the current test-results.)

Noted a few nits below while refreshing my memory.

https://codereview.chromium.org/1421533006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1421533006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:46: && type <= (unsigned
short)CSSPrimitiveValue::UnitType::Picas;
Nit: Use static_cast

https://codereview.chromium.org/1421533006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGLengthTearOff.h (right):

https://codereview.chromium.org/1421533006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.h:39: 
Nit: Drop

[Stephen Chennney, 2015-10-27 21:28:00]

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

[Stephen Chennney, 2015-10-27 21:28:32]

I forgot to address one of the nits. This patch is to see if my fix for the
unitless length thing works.

[commit-bot: I haz the power, 2015-10-27 21:28:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/20001

[Stephen Chennney, 2015-10-27 21:33:28]

On 2015/10/27 21:28:32, Stephen Chenney wrote:
> I forgot to address one of the nits. This patch is to see if my fix for the
> unitless length thing works.

Tons of crashes for some reason I don't understand yet. That's it for me today.

[fs, 2015-10-27 22:44:15]

On 2015/10/27 at 21:33:28, schenney wrote:
> On 2015/10/27 21:28:32, Stephen Chenney wrote:
> > I forgot to address one of the nits. This patch is to see if my fix for the
> > unitless length thing works.
> 
> Tons of crashes for some reason I don't understand yet. That's it for me
today.

Likely this:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

MOst presentation attributes will not "normalize" unitless -> px, and now with
the "quirk" in the CSS parser (fast path) those end up as "numbers" (unitless)
rather than "pixels". I think.

[commit-bot: I haz the power, 2015-10-27 22:54:25]

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

[commit-bot: I haz the power, 2015-10-27 22:54:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Stephen Chennney, 2015-10-28 17:06:26]

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

[commit-bot: I haz the power, 2015-10-28 17:07:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/40001

[Stephen Chennney, 2015-10-28 17:08:31]

The latest version of the patch only significantly modifies the behavior of one
existing test. i.e. I have no new expectations in this patch.

I went with a special unit type for unitless lengths, that are really "User"
units in SVG. Review appreciated at this point while I fix the couple of test
issues. Looks like conversion to rems is broken.

[commit-bot: I haz the power, 2015-10-28 18:16:38]

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

[commit-bot: I haz the power, 2015-10-28 18:16:39]

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

[fs, 2015-10-28 20:14:34]

On 2015/10/28 at 17:08:31, schenney wrote:
...
> I went with a special unit type for unitless lengths, that are really "User"
units in SVG. Review appreciated at this point while I fix the couple of test
issues.

Hmm, I guess this is essentially the same as adding another CSS parser mode
(which I think we discussed in the old review? If not, then likely in
person/offline only...) I guess I could buy this, but we'll need to see what Tim
thinks. Also, I suppose we should try to weed out any existing "*Number" from
SVGLength* (except the DOM exposed stuff) in favor of UserCoordinates?

> Looks like conversion to rems is broken.

Hopefully spotted.

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (left):

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:513:
ASSERT(isCalculated());
I guess this should be restored (I think this implies cssCalcValue() returns
non-null or something like that.) We ought to take the isLength() branch for
"User", right?

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.h:100: || unitType ==
CSSPrimitiveValue::UnitType::Chs;    }
} on next line.

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:46: && type <=
static_cast<unsigned short>(CSSPrimitiveValue::UnitType::UserCoordinates);
This ought to still be Picas?

[Timothy Loh, 2015-10-29 00:49:31]

On 2015/10/28 20:14:34, fs wrote:
> On 2015/10/28 at 17:08:31, schenney wrote:
> ...
> > I went with a special unit type for unitless lengths, that are really "User"
> units in SVG. Review appreciated at this point while I fix the couple of test
> issues.
> 
> Hmm, I guess this is essentially the same as adding another CSS parser mode
> (which I think we discussed in the old review? If not, then likely in
> person/offline only...) I guess I could buy this, but we'll need to see what
Tim
> thinks. Also, I suppose we should try to weed out any existing "*Number" from
> SVGLength* (except the DOM exposed stuff) in favor of UserCoordinates?

I haven't looked yet but this at least sounds better than the previous approach
of overloading the Number type. Some more context in the patch description about
the additional requirements on CSSPrimitiveValue would help since I'm not very
familiar with SVGLength :-)

[Stephen Chennney, 2015-10-30 17:30:15]

Description was changed from

==========
Make SVGLength wrap a CSSPrimitiveValue.

R=fs@opera.com,pdr@chromium.org,timloh@chromium.org
BUG=467464
==========

to

==========
Make SVGLength wrap a CSSPrimitiveValue.

This patch is needed to facilitate improvements to SVG attribute parsing, in
turn demanded by the reconciliation of CSS and SVG attributes and properties
from the SVG 2.0 spec. One parser to rule them all is a better approach than
writing SVG specific parsers for what are essentially the same types as CSS.

The only significant difference between the primitive types in CSS and SVG are
related to dimensionless lengths, those for which the user defines no units,
just a number. In CSS these are disallowed except when in quirks mode, while for
SVG a pure number is considered in "user units" (see
http://www.w3.org/TR/SVG/types.html#InterfaceSVGLength). Basically, the user
could be using their own dimensioning system and rather than defining some kind
of customization method, the spec just assumes things without units are the
user's responsibility. This works because the SVG viewport defines the mapping
from SVG user units into document units.

To address this difference, we add a new CSSPrimitive UnitType for SVG user
units and assign it to lengths created in SVG parsing mode that have no other
unit.

R=fs@opera.com,pdr@chromium.org,timloh@chromium.org
BUG=467464
==========

[Stephen Chennney, 2015-10-30 18:00:55]

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (left):

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:513:
ASSERT(isCalculated());
On 2015/10/28 20:14:34, fs wrote:
> I guess this should be restored (I think this implies cssCalcValue() returns
> non-null or something like that.) We ought to take the isLength() branch for
> "User", right?

I removed it because the exact same assertion is the first thing in
cssCalcValue().

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1421533006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:46: && type <=
static_cast<unsigned short>(CSSPrimitiveValue::UnitType::UserCoordinates);
On 2015/10/28 20:14:34, fs wrote:
> This ought to still be Picas?

Yes. Done.

[Stephen Chennney, 2015-10-30 18:02:40]

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

[commit-bot: I haz the power, 2015-10-30 18:06:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/60001

[commit-bot: I haz the power, 2015-10-30 19:04:27]

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

[commit-bot: I haz the power, 2015-10-30 19:04:28]

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

[fs, 2015-10-30 19:50:00]

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

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:69:
CSSPrimitiveValue::UnitType::Chs };
Nit: }; on next line

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp:52:
baseValue()->newValueSpecifiedUnits(CSSPrimitiveValue::UnitType::Number, 0);
All of these explicit uses should probably be UserUnits now.

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.h:58: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Number)
Can we still get Number here?

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:57: return
static_cast<CSSPrimitiveValue::UnitType>(type);
I think this function should map LengthTypeNumber to UserUnits, else we'll have
two ways of saying the same thing. (And valueAsString = "42" would give a
different result than newValueSpecifiedUnits(42, ...NUMBER))

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:65: case
CSSPrimitiveValue::UnitType::Number:
...and if the above was true, this would not be a valid value in this function.

[Stephen Chennney, 2015-11-03 21:59:41]

I think I've fixed the test cases, but maybe not all. Here's another round for
the try bots.

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp:52:
baseValue()->newValueSpecifiedUnits(CSSPrimitiveValue::UnitType::Number, 0);
On 2015/10/30 19:50:00, fs wrote:
> All of these explicit uses should probably be UserUnits now.

Yes. I think I got them.

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.h:58: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Number)
On 2015/10/30 19:50:00, fs wrote:
> Can we still get Number here?

Running tests to find out.

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:57: return
static_cast<CSSPrimitiveValue::UnitType>(type);
On 2015/10/30 19:50:00, fs wrote:
> I think this function should map LengthTypeNumber to UserUnits, else we'll
have
> two ways of saying the same thing. (And valueAsString = "42" would give a
> different result than newValueSpecifiedUnits(42, ...NUMBER))

Yes. Done.

https://codereview.chromium.org/1421533006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:65: case
CSSPrimitiveValue::UnitType::Number:
On 2015/10/30 19:50:00, fs wrote:
> ...and if the above was true, this would not be a valid value in this
function.

Yes. Done, or at least about to be tested.

[Stephen Chennney, 2015-11-03 21:59:58]

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

[commit-bot: I haz the power, 2015-11-03 22:00:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/80001

[fs, 2015-11-03 22:40:14]

Looks good, but something with clip/scroll layers seems to be having a bad day
=/

Some nits for the road:

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp
(right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:69:
CSSPrimitiveValue::UnitType::Chs };
Nit: }; on next line.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:137:
CSSPrimitiveValue::UnitType unitType = CSSPrimitiveValue::UnitType::Number;
UserUnits?

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:154:
unitType = CSSPrimitiveValue::UnitType::Number;
UserUnits?

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:299: if
(cssParserMode == SVGAttributeMode)
Maybe do something similar here as in the fast-path parser, i.e:

...UnitType unitType = ...Pixels;
if (<svg-mode>)
    unitType = ...UserUnits;
return cssValuePool().createValue(..., unitType);

or some variation thereof.

[commit-bot: I haz the power, 2015-11-03 23:37:05]

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

[commit-bot: I haz the power, 2015-11-03 23:37:05]

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

[Timothy Loh, 2015-11-04 00:33:44]

Seems like this will add calc() support to (the web-exposed) SVGLength. Is this
intentional / tested?

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (left):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:513:
ASSERT(isCalculated());
This change probably isn't needed

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:114:
fprintf(stderr, "parseSimpleLengthValue() parsed %g, svg? %s, quirks %s\n",
number, svgMode ? "true" : "false", quirksMode ? "true" : "false");
You left in some debugging code here ;)

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:295:
fprintf(stderr, "consumeLength() parsed %g\n", token.numericValue());
More debugging code ;)

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:213: if
(primitiveValue.isNumber())
Don't think this is needed, probably left in from the previous version of the
change

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:112: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Percentage) {
isPercentage()

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:123: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Percentage)
isPercentage()

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:132: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Percentage) {
isPercentage()

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:153:
ASSERT(parsed->isPrimitiveValue());
This is.. super redundant, since we checked it a few lines above and the
toCSSPrimitiveValue case also asserts this.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:190: case
CSSPrimitiveValue::UnitType::Unknown:
this might be an ASSERT_NOT_REACHED case (i.e. let default catch it)

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:246: return 0;
this might be an ASSERT_NOT_REACHED case (i.e. let default catch it)

[fs, 2015-11-04 08:41:12]

On 2015/11/04 at 00:33:44, timloh wrote:
> Seems like this will add calc() support to (the web-exposed) SVGLength. Is
this intentional / tested?

The intention is to allow it eventually, but we should probably enable that as a
separate change - so I guess the parsing code in SVGLength should check for that
case and reject it (for now.)

[Stephen Chennney, 2015-11-04 18:04:47]

I'm not sure about the calc() thing, so I need advice.

We're also now parsing all unit specifies as case-insensitive (I think), where
the SVG spec says that SVG attributes should be lower case, which is different
from CSS.

But overall I think this is close if not ready to land.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp
(right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:69:
CSSPrimitiveValue::UnitType::Chs };
On 2015/11/03 22:40:14, fs wrote:
> Nit: }; on next line.

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:137:
CSSPrimitiveValue::UnitType unitType = CSSPrimitiveValue::UnitType::Number;
On 2015/11/03 22:40:14, fs wrote:
> UserUnits?

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/LengthSVGInterpolation.cpp:154:
unitType = CSSPrimitiveValue::UnitType::Number;
On 2015/11/03 22:40:14, fs wrote:
> UserUnits?

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (left):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:513:
ASSERT(isCalculated());
On 2015/11/04 00:33:44, Timothy Loh wrote:
> This change probably isn't needed

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:114:
fprintf(stderr, "parseSimpleLengthValue() parsed %g, svg? %s, quirks %s\n",
number, svgMode ? "true" : "false", quirksMode ? "true" : "false");
On 2015/11/04 00:33:44, Timothy Loh wrote:
> You left in some debugging code here ;)

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:295:
fprintf(stderr, "consumeLength() parsed %g\n", token.numericValue());
On 2015/11/04 00:33:44, Timothy Loh wrote:
> More debugging code ;)

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:299: if
(cssParserMode == SVGAttributeMode)
On 2015/11/03 22:40:14, fs wrote:
> Maybe do something similar here as in the fast-path parser, i.e:
> 
> ...UnitType unitType = ...Pixels;
> if (<svg-mode>)
>     unitType = ...UserUnits;
> return cssValuePool().createValue(..., unitType);
> 
> or some variation thereof.

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:213: if
(primitiveValue.isNumber())
On 2015/11/04 00:33:44, Timothy Loh wrote:
> Don't think this is needed, probably left in from the previous version of the
> change

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:112: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Percentage) {
On 2015/11/04 00:33:44, Timothy Loh wrote:
> isPercentage()

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:123: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Percentage)
On 2015/11/04 00:33:44, Timothy Loh wrote:
> isPercentage()

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:132: if
(m_value->typeWithCalcResolved() == CSSPrimitiveValue::UnitType::Percentage) {
On 2015/11/04 00:33:44, Timothy Loh wrote:
> isPercentage()

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:148: if (!parsed ||
!parsed->isPrimitiveValue()) {
Does this catch and prevent the calc() case?

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLength.cpp:153:
ASSERT(parsed->isPrimitiveValue());
On 2015/11/04 00:33:44, Timothy Loh wrote:
> This is.. super redundant, since we checked it a few lines above and the
> toCSSPrimitiveValue case also asserts this.

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:190: case
CSSPrimitiveValue::UnitType::Unknown:
On 2015/11/04 00:33:44, Timothy Loh wrote:
> this might be an ASSERT_NOT_REACHED case (i.e. let default catch it)

Done.

https://codereview.chromium.org/1421533006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:246: return 0;
On 2015/11/04 00:33:44, Timothy Loh wrote:
> this might be an ASSERT_NOT_REACHED case (i.e. let default catch it)

Done.

[Stephen Chennney, 2015-11-04 18:04:53]

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

[commit-bot: I haz the power, 2015-11-04 18:05:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/100001

[Stephen Chennney, 2015-11-04 18:06:48]

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGLength.cpp:97:
m_value->typeWithCalcResolved());
Is a check for calc() needed here?

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGLength.cpp:148: if (!parsed ||
!parsed->isPrimitiveValue()) {
Or does this protect us from calc()?

[commit-bot: I haz the power, 2015-11-04 18:08:05]

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

[commit-bot: I haz the power, 2015-11-04 18:08:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Stephen Chennney, 2015-11-04 18:32:00]

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

[commit-bot: I haz the power, 2015-11-04 18:33:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/120001

[commit-bot: I haz the power, 2015-11-04 19:50:00]

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

[commit-bot: I haz the power, 2015-11-04 19:50:01]

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

[fs, 2015-11-04 21:12:18]

On 2015/11/04 at 18:04:47, schenney wrote:
> I'm not sure about the calc() thing, so I need advice.

In SVG2, SVGLength is defined in terms of CSS terms:

https://svgwg.org/svg2-draft/types.html#InterfaceSVGLength

i.e. by referring to http://www.w3.org/TR/css3-values/#length-value etc. CSS
Values and Units also defines calc() by saying:

"It can be used wherever <length>, ... values are allowed."
 (http://www.w3.org/TR/css3-values/#calc-notation)

But landing that in a follow-up CL seems reasonable, so let's disallow them for
now.

> We're also now parsing all unit specifies as case-insensitive (I think), where
the SVG spec says that SVG attributes should be lower case, which is different
from CSS.

The spec reference above should give the answer to this as well.
(http://www.w3.org/TR/css3-values/#dimensions)

(Note: Using the TR specs because that what SVG2 currently references.)

> But overall I think this is close if not ready to land.

Yes, this LGTM with the restriction for calc() and "additional" unit-types
added.

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGLength.cpp:148: if (!parsed ||
!parsed->isPrimitiveValue()) {
On 2015/11/04 at 18:06:48, Stephen Chenney wrote:
> Or does this protect us from calc()?

I think what we need is to add additional constraints here. Something like:

// TODO(fs): We want to allow calc() eventually.
CSSPrimitiveValue& value = toCSSPrimitiveValue(...);
if (value.isCalculated() ||
!supportedCSSUnitTypes(value.typeWithCalcResolved()))
   return; // Error

Where supported... is a helper that just returns true for all the types we
currently support.

I'm sure Tim can enlighten us as to whether there're more elegant ways to
express that.

[Stephen Chennney, 2015-11-04 22:05:47]

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

[commit-bot: I haz the power, 2015-11-04 22:06:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421533006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421533006/160001

[Stephen Chennney, 2015-11-04 22:06:52]

timloh, over to you for the final word. Hit the CQ if you think it's good - I
don't know when I'll get to it next.

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGLength.cpp:97:
m_value->typeWithCalcResolved());
On 2015/11/04 18:06:48, Stephen Chenney wrote:
> Is a check for calc() needed here?

No, I see that this maintains the existing type.

[commit-bot: I haz the power, 2015-11-05 00:12:11]

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

[commit-bot: I haz the power, 2015-11-05 00:12:13]

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

[Timothy Loh, 2015-11-05 02:36:24]

On 2015/11/04 22:06:52, Stephen Chenney (on leave) wrote:
> timloh, over to you for the final word. Hit the CQ if you think it's good - I
> don't know when I'll get to it next.
> 
>
https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):
> 
>
https://codereview.chromium.org/1421533006/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/SVGLength.cpp:97:
> m_value->typeWithCalcResolved());
> On 2015/11/04 18:06:48, Stephen Chenney wrote:
> > Is a check for calc() needed here?
> 
> No, I see that this maintains the existing type.

lgtm

[Timothy Loh, 2015-11-05 02:37:23]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2015-11-05 02:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1421533006/#ps160001
(title: "Error on calc and invalid types")

[commit-bot: I haz the power, 2015-11-05 02:39:02]

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

[commit-bot: I haz the power, 2015-11-05 03:18:20]

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

[commit-bot: I haz the power, 2015-11-05 03:18:21]

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

[fs, 2015-11-05 08:36:14]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-11-05 08:36:34]

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

[commit-bot: I haz the power, 2015-11-05 10:53:43]

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

[commit-bot: I haz the power, 2015-11-05 10:53:44]

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

[fs, 2015-11-05 11:16:39]

On 2015/11/05 at 10:53:44, commit-bot wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Looks like there's a single pixel diff on svg/hixie/perf/006.xml - highly
annoying...

[Stephen Chennney, 2015-11-05 18:03:58]

On 2015/11/05 11:16:39, fs wrote:
> On 2015/11/05 at 10:53:44, commit-bot wrote:
> > Try jobs failed on following builders:
> >   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> 
> Looks like there's a single pixel diff on svg/hixie/perf/006.xml - highly
> annoying...

I'll try to update and land tonight.

[Stephen Chennney, 2015-11-05 19:29:03]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2015-11-05 19:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1421533006/#ps180001
(title: "Added expectation")

[commit-bot: I haz the power, 2015-11-05 19:29:18]

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

[commit-bot: I haz the power, 2015-11-05 20:40:35]

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

[commit-bot: I haz the power, 2015-11-05 20:40:36]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2015-11-05 22:07:55]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-11-05 22:09:15]

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

[commit-bot: I haz the power, 2015-11-05 22:48:31]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-05 22:49:11]

Patchset 10 (id:??) landed as
https://crrev.com/13b78a5ac7b5c84c3cc12e7b1a1b07620f0a3f98
Cr-Commit-Position: refs/heads/master@{#358153}