Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change the stroke-dasharray produces UserUnits
CSSPrimitiveValues instead of Pixels. This caused
regressions on interpolation tests, I added UserUnits
to the unitTypeToLengthUnitType switch to fix this.

BUG=499780
Committed: https://crrev.com/22ae97662a0aa4f9f697ec97d61b89c958b9cd31
Cr-Commit-Position: refs/heads/master@{#361069}

[rwlbuis, 2015-11-18 03:42:48]

Description was changed from

==========
dashes WIP

BUG=
==========

to

==========
Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780
==========

[rwlbuis, 2015-11-18 03:45:35]

rob.buis@samsung.com changed reviewers:
+ alancutter@chromium.org, timloh@chromium.org

[rwlbuis, 2015-11-18 03:45:36]

PTAL.

Note that this currently fails, because of the UserUnits change. We now create
CSSPrimitiveValue with UserUnits (instead of Pixels) for this property, but it
seems the interpolation code can't deal with this at the moment, is this a known
problem?

[Timothy Loh, 2015-11-19 02:16:47]

timloh@chromium.org changed reviewers:
+ fs@opera.com

[Timothy Loh, 2015-11-19 02:16:48]

fs: Currently we have some restrictions on parsing CSS SVG properties (calcs and
viewport units are disallowed). From skimming the code, it seems like everything
should just work if we drop these restrictions. Should we go ahead and remove
these and add tests, or are there more concerns that I'm not aware of?

[Timothy Loh, 2015-11-19 02:56:32]

On 2015/11/18 03:45:36, rwlbuis wrote:
> PTAL.
> 
> Note that this currently fails, because of the UserUnits change. We now create
> CSSPrimitiveValue with UserUnits (instead of Pixels) for this property, but it
> seems the interpolation code can't deal with this at the moment, is this a
known
> problem?

Isn't this failing because we're now parsing with the parser mode from the
CSSParserContext instead of forcing it to use SVGAttributeMode? All of the
properties in parseSVGValue force SVGAttributeMode, so I guess we should just
keep doing that for now. FF seems to behave similarly and always allow unitless
values for stroke-dasharray also.

[rwlbuis, 2015-11-19 03:16:45]

On 2015/11/19 02:56:32, Timothy Loh wrote:
> Isn't this failing because we're now parsing with the parser mode from the
> CSSParserContext instead of forcing it to use SVGAttributeMode? All of the
> properties in parseSVGValue force SVGAttributeMode, so I guess we should just
> keep doing that for now. FF seems to behave similarly and always allow
unitless
> values for stroke-dasharray also.

You are right, sorry should have uploaded the version with SVGAttributeMode. I
did that locally and the same test failure failed in different ways. As I said
above it must be because we generate UserUnits rather than Pixels with this
change. Again, not sure if that should be supported by the interpolation
framework (I am guessing it should).

[Timothy Loh, 2015-11-19 03:33:32]

On 2015/11/19 03:16:45, rwlbuis wrote:
> On 2015/11/19 02:56:32, Timothy Loh wrote:
> > Isn't this failing because we're now parsing with the parser mode from the
> > CSSParserContext instead of forcing it to use SVGAttributeMode? All of the
> > properties in parseSVGValue force SVGAttributeMode, so I guess we should
just
> > keep doing that for now. FF seems to behave similarly and always allow
> unitless
> > values for stroke-dasharray also.
> 
> You are right, sorry should have uploaded the version with SVGAttributeMode. I
> did that locally and the same test failure failed in different ways. As I said
> above it must be because we generate UserUnits rather than Pixels with this
> change. Again, not sure if that should be supported by the interpolation
> framework (I am guessing it should).

Probably it should. I think UserUnits was just missed from the switch in
CSSPrimitiveValue::unitTypeToLengthUnitType?

(alancutter@: Do you know why CSSPrimitiveValue::accumulateLengthArray just
ignores unknown components?)

[alancutter (OOO until 2018), 2015-11-19 04:01:37]

On 2015/11/19 at 03:33:32, timloh wrote:
> On 2015/11/19 03:16:45, rwlbuis wrote:
> > On 2015/11/19 02:56:32, Timothy Loh wrote:
> > > Isn't this failing because we're now parsing with the parser mode from the
> > > CSSParserContext instead of forcing it to use SVGAttributeMode? All of the
> > > properties in parseSVGValue force SVGAttributeMode, so I guess we should
just
> > > keep doing that for now. FF seems to behave similarly and always allow
> > unitless
> > > values for stroke-dasharray also.
> > 
> > You are right, sorry should have uploaded the version with SVGAttributeMode.
I
> > did that locally and the same test failure failed in different ways. As I
said
> > above it must be because we generate UserUnits rather than Pixels with this
> > change. Again, not sure if that should be supported by the interpolation
> > framework (I am guessing it should).
> 
> Probably it should. I think UserUnits was just missed from the switch in
CSSPrimitiveValue::unitTypeToLengthUnitType?

Yep, mapping UserUnits to UnitTypePixels should keep everything working the same
and make the test pass.

> (alancutter@: Do you know why CSSPrimitiveValue::accumulateLengthArray just
ignores unknown components?)

Can't think of why it doesn't ASSERT_NOT_REACHED() off the top of my head.

[fs, 2015-11-19 11:23:23]

On 2015/11/19 at 02:16:48, timloh wrote:
> fs: Currently we have some restrictions on parsing CSS SVG properties (calcs
and viewport units are disallowed). From skimming the code, it seems like
everything should just work if we drop these restrictions.
> Should we go ahead and remove these and add tests, or are there more concerns
that I'm not aware of?

Yes, the plan is/was to remove the restrictions (per SVG2 essentially). There's
crbug.com/368598 for the "units" part, I just filed crbug.com/558304 for calc().
So if you want to remove restrictions then just go ahead.

[rwlbuis, 2015-11-19 22:49:02]

Description was changed from

==========
Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780
==========

to

==========
Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change the stroke-dasharray produces UserUnits
CSSPrimitiveValues instead of Pixels as before. This caused
regressions on interpolation tests, I added UserUnits
to the unitTypeToLengthUnitType switch to fix this.

BUG=499780
==========

[rwlbuis, 2015-11-19 22:49:38]

Description was changed from

==========
Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change the stroke-dasharray produces UserUnits
CSSPrimitiveValues instead of Pixels as before. This caused
regressions on interpolation tests, I added UserUnits
to the unitTypeToLengthUnitType switch to fix this.

BUG=499780
==========

to

==========
Move stroke-dasharray property into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change the stroke-dasharray produces UserUnits
CSSPrimitiveValues instead of Pixels. This caused
regressions on interpolation tests, I added UserUnits
to the unitTypeToLengthUnitType switch to fix this.

BUG=499780
==========

[rwlbuis, 2015-11-19 22:51:14]

On 2015/11/19 03:33:32, Timothy Loh wrote:
> > You are right, sorry should have uploaded the version with SVGAttributeMode.
I
> > did that locally and the same test failure failed in different ways. As I
said
> > above it must be because we generate UserUnits rather than Pixels with this
> > change. Again, not sure if that should be supported by the interpolation
> > framework (I am guessing it should).
> 
> Probably it should. I think UserUnits was just missed from the switch in
> CSSPrimitiveValue::unitTypeToLengthUnitType?

Indeed that was the problem! I updated the patch and the Description, PTAL.

[Timothy Loh, 2015-11-20 00:06:57]

On 2015/11/19 22:51:14, rwlbuis wrote:
> On 2015/11/19 03:33:32, Timothy Loh wrote:
> > > You are right, sorry should have uploaded the version with
SVGAttributeMode.
> I
> > > did that locally and the same test failure failed in different ways. As I
> said
> > > above it must be because we generate UserUnits rather than Pixels with
this
> > > change. Again, not sure if that should be supported by the interpolation
> > > framework (I am guessing it should).
> > 
> > Probably it should. I think UserUnits was just missed from the switch in
> > CSSPrimitiveValue::unitTypeToLengthUnitType?
> 
> Indeed that was the problem! I updated the patch and the Description, PTAL.

Can we update consumeLength to reject vw/vh/vmin/vmax and consumeLengthOrPercent
to reject calcs for now? We shouldn't be adding functionality without actually
making sure it works and testing it (SVGLength parses by calling
parseSingleValue with CSSPropertyX). It looks like the svg properties only use
consumeLengthOrPercent, so we don't need to reject calc everywhere else.

[Timothy Loh, 2015-11-20 00:07:14]

On 2015/11/20 00:06:57, Timothy Loh wrote:
> Can we update consumeLength to reject vw/vh/vmin/vmax and
consumeLengthOrPercent
> to reject calcs for now? We shouldn't be adding functionality without actually
> making sure it works and testing it (SVGLength parses by calling
> parseSingleValue with CSSPropertyX). It looks like the svg properties only use
> consumeLengthOrPercent, so we don't need to reject calc everywhere else.

(reject stuff for SVGAttributeMode that is)

[rwlbuis, 2015-11-21 02:12:48]

On 2015/11/20 00:07:14, Timothy Loh wrote:
> On 2015/11/20 00:06:57, Timothy Loh wrote:
> > Can we update consumeLength to reject vw/vh/vmin/vmax and
> consumeLengthOrPercent
> > to reject calcs for now? We shouldn't be adding functionality without
actually
> > making sure it works and testing it (SVGLength parses by calling
> > parseSingleValue with CSSPropertyX). It looks like the svg properties only
use
> > consumeLengthOrPercent, so we don't need to reject calc everywhere else.
> 
> (reject stuff for SVGAttributeMode that is)

Done :)

[Timothy Loh, 2015-11-23 00:22:05]

lgtm

[rwlbuis, 2015-11-23 03:17:56]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-11-23 03:18:02]

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

[commit-bot: I haz the power, 2015-11-23 04:39:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-23 04:39:48]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-11-23 04:40:13]

Patchset 5 (id:??) landed as
https://crrev.com/22ae97662a0aa4f9f697ec97d61b89c958b9cd31
Cr-Commit-Position: refs/heads/master@{#361069}