SVG: Promote d to a property

SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821

BUG=535429
Committed: https://crrev.com/c0f950e6701367e996f94d379f36c2109adab8ee
Cr-Commit-Position: refs/heads/master@{#365163}

[Eric Willigers, 2015-11-26 00:12:08]

Patchset #3 (id:40001) has been deleted

[Eric Willigers, 2015-11-26 05:06:11]

Description was changed from

==========
[WIP] SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821

'd' is not currently listed in
https://svgwg.org/svg2-draft/single-page.html#styling-PresentationAttributes

BUG=535429
==========

to

==========
SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821


BUG=535429
==========

[Eric Willigers, 2015-11-26 05:06:11]

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

[fs, 2015-11-26 12:51:38]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2015-11-26 12:51:40]

I'm not pdr, but here are my thoughts...

First a rant about path representation... So we have:

  String     - full semantics, reasonably compact (depending greatly on the
path/precision)
  Path       - normalized, fairly expensive to build, we use it or layout and
paint
  ByteStream - full semantics, reasonably compact (rarely worse than String)

So we need String for computed style - if we don't have String we need a
ByteStream to be able to produce a String with "full fidelity".
Also, we want to avoid keeping multiple representations if we can.

So, storing a String has:
  pro: conversion to string is "free" (possibly, depends on if normalization is
required)
       fairly compact
       sharable
  con: re-parsing (expensive) needed to convert to other representation

Storing a ByteStream:
  pro: fairly compact
       no re-parsing (less expensive to use for building other representations)
  con: conversion to string has a cost
       not sharable (can be fixed with the cost of a ref-count, so
"non-trivially sharable")

</rant>

And, related to the above, maybe we should consider keeping a CSSPathValue (or
whatever we end up with...) in SVGPath, essentially transferring the
representation from the latter to the former. Then the
collectPresentationAttributeStyle step will be cheaper (and it might be possible
to share data even more [thinking a bit about the correspondingElement bits].)

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

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1950: static
PassRefPtrWillBeRawPtr<CSSValue> consumeSVGDPath(CSSParserTokenRange& range)
Shouldn't the syntax for this be the same as for motion-path - i.e
path(<string>) ?

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1957: if
(!buildPathFromString(pathString, path) || !range.atEnd())
Here we build a Path (not entirely free) just to throw it away. If we just want
to check the syntax of the path it should be possible to add a new
SVGPathUtilities function for that. Let me know and I can add one (I see this is
used for motion-path too.)

This does mean that the semantics for 'd' changes though:

<path d="M100,100h100v100h-100z ballongdansen"/>

should produce a rectangle@(100,100) - it currently does w/ this patch because
we re-serialize to string after parsing the attribute - it'd be good to not rely
on an intermediate step "cleaning up" the path data though. I guess whether
parsing as an presentation attribute and via a stylesheet/inline style should be
treated differently is a different question though.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:882: void
StyleBuilderFunctions::applyValueCSSPropertyD(StyleResolverState& state,
CSSValue* value)
Feels like it should be possible to just have a converter for paths and use that
(should apply to motion-path too I think, but maybe the 'none' makes that
troublesome?)

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/DataEquivalency.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/DataEquivalency.h:8: #include
"platform/heap/Handle.h"
Hmm?

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyle.h:93: static const
SVGPathByteStream* initialD() { return nullptr; }
It feels like having a "template" - empty path object here would make using code
simpler. Having an empty path should be consistent with "missing value default".
This goes together with the call for a slightly more complex representation.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h:293:
OwnPtr<SVGPathByteStream> d;
Why not something similar to StyleMotionPath here?

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:84: if (const
ComputedStyle* style = static_cast<const Element*>(this)->computedStyle()) {
Per below, this code should probably move to asPath (and just using
layoutObject()->style() and similar would probably make sense - I don't think
computedStyle buys us anything here. We assume that layoutObject() is non-null
here.) Also reinstate the correspondingElement() bit - that bit might require
more care about things being null.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:90: return
m_path->currentValue()->byteStream();
Should only be using SVGComputedStyle here. No access to attributes.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:95:
static_cast<Element*>(this)->ensureComputedStyle();
I don't think these methods should need to care about the computed style, and
just operate on the path from the attribute. This is not specified (AFAIK), but
it's consistent with other presentation attributes , like for instance
'transform'.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:140:
markForLayoutAndParentResourceInvalidation(layoutObject());
Missing InvalidationGuard - or drop this and don't return.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:145: if (attrName ==
SVGNames::dAttr || attrName == SVGNames::pathLengthAttr) {
Checking for dAttr here is "dead", but depends on how the above is resolved.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:159:
addPropertyToPresentationAttributeStyle(style, CSSPropertyD,
m_path->currentValue()->valueAsString());
This will make for a fairly inefficient round-trip:

m_path (SVGPathByteStream) -> String -> SVGPathByteStream -> ...

but more on that elsewhere...

[pdr., 2015-12-02 05:36:50]

@fs, I originally thought a string would be optimal (bytestream doesn't actually
save much space) but seeing this approach made me realize the bytestream
approach could work better as long as it's shareable. Is that the approach you'd
like too? I wasn't sure of the rant's punchline.

To make sure we're on the same page, this would mean something like a
CSSPathValue which contains a bytestream, and a SVGPath with a refcounted
CSSPathValue member.

We don't need to explicitly keep around invalid css properties and invalid
attributes are stored for us. Therefore, a string representation like "M0 0
L100duintesmällaballongerna" can be converted to the byte stream "M0 0"
internally for free; "no strings attached".


Offline, Eric pointed out that there's a remaining issue with the empty path
that probably needs spec work (path('') vs None).

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:95:
static_cast<Element*>(this)->ensureComputedStyle();
On 2015/11/26 at 12:51:40, fs wrote:
> I don't think these methods should need to care about the computed style, and
just operate on the path from the attribute. This is not specified (AFAIK), but
it's consistent with other presentation attributes , like for instance
'transform'.

I'm not quite sure what you mean about the computed style--do you just mean we
should be using asPath?

I made a test of this: http://output.jsbin.com/nuqutejebu showing how
getTotalLength()/d compares to getBBox()/font-size. (This test also shows we
have a bug but I think switching to asPath will fix that right up.)

[fs, 2015-12-02 09:57:56]

On 2015/12/02 at 05:36:50, pdr wrote:
> @fs, I originally thought a string would be optimal (bytestream doesn't
actually save much space) but seeing this approach made me realize the
bytestream approach could work better as long as it's shareable. Is that the
approach you'd like too?

While I'd like to only store the string and then lazily parse that into a Path
[1], when I start thinking about it I always end up with the string being
reparsed more than I'd want... So I do lean towards a shareable bytestream
approach, yes.

> I wasn't sure of the rant's punchline.

Neither was I once I'd written it TBH... something about "information helps
decision-making" I think.
 
> To make sure we're on the same page, this would mean something like a
CSSPathValue which contains a bytestream, and a SVGPath with a refcounted
CSSPathValue member.

Yepp.

> We don't need to explicitly keep around invalid css properties and invalid
attributes are stored for us. Therefore, a string representation like "M0 0
L100duintesmällaballongerna" can be converted to the byte stream "M0 0"
internally for free; "no strings attached".

I've heard that there are no free lunches, but yes. I guess that will even be
required if the computed value string (of the above example) should be "path('M0
0')". I wouldn't want to optimize for the computed value string representation
of course... (And, yes, I've left the balloons alone - if you heard a bang, it
wasn't me ;-))
 
> Offline, Eric pointed out that there's a remaining issue with the empty path
that probably needs spec work (path('') vs None).

I guess it's more a question of whether 'none' is allowed and if it should
remain as computed value? For all other cases it ought to be the same as <path
d=""/> vs. <path/> (which render the same: nothing.)

>
https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):
> 
>
https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/SVGPathElement.cpp:95:
static_cast<Element*>(this)->ensureComputedStyle();
> On 2015/11/26 at 12:51:40, fs wrote:
> > I don't think these methods should need to care about the computed style,
and just operate on the path from the attribute. This is not specified (AFAIK),
but it's consistent with other presentation attributes , like for instance
'transform'.
> 
> I'm not quite sure what you mean about the computed style--do you just mean we
should be using asPath?
> 
> I made a test of this: http://output.jsbin.com/nuqutejebu showing how
getTotalLength()/d compares to getBBox()/font-size. (This test also shows we
have a bug but I think switching to asPath will fix that right up.)

I was thinking about [2] when I wrote this, but I realize now that it may not be
an accurate analogue. I suppose we should just do document().updateLayout... and
get hold of the ComputedStyle path data (if attached) or the attribute (or do
you think we should use Element::ensureComputedStyle?).

[1] I've been toying with this in the context of SVGPath - it should save on
load time/memory use, but on the other hand has other "downsides" (no [sync]
error reporting, possibly need to produce a normalized string repr., ...)
[2] http://www.w3.org/2015/06/12-svg-minutes.html#item09

[Eric Willigers, 2015-12-10 23:51:29]

Patchset #8 (id:160001) has been deleted

[Eric Willigers, 2015-12-10 23:51:51]

Patchset #7 (id:140001) has been deleted

[Eric Willigers, 2015-12-10 23:52:53]

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

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1950: static
PassRefPtrWillBeRawPtr<CSSValue> consumeSVGDPath(CSSParserTokenRange& range)
On 2015/11/26 12:51:39, fs wrote:
> Shouldn't the syntax for this be the same as for motion-path - i.e
> path(<string>) ?

Done. Should we have None or path('') if the element has no path attribute?

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/DataEquivalency.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/DataEquivalency.h:8: #include
"platform/heap/Handle.h"
On 2015/11/26 12:51:39, fs wrote:
> Hmm?

The header uses Member and Persistent but doesn't include their definitions.

I can leave this out by not using DataEquivalency.

CSSValue classes don't define operator== anyway.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyle.h:93: static const
SVGPathByteStream* initialD() { return nullptr; }
On 2015/11/26 12:51:40, fs wrote:
> It feels like having a "template" - empty path object here would make using
code
> simpler. Having an empty path should be consistent with "missing value
default".
> This goes together with the call for a slightly more complex representation.

I like the template empty path object, but who should own it? When we have
Oilpan, we don't want the empty path object to be garbage collected.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h:293:
OwnPtr<SVGPathByteStream> d;
On 2015/11/26 12:51:40, fs wrote:
> Why not something similar to StyleMotionPath here?

For motion path, we didn't need to animate the path, so Path was a sufficient
representation. The inheritance from StyleMotionPath was to prepare for
"<url> | [ <basic-shape> | <path()> ] || <geometry-box> | none"

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:84: if (const
ComputedStyle* style = static_cast<const Element*>(this)->computedStyle()) {
On 2015/11/26 12:51:40, fs wrote:
> Per below, this code should probably move to asPath (and just using
> layoutObject()->style() and similar would probably make sense - I don't think
> computedStyle buys us anything here. We assume that layoutObject() is non-null
> here.) Also reinstate the correspondingElement() bit - that bit might require
> more care about things being null.

A complication with the old correspondingElement() approach is that we should
use the CSS properties of the use element. For example, the new test
svg/css/inherit-d.svg fails when I reinstate that code.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:140:
markForLayoutAndParentResourceInvalidation(layoutObject());
On 2015/11/26 12:51:40, fs wrote:
> Missing InvalidationGuard - or drop this and don't return.

InvalidationGuard is at line 129.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:145: if (attrName ==
SVGNames::dAttr || attrName == SVGNames::pathLengthAttr) {
On 2015/11/26 12:51:40, fs wrote:
> Checking for dAttr here is "dead", but depends on how the above is resolved.

Acknowledged.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:159:
addPropertyToPresentationAttributeStyle(style, CSSPropertyD,
m_path->currentValue()->valueAsString());
On 2015/11/26 12:51:40, fs wrote:
> This will make for a fairly inefficient round-trip:
> 
> m_path (SVGPathByteStream) -> String -> SVGPathByteStream -> ...
> 
> but more on that elsewhere...

Changed to use CSSPathValue

[Eric Willigers, 2015-12-11 03:24:59]

Patchset #8 (id:200001) has been deleted

[fs, 2015-12-11 13:01:58]

This looks pretty good. With some things addressed, we could probably iterate
from here (given that perf isn't hurt in "stable" configuration.)

Somewhat sprinkled below I've been trying to make a case for a
Style<something?>Path - just to make it clear up-front - that's one of the
things I'd be happy to iterate on (even if CSSValues in ComputedStyle feels a
bit atypical.)

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

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1950: static
PassRefPtrWillBeRawPtr<CSSValue> consumeSVGDPath(CSSParserTokenRange& range)
On 2015/12/10 at 23:52:52, Eric Willigers wrote:
> On 2015/11/26 12:51:39, fs wrote:
> > Shouldn't the syntax for this be the same as for motion-path - i.e
> > path(<string>) ?
> 
> Done. Should we have None or path('') if the element has no path attribute?

The way you've done it in the latest PS seems ok to me. As I mentioned earlier -
they are semantically equivalent. I'd think that "path('')" should not turn into
"none" (and probably vice versa) if they both existed. No realy opinion if any
form is better though...

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1957: if
(!buildPathFromString(pathString, path) || !range.atEnd())
On 2015/11/26 at 12:51:39, fs wrote:
> Here we build a Path (not entirely free) just to throw it away. If we just
want to check the syntax of the path it should be possible to add a new
SVGPathUtilities function for that. Let me know and I can add one (I see this is
used for motion-path too.)
> 
> This does mean that the semantics for 'd' changes though:
> 
> <path d="M100,100h100v100h-100z ballongdansen"/>
> 
> should produce a rectangle@(100,100) - it currently does w/ this patch because
we re-serialize to string after parsing the attribute - it'd be good to not rely
on an intermediate step "cleaning up" the path data though. I guess whether
parsing as an presentation attribute and via a stylesheet/inline style should be
treated differently is a different question though.

Just noting: We should probably make sure this is addressed (spec-wise) before
we ship this.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyle.h:93: static const
SVGPathByteStream* initialD() { return nullptr; }
On 2015/12/10 at 23:52:52, Eric Willigers wrote:
> On 2015/11/26 12:51:40, fs wrote:
> > It feels like having a "template" - empty path object here would make using
code
> > simpler. Having an empty path should be consistent with "missing value
default".
> > This goes together with the call for a slightly more complex representation.
> 
> I like the template empty path object, but who should own it? When we have
Oilpan, we don't want the empty path object to be garbage collected.

It looks like you handled this in the way at least I would expect. More
(somewhat) related to this elsewhere though.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h:293:
OwnPtr<SVGPathByteStream> d;
On 2015/12/10 at 23:52:52, Eric Willigers wrote:
> On 2015/11/26 12:51:40, fs wrote:
> > Why not something similar to StyleMotionPath here?
> 
> For motion path, we didn't need to animate the path, so Path was a sufficient
representation. The inheritance from StyleMotionPath was to prepare for
> "<url> | [ <basic-shape> | <path()> ] || <geometry-box> | none"

I don't think it would be too far-fetched to expect "d: <basic-shape> |
<path()>" (or <shape> or whatever production that might be). FWIW, ISTR that
having happened for some <shape> consuming properties already (clip-path). So I
think gravitating towards a Style<something?>Path for use in such cases may be
good. We can iterate on the exact representational requirements (<Path, String>
vs. <Path, SVGPathByteStream> etc) though I think as long as we make sure to not
hurt "stable" performance.

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:84: if (const
ComputedStyle* style = static_cast<const Element*>(this)->computedStyle()) {
On 2015/12/10 at 23:52:52, Eric Willigers wrote:
> On 2015/11/26 12:51:40, fs wrote:
> > Per below, this code should probably move to asPath (and just using
> > layoutObject()->style() and similar would probably make sense - I don't
think
> > computedStyle buys us anything here. We assume that layoutObject() is
non-null
> > here.) Also reinstate the correspondingElement() bit - that bit might
require
> > more care about things being null.
> 
> A complication with the old correspondingElement() approach is that we should
use the CSS properties of the use element. For example, the new test
svg/css/inherit-d.svg fails when I reinstate that code.

I think it would still be beneficial (and equivalent to the existing code) to
share the value for the presentation attribute style. With a
Style<something?>Path and a cached Path somewhere in the chain (maybe even a
cached Style<something?>Path in CSSPathValue...). The cached (or rather: shared)
Path is what will help us down-stream in Skia since it'll enable re-use of
tesselations for GPU rasterization - in case you're wondering why I'm assuming
such an adamant stance on this issue =). Well, that, memory use and avoiding
re-calculation of the path really... (but they are somewhat connected too)

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:140:
markForLayoutAndParentResourceInvalidation(layoutObject());
On 2015/12/10 at 23:52:52, Eric Willigers wrote:
> On 2015/11/26 12:51:40, fs wrote:
> > Missing InvalidationGuard - or drop this and don't return.
> 
> InvalidationGuard is at line 129.

*goes to check eyesight*

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:173: d
runtime_flag=CSSPropertyD, svg, custom_all
Looks like we could make this converted=convertPath or something - or at least
custom_value?

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

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1931: if (id ==
CSSValueNone && property == CSSPropertyMotionPath)
Suggestion: Could also do the trivial split - consumePath & consumePathOrNone -
at least until it's decided whether 'none' should apply to 'd' or not (and how
that in turn interacts w/ computed value.) I'll defer this to timloh & co
though.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyle.h:121: layout.access()->d
= d;
I think there might still be some use in doing "if !=" dance here. Even just
comparing the pointers.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:51: static
PassRefPtrWillBeRawPtr<SVGPath> create(PassRefPtrWillBeRawPtr<CSSPathValue> d)
Nit: I think we should refrain from using "d" in the context of this object
(SVGPath). maybe 'pathValue' instead?

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:75: explicit
SVGPath(PassRefPtrWillBeRawPtr<CSSPathValue> d);
Nit: Can drop 'd' here (it doesn't really add anything useful IMHO)

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:77:
RefPtrWillBePersistent<CSSPathValue> m_pathValue;
You should make this a RefPtrWillBeMember<...> instead and
DECLARE_TRACE/DEFINE_TRACE (SVGPropertyBase inherits
RefCountedWillBeGarbageCollectedFinalized)

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathByteStream.h (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathByteStream.h:35: class
SVGPathByteStream final {
Redundant final.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:24: #include
"core/dom/NodeComputedStyle.h"
No longer needed?

[Eric Willigers, 2015-12-14 05:34:33]

Patchset #9 (id:240001) has been deleted

[Eric Willigers, 2015-12-14 05:35:00]

Patchset #2 (id:20001) has been deleted

[Eric Willigers, 2015-12-14 05:35:57]

Patchset #1 (id:1) has been deleted

[Eric Willigers, 2015-12-14 05:36:46]

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

https://codereview.chromium.org/1439793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1957: if
(!buildPathFromString(pathString, path) || !range.atEnd())
On 2015/12/11 13:01:58, fs wrote:
> On 2015/11/26 at 12:51:39, fs wrote:
> > Here we build a Path (not entirely free) just to throw it away. If we just
> want to check the syntax of the path it should be possible to add a new
> SVGPathUtilities function for that. Let me know and I can add one (I see this
is
> used for motion-path too.)
> > 
> > This does mean that the semantics for 'd' changes though:
> > 
> > <path d="M100,100h100v100h-100z ballongdansen"/>
> > 
> > should produce a rectangle@(100,100) - it currently does w/ this patch
because
> we re-serialize to string after parsing the attribute - it'd be good to not
rely
> on an intermediate step "cleaning up" the path data though. I guess whether
> parsing as an presentation attribute and via a stylesheet/inline style should
be
> treated differently is a different question though.
> 
> Just noting: We should probably make sure this is addressed (spec-wise) before
> we ship this.

The implemented approach is conservative, and consistent with other CSS
properties.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:173: d
runtime_flag=CSSPropertyD, svg, custom_all
On 2015/12/11 13:01:58, fs wrote:
> Looks like we could make this converted=convertPath or something - or at least
> custom_value?

Done.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/SVGComputedStyle.h (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/SVGComputedStyle.h:121: layout.access()->d
= d;
On 2015/12/11 13:01:58, fs wrote:
> I think there might still be some use in doing "if !=" dance here. Even just
> comparing the pointers.

Done.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:51: static
PassRefPtrWillBeRawPtr<SVGPath> create(PassRefPtrWillBeRawPtr<CSSPathValue> d)
On 2015/12/11 13:01:58, fs wrote:
> Nit: I think we should refrain from using "d" in the context of this object
> (SVGPath). maybe 'pathValue' instead?

Done.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:75: explicit
SVGPath(PassRefPtrWillBeRawPtr<CSSPathValue> d);
On 2015/12/11 13:01:58, fs wrote:
> Nit: Can drop 'd' here (it doesn't really add anything useful IMHO)

Done.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:77:
RefPtrWillBePersistent<CSSPathValue> m_pathValue;
On 2015/12/11 13:01:58, fs wrote:
> You should make this a RefPtrWillBeMember<...> instead and
> DECLARE_TRACE/DEFINE_TRACE (SVGPropertyBase inherits
> RefCountedWillBeGarbageCollectedFinalized)

Done.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathByteStream.h (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathByteStream.h:35: class
SVGPathByteStream final {
On 2015/12/11 13:01:58, fs wrote:
> Redundant final.

Acknowledged.

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:24: #include
"core/dom/NodeComputedStyle.h"
On 2015/12/11 13:01:58, fs wrote:
> No longer needed?

Acknowledged.

[fs, 2015-12-14 11:51:40]

This LGTM (w/ some additional nits) for a first iteration. I'm a tad worried
that creating the Path object up-front will regress some of the loading tests,
but I guess we can wait and see...

pdr?

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPathValue.cpp (right):

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPathValue.cpp:24:
buildPathFromByteStream(*m_pathByteStream, m_path);
I think we should strive to make this realization lazy.

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:73: DECLARE_TRACE();
DECLARE_VIRTUAL_TRACE I think, but I'm not 100% sure, because I think these are
not traced via their base-class... but maybe they are sometimes, better safe
than sorry I guess.

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:79: 
Nit: Remove blank line.

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:90: 
Ditto.

[pdr., 2015-12-14 19:12:06]

On 2015/12/14 at 11:51:40, fs wrote:
> This LGTM (w/ some additional nits) for a first iteration. I'm a tad worried
that creating the Path object up-front will regress some of the loading tests,
but I guess we can wait and see...
> 
> pdr?
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/CSSPathValue.cpp (right):
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/CSSPathValue.cpp:24:
buildPathFromByteStream(*m_pathByteStream, m_path);
> I think we should strive to make this realization lazy.
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/svg/SVGPath.h (right):
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/SVGPath.h:73: DECLARE_TRACE();
> DECLARE_VIRTUAL_TRACE I think, but I'm not 100% sure, because I think these
are not traced via their base-class... but maybe they are sometimes, better safe
than sorry I guess.
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/SVGPathElement.cpp:79: 
> Nit: Remove blank line.
> 
>
https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/SVGPathElement.cpp:90: 
> Ditto.


LGTM2!

[Eric Willigers, 2015-12-14 22:18:23]

Lots of thanks.

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPath.h:73: DECLARE_TRACE();
On 2015/12/14 11:51:40, fs wrote:
> DECLARE_VIRTUAL_TRACE I think, but I'm not 100% sure, because I think these
are
> not traced via their base-class... but maybe they are sometimes, better safe
> than sorry I guess.

Done.

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:79: 
On 2015/12/14 11:51:40, fs wrote:
> Nit: Remove blank line.

Done.

https://codereview.chromium.org/1439793003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGPathElement.cpp:90: 
On 2015/12/14 11:51:40, fs wrote:
> Ditto.

Done.

[Eric Willigers, 2015-12-14 22:18:29]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2015-12-14 22:18:29]

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/1439793003/#ps300001
(title: "DECLARE_VIRTUAL_TRACE")

[commit-bot: I haz the power, 2015-12-14 22:21:05]

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

[Eric Willigers, 2015-12-15 02:13:25]

ericwilligers@chromium.org changed reviewers:
+ haraken@chromium.org

[Eric Willigers, 2015-12-15 02:13:25]

+haraken for platform owners

[haraken, 2015-12-15 02:21:30]

platform LGTM

[Ilya Sherman, 2015-12-15 02:29:14]

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

[Ilya Sherman, 2015-12-15 02:29:15]

histograms.xml lgtm, though...

https://codereview.chromium.org/1439793003/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1439793003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:69328: +  <int value="518" label="d"/>
I am hard pressed to think of a more obscure property name :(

[pdr., 2015-12-15 04:17:03]

On 2015/12/15 at 02:29:15, isherman wrote:
> histograms.xml lgtm, though...
> 
>
https://codereview.chromium.org/1439793003/diff/300001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1439793003/diff/300001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:69328: +  <int value="518" label="d"/>
> I am hard pressed to think of a more obscure property name :(

You may feel better or worse once you find out why :)

The property name 'd' and the path syntax consisting of one-letter path commands
were chosen for... filesize. It was a different time... xml was hot and
network-level compression was not.

[commit-bot: I haz the power, 2015-12-15 04:26:35]

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

[commit-bot: I haz the power, 2015-12-15 04:26:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Eric Willigers, 2015-12-15 05:21:21]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2015-12-15 05:22:55]

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

[commit-bot: I haz the power, 2015-12-15 05:29:47]

Description was changed from

==========
SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821


BUG=535429
==========

to

==========
SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821


BUG=535429
==========

[commit-bot: I haz the power, 2015-12-15 05:29:49]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2015-12-15 05:30:39]

Description was changed from

==========
SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821


BUG=535429
==========

to

==========
SVG: Promote d to a property

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821

BUG=535429

Committed: https://crrev.com/c0f950e6701367e996f94d379f36c2109adab8ee
Cr-Commit-Position: refs/heads/master@{#365163}
==========

[commit-bot: I haz the power, 2015-12-15 05:30:41]

Patchset 9 (id:??) landed as
https://crrev.com/c0f950e6701367e996f94d379f36c2109adab8ee
Cr-Commit-Position: refs/heads/master@{#365163}