Move remaining SVG properties into CSSPropertyParser

Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units. Also support for percentages
in the opacity properties is removed as this
is not specified anywhere in the SVG specs.

BUG=499780
Committed: https://crrev.com/53c8da9fe520c90bc1ae325dd4e0513b7851a56c
Cr-Commit-Position: refs/heads/master@{#362311}

[rwlbuis, 2015-11-18 03:28:32]

Patchset #4 (id:60001) has been deleted

[rwlbuis, 2015-11-30 02:04:03]

Description was changed from

==========
Various SVG WIP

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[rwlbuis, 2015-11-30 02:05:00]

Description was changed from

==========
Move remaining SVG properties into CSSPropertyParser

Move stroke-dasharray property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

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

[rwlbuis, 2015-11-30 02:05:17]

Description was changed from

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[rwlbuis, 2015-11-30 02:05:52]

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

[rwlbuis, 2015-11-30 02:05:52]

PTAL.

[Timothy Loh, 2015-11-30 06:09:56]

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

[Timothy Loh, 2015-11-30 06:09:57]

Description should mention the web-facing changes.

lgtm, but wait for fs to have a look.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1955: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeOpacity(CSSParserTokenRange&
range)
consumeSVGOpacity since opacity isn't going to call into here.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1959: return
opacity;
opacity.release();

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1968: return
consumeLengthOrPercent(range, SVGAttributeMode, ValueRangeAll,
UnitlessQuirk::Allow);
Should be UnitlessQuirk::Forbid. SVGAttributeMode handles the
unitless->UserUnits case, while UnitLessQuirk::Allow should be only for the
actual unitless length quirk as per spec.

https://quirks.spec.whatwg.org/#the-unitless-length-quirk

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2178: return
consumeLengthOrPercent(m_range, SVGAttributeMode, ValueRangeAll,
UnitlessQuirk::Allow);
::Forbid

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1129: //
in either this switch statement or the one in CSSPropertyParser::parseValue
probably should update this to.. ::parseSingleValue I guess?

[fs, 2015-11-30 09:52:59]

LGTM w/ Tim's issues addressed.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1955: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeOpacity(CSSParserTokenRange&
range)
On 2015/11/30 at 06:09:57, Timothy Loh wrote:
> consumeSVGOpacity since opacity isn't going to call into here.

They really should be the same though (according to new and old spec AFAICS).
Maybe something to add a use-counter for and try to fix...

[rwlbuis, 2015-11-30 18:02:42]

Description was changed from

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units.

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

[rwlbuis, 2015-11-30 22:17:43]

PTAL again please because of the opacity changes. I also added a testcase for
this.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1955: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeOpacity(CSSParserTokenRange&
range)
On 2015/11/30 09:52:59, fs wrote:
> On 2015/11/30 at 06:09:57, Timothy Loh wrote:
> > consumeSVGOpacity since opacity isn't going to call into here.
> 
> They really should be the same though (according to new and old spec AFAICS).
> Maybe something to add a use-counter for and try to fix...

The plot thickens. The only difference I saw was that stroke-opacity etc.
accepted percentages. But I don't see anything about this in the specs! So I
removed support for this in the latest patch, then we can just call
consumeNumber, and it is really the same as opacity.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1959: return
opacity;
On 2015/11/30 06:09:57, Timothy Loh wrote:
> opacity.release();

Correct, however we may not need it.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1968: return
consumeLengthOrPercent(range, SVGAttributeMode, ValueRangeAll,
UnitlessQuirk::Allow);
On 2015/11/30 06:09:57, Timothy Loh wrote:
> Should be UnitlessQuirk::Forbid. SVGAttributeMode handles the
> unitless->UserUnits case, while UnitLessQuirk::Allow should be only for the
> actual unitless length quirk as per spec.
> 
> https://quirks.spec.whatwg.org/#the-unitless-length-quirk

Done.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2178: return
consumeLengthOrPercent(m_range, SVGAttributeMode, ValueRangeAll,
UnitlessQuirk::Allow);
On 2015/11/30 06:09:57, Timothy Loh wrote:
> ::Forbid

Done.

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1129: //
in either this switch statement or the one in CSSPropertyParser::parseValue
On 2015/11/30 06:09:57, Timothy Loh wrote:
> probably should update this to.. ::parseSingleValue I guess?

Done.

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

Description was changed from

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units.

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units. Also support for percentages
in the opacity properties is removed as this
is not specified anywhere in the SVG specs.

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

[fs, 2015-11-30 23:10:31]

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1955: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeOpacity(CSSParserTokenRange&
range)
On 2015/11/30 at 22:17:43, rwlbuis wrote:
> On 2015/11/30 09:52:59, fs wrote:
> > On 2015/11/30 at 06:09:57, Timothy Loh wrote:
> > > consumeSVGOpacity since opacity isn't going to call into here.
> > 
> > They really should be the same though (according to new and old spec
AFAICS).
> > Maybe something to add a use-counter for and try to fix...
> 
> The plot thickens. The only difference I saw was that stroke-opacity etc.
accepted percentages. But I don't see anything about this in the specs! So I
removed support for this in the latest patch, then we can just call
consumeNumber, and it is really the same as opacity.

SGTM to remove if <other browser> passes the new test.

[rwlbuis, 2015-11-30 23:21:58]

On 2015/11/30 23:10:31, fs wrote:
>
https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1441233006/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1955: static
> PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeOpacity(CSSParserTokenRange&
> range)
> On 2015/11/30 at 22:17:43, rwlbuis wrote:
> > On 2015/11/30 09:52:59, fs wrote:
> > > On 2015/11/30 at 06:09:57, Timothy Loh wrote:
> > > > consumeSVGOpacity since opacity isn't going to call into here.
> > > 
> > > They really should be the same though (according to new and old spec
> AFAICS).
> > > Maybe something to add a use-counter for and try to fix...
> > 
> > The plot thickens. The only difference I saw was that stroke-opacity etc.
> accepted percentages. But I don't see anything about this in the specs! So I
> removed support for this in the latest patch, then we can just call
> consumeNumber, and it is really the same as opacity.
> 
> SGTM to remove if <other browser> passes the new test.

Sorry, should have mentioned Firefox indeed passes
svg/css/opacity-not-supporting-percentage.html :)

[Timothy Loh, 2015-11-30 23:43:45]

lgtm

[rwlbuis, 2015-11-30 23:49:12]

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

[rwlbuis, 2015-11-30 23:49:12]

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/1441233006/#ps140001
(title: "Add tests for opacity+percentage combination")

[commit-bot: I haz the power, 2015-11-30 23:50:45]

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

[commit-bot: I haz the power, 2015-12-01 01:42:59]

Description was changed from

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units. Also support for percentages
in the opacity properties is removed as this
is not specified anywhere in the SVG specs.

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units. Also support for percentages
in the opacity properties is removed as this
is not specified anywhere in the SVG specs.

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

[commit-bot: I haz the power, 2015-12-01 01:43:01]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-12-01 01:43:43]

Description was changed from

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units. Also support for percentages
in the opacity properties is removed as this
is not specified anywhere in the SVG specs.

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

to

==========
Move remaining SVG properties into CSSPropertyParser

Move remaining SVG property handling from
LegacyCSSPropertyParser into CSSPropertyParser.

With this change, the properties baseline-shift,
stroke-width and stroke-dashoffset will show
computed style without pixel units when set
using user units. Also support for percentages
in the opacity properties is removed as this
is not specified anywhere in the SVG specs.

BUG=499780

Committed: https://crrev.com/53c8da9fe520c90bc1ae325dd4e0513b7851a56c
Cr-Commit-Position: refs/heads/master@{#362311}
==========

[commit-bot: I haz the power, 2015-12-01 01:43:44]

Patchset 7 (id:??) landed as
https://crrev.com/53c8da9fe520c90bc1ae325dd4e0513b7851a56c
Cr-Commit-Position: refs/heads/master@{#362311}