[svg2] Make 'x' and 'y' presentation attributes

[svg2] Make 'x' and 'y' presentation attributes

The following elements' x and y attributes have been made
into presentation attributes:

* foreignObject
* image
* mask
* pattern
* rect
* svg
* use

The cursor, filter, all the filter primitive elements, text,
tspan and textPath elements have been excluded for now.

This is a partial merge of Dirk Schulze's WebKit patch
http://trac.webkit.org/changeset/171591.

This patch avoids reparsing the attribute values when they
are added to the presentation style, which was one of the
leading reasons for the slowness seen in bug 369942.

BUG=400725
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190180

[Erik Dahlström (inactive), 2015-02-04 11:39:57]

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

[Erik Dahlström (inactive), 2015-02-04 12:12:59]

Perf tryruns on this CL showed no significant regressions, though the results
vary a bit (have seen both 1-2% faster and 1-2% slower overall on the
blink_perf.svg tests).

I'd like to also move towards using the CSS length parser, and while this CL
doesn't go quite that far it's a step in that direction.

[fs, 2015-02-04 12:17:05]

On 2015/02/04 12:12:59, Erik Dahlström wrote:
> Perf tryruns on this CL showed no significant regressions, though the results
> vary a bit (have seen both 1-2% faster and 1-2% slower overall on the
> blink_perf.svg tests).

I'm sure pdr would appreciate a link to the results, so he can gauge that part
for himself =)

> I'd like to also move towards using the CSS length parser, and while this CL
> doesn't go quite that far it's a step in that direction.

Do we have a bug reported for that?

[fs, 2015-02-04 13:24:53]

> [svg2] Make x and y presentation attributes (part #1).

What's "part 2" =) - or is that "Make 'foo' and 'bar' presentation attributes"?

Also, please drop the full stop there and line-break the description at a
sensible column.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:1376: void setX(Length x) {
accessSVGStyle().setX(x); }
I've seen the setters used, but not the getters... Personally I'd prefer to not
have these forwarders at all, but the setters seem to be actually used (by
anim-code). Could we drop the getters?

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/SVGRenderStyleDefs.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/SVGRenderStyleDefs.cpp:31: #include
"core/rendering/style/RenderStyle.h"
What's this needed for? (If it's needed, it should have a blank line before it -
drop the blank line after config.h when at it.)

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/SVGRenderStyleDefs.cpp:205: :
x(SVGRenderStyle::initialX())
Fix indentation.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGPathData.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGPathData.cpp:22: #include
"core/rendering/RenderObject.h"
These should go the list/block of includes below.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGPathData.cpp:108: if (!renderer)
For <rect>s, I believe there's only one way to reach this code, and that's from
within the RenderObject itself (RenderSVGRect/Shape), so it seems an ASSERT
should suffice here.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimationElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimationElement.cpp:371: if
(!targetElement->isPresentationAttribute(attributeName))
<to be written>

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:91: if (property == m_width)
Maybe we could split this part out and land separately? I.e. use the "new"
approach, which should allow this to be:

if (property == m_width)
    addSVGLengthPropertyToPresentationAttributeStyle(..., CSSPropertyWidth,
m_width);
else if (property == m_height)
    addSVGLengthPropertyToPresentationAtt...

IIRC the the original structure with parsing was to avoid "injecting" keywords
('auto' and 'inherit' I guess), but that shouldn't be the case when using the
SVGLength - right?

(see comment about this new helper method below.)

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:97:
addPropertyToPresentationAttributeStyle(style, CSSPropertyX,
m_x->currentValue()->valueInSpecifiedUnits(),
m_x->currentValue()->cssUnitTypeQuirk());
Maybe turn this "pattern" into:

void addSVGLengthPropertyToPresentationAttributeStyle(MutableStylePropertySet*,
CSSPropertyID, SVGLength&)

(on SVGElement?)

then the quirks-handling would be contained in a single spot too hopefully.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:114: bool widthOrHeight = attrName
== SVGNames::widthAttr
Maybe do the something similar for 'x' and 'y' (and then maybe drop
isLengthAttribute)

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
Source/core/svg/SVGImageElement.cpp:130: if (attrName == SVGNames::xAttr
Not sure how the 'width' and 'height' values should weigh into this, but it
seems a bit weird that they weren't already...

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
File Source/core/svg/SVGImageElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
Source/core/svg/SVGImageElement.h:58: virtual bool isPresentationAttribute(const
QualifiedName&) const override final;
This class is final, so the final qualifiers are redundant.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:98: return
convertValueFromPercentageToUserUnits(length.value() / 100, mode,
IGNORE_EXCEPTION);
Move this after the 'auto' check, and then use the version of
convertValueFromPercentageToUserUnits that doesn't take an ExceptionState (but a
viewport-size).

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:98: return
convertValueFromPercentageToUserUnits(length.value() / 100, mode,
IGNORE_EXCEPTION);
I'm sure bratell would want you to do the division last ;-)

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:106: case LengthModeWidth:
(Given the amount of these switches this file/class now has, I wonder if it
wouldn't be nicer to factor these as:

static float factorForLengthMode(SVGLengthMode mode, const FloatSize&
viewportSize)
{
    switch (mode) {
    case LengthModeWidth:
        return viewportSize.width();
...
    }
}

...
return floatValueForLength(length, factorForLengthMode(...));

I'll see if I can whip something up for that...)

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGMaskE...
File Source/core/svg/SVGMaskElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGMaskE...
Source/core/svg/SVGMaskElement.h:58: virtual bool isPresentationAttribute(const
QualifiedName&) const override final;
Class is final.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGPatte...
File Source/core/svg/SVGPatternElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGPatte...
Source/core/svg/SVGPatternElement.h:73: virtual bool
isPresentationAttribute(const QualifiedName&) const override final;
final

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
File Source/core/svg/SVGRectElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
Source/core/svg/SVGRectElement.cpp:92: } else if (!property) {
Why the condition here?

(Also, could drop the {}'s in this if-cascade.)

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
File Source/core/svg/SVGRectElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
Source/core/svg/SVGRectElement.h:46: virtual bool isPresentationAttribute(const
QualifiedName&) const override;
final

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGUseEl...
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGUseEl...
Source/core/svg/SVGUseElement.h:59: virtual void
collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&,
MutableStylePropertySet*) override final;
final

[Erik Dahlström (inactive), 2015-02-04 13:54:23]

On 2015/02/04 12:17:05, fs wrote:
> On 2015/02/04 12:12:59, Erik Dahlström wrote:
> > Perf tryruns on this CL showed no significant regressions, though the
results
> > vary a bit (have seen both 1-2% faster and 1-2% slower overall on the
> > blink_perf.svg tests).
> 
> I'm sure pdr would appreciate a link to the results, so he can gauge that part
> for himself =)

Latest run (from this morning): https://codereview.chromium.org/898873002/ -
results:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-02...

> > I'd like to also move towards using the CSS length parser, and while this CL
> > doesn't go quite that far it's a step in that direction.
> 
> Do we have a bug reported for that?

Filed bug 455200.

[pdr., 2015-02-04 19:52:50]

On 2015/02/04 at 13:54:23, ed wrote:
> On 2015/02/04 12:17:05, fs wrote:
> > On 2015/02/04 12:12:59, Erik Dahlström wrote:
> > > Perf tryruns on this CL showed no significant regressions, though the
results
> > > vary a bit (have seen both 1-2% faster and 1-2% slower overall on the
> > > blink_perf.svg tests).
> > 
> > I'm sure pdr would appreciate a link to the results, so he can gauge that
part
> > for himself =)
> 
> Latest run (from this morning): https://codereview.chromium.org/898873002/ -
results:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-02...
> 
> > > I'd like to also move towards using the CSS length parser, and while this
CL
> > > doesn't go quite that far it's a step in that direction.
> > 
> > Do we have a bug reported for that?
> 
> Filed bug 455200.

Do you have a theory on why this might be faster?

Because this is a partial merge, could you add Dirk's name and a reference to
the webkit patch in the change description? This gives him some credit, and
helps us track regressions.

[Timothy Loh, 2015-02-05 03:26:30]

https://codereview.chromium.org/896773002/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/896773002/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:509:
result.setQuirk(primitiveValue->isQuirkValue());
No need for quirk values

[Erik Dahlström (inactive), 2015-02-05 09:29:16]

On 2015/02/04 19:52:50, pdr wrote:
> On 2015/02/04 at 13:54:23, ed wrote:
> > On 2015/02/04 12:17:05, fs wrote:
> > > On 2015/02/04 12:12:59, Erik Dahlström wrote:
> > > > Perf tryruns on this CL showed no significant regressions, though the
> results
> > > > vary a bit (have seen both 1-2% faster and 1-2% slower overall on the
> > > > blink_perf.svg tests).
> > > 
> > > I'm sure pdr would appreciate a link to the results, so he can gauge that
> part
> > > for himself =)
> > 
> > Latest run (from this morning): https://codereview.chromium.org/898873002/ -
> results:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-02...
> > 
> > > > I'd like to also move towards using the CSS length parser, and while
this
> CL
> > > > doesn't go quite that far it's a step in that direction.
> > > 
> > > Do we have a bug reported for that?
> > 
> > Filed bug 455200.
> 
> Do you have a theory on why this might be faster?

"this" meaning what? This patch or the CSS length parser? 

If the former: I believe this CL should be a pretty neutral change in terms of
performance.
If the latter: using the CSS parser is probably going to be a bit slower since
it does more work, but that's a price that will have to be paid sooner or later,
for the richer set of features. It should hopefully be possible to limit that
extra cost to only the users of such features (say e.g css calc() in length
values).

> Because this is a partial merge, could you add Dirk's name and a reference to
> the webkit patch in the change description? This gives him some credit, and
> helps us track regressions.

Done.

[fs, 2015-02-05 09:37:21]

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimationElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimationElement.cpp:371: if
(!targetElement->isPresentationAttribute(attributeName))
On 2015/02/04 13:24:53, fs wrote:
> <to be written>

Ehrm... And here I intended to write something like:

If this returns 'true', then it seems that the below would return 'false' for
the newly added 'x' and 'y'?

[Erik Dahlström (inactive), 2015-02-05 16:09:03]

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:1376: void setX(Length x) {
accessSVGStyle().setX(x); }
On 2015/02/04 13:24:52, fs wrote:
> I've seen the setters used, but not the getters... Personally I'd prefer to
not
> have these forwarders at all, but the setters seem to be actually used (by
> anim-code). Could we drop the getters?

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/SVGRenderStyleDefs.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/SVGRenderStyleDefs.cpp:31: #include
"core/rendering/style/RenderStyle.h"
On 2015/02/04 13:24:53, fs wrote:
> What's this needed for? (If it's needed, it should have a blank line before it
-
> drop the blank line after config.h when at it.)

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/SVGRenderStyleDefs.cpp:205: :
x(SVGRenderStyle::initialX())
On 2015/02/04 13:24:53, fs wrote:
> Fix indentation.

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGPathData.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGPathData.cpp:22: #include
"core/rendering/RenderObject.h"
On 2015/02/04 13:24:53, fs wrote:
> These should go the list/block of includes below.

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGPathData.cpp:108: if (!renderer)
On 2015/02/04 13:24:53, fs wrote:
> For <rect>s, I believe there's only one way to reach this code, and that's
from
> within the RenderObject itself (RenderSVGRect/Shape), so it seems an ASSERT
> should suffice here.

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimationElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimationElement.cpp:371: if
(!targetElement->isPresentationAttribute(attributeName))
On 2015/02/05 09:37:21, fs wrote:
> On 2015/02/04 13:24:53, fs wrote:
> > <to be written>
> 
> Ehrm... And here I intended to write something like:
> 
> If this returns 'true', then it seems that the below would return 'false' for
> the newly added 'x' and 'y'?

Argh, yes, and now I remember why the whole ApplyCSSAndXMLAnimation hack was
needed, will need to add that back again :(

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:91: if (property == m_width)
On 2015/02/04 13:24:53, fs wrote:
> Maybe we could split this part out and land separately? I.e. use the "new"
> approach, which should allow this to be:
> 
> if (property == m_width)
>     addSVGLengthPropertyToPresentationAttributeStyle(..., CSSPropertyWidth,
> m_width);
> else if (property == m_height)
>     addSVGLengthPropertyToPresentationAtt...
> 
> IIRC the the original structure with parsing was to avoid "injecting" keywords
> ('auto' and 'inherit' I guess), but that shouldn't be the case when using the
> SVGLength - right?

That shouldn't be the case no, but we should allow them in the future I think. A
move to parsing with CSS would provide that automatically.

Yes, I'd like to convert the width/height ones to follow the same pattern, but
wasn't sure if it should be done in this patch or not.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:97:
addPropertyToPresentationAttributeStyle(style, CSSPropertyX,
m_x->currentValue()->valueInSpecifiedUnits(),
m_x->currentValue()->cssUnitTypeQuirk());
On 2015/02/04 13:24:53, fs wrote:
> Maybe turn this "pattern" into:
> 
> void
addSVGLengthPropertyToPresentationAttributeStyle(MutableStylePropertySet*,
> CSSPropertyID, SVGLength&)
> 
> (on SVGElement?)
> 
> then the quirks-handling would be contained in a single spot too hopefully.

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
Source/core/svg/SVGImageElement.cpp:130: if (attrName == SVGNames::xAttr
On 2015/02/04 13:24:53, fs wrote:
> Not sure how the 'width' and 'height' values should weigh into this, but it
> seems a bit weird that they weren't already...

Agreed, that looks like a bug. Will fix.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
File Source/core/svg/SVGImageElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGImage...
Source/core/svg/SVGImageElement.h:58: virtual bool isPresentationAttribute(const
QualifiedName&) const override final;
On 2015/02/04 13:24:53, fs wrote:
> This class is final, so the final qualifiers are redundant.

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:98: return
convertValueFromPercentageToUserUnits(length.value() / 100, mode,
IGNORE_EXCEPTION);
On 2015/02/04 13:24:53, fs wrote:
> Move this after the 'auto' check, and then use the version of
> convertValueFromPercentageToUserUnits that doesn't take an ExceptionState (but
a
> viewport-size).

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:98: return
convertValueFromPercentageToUserUnits(length.value() / 100, mode,
IGNORE_EXCEPTION);
On 2015/02/04 13:24:53, fs wrote:
> I'm sure bratell would want you to do the division last ;-)

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:106: case LengthModeWidth:
On 2015/02/04 13:24:53, fs wrote:
> (Given the amount of these switches this file/class now has, I wonder if it
> wouldn't be nicer to factor these as:
> 
> static float factorForLengthMode(SVGLengthMode mode, const FloatSize&
> viewportSize)
> {
>     switch (mode) {
>     case LengthModeWidth:
>         return viewportSize.width();
> ...
>     }
> }
> 
> ...
> return floatValueForLength(length, factorForLengthMode(...));
> 
> I'll see if I can whip something up for that...)

Acknowledged.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGMaskE...
File Source/core/svg/SVGMaskElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGMaskE...
Source/core/svg/SVGMaskElement.h:58: virtual bool isPresentationAttribute(const
QualifiedName&) const override final;
On 2015/02/04 13:24:53, fs wrote:
> Class is final.

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGPatte...
File Source/core/svg/SVGPatternElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGPatte...
Source/core/svg/SVGPatternElement.h:73: virtual bool
isPresentationAttribute(const QualifiedName&) const override final;
On 2015/02/04 13:24:53, fs wrote:
> final

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
File Source/core/svg/SVGRectElement.cpp (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
Source/core/svg/SVGRectElement.cpp:92: } else if (!property) {
On 2015/02/04 13:24:53, fs wrote:
> Why the condition here?

Just a remnant from early testing, shouldn't be necessary.

> (Also, could drop the {}'s in this if-cascade.)

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
File Source/core/svg/SVGRectElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGRectE...
Source/core/svg/SVGRectElement.h:46: virtual bool isPresentationAttribute(const
QualifiedName&) const override;
On 2015/02/04 13:24:53, fs wrote:
> final

Done.

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGUseEl...
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/896773002/diff/80001/Source/core/svg/SVGUseEl...
Source/core/svg/SVGUseElement.h:59: virtual void
collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&,
MutableStylePropertySet*) override final;
On 2015/02/04 13:24:53, fs wrote:
> final

Done.

https://codereview.chromium.org/896773002/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/896773002/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:509:
result.setQuirk(primitiveValue->isQuirkValue());
On 2015/02/05 03:26:30, Timothy Loh wrote:
> No need for quirk values

Acknowledged.

[pdr., 2015-02-05 22:25:52]

LGTM here, but please wait for fs's as well.

[Erik Dahlström (inactive), 2015-02-06 09:32:52]

New patchsets have been uploaded after l-g-t-m from pdr@chromium.org

[fs, 2015-02-06 11:54:01]

LGTM w/ some test nits/thoughts

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/animati...
File LayoutTests/svg/animations/script-tests/animate-css-xml-attributeType.js
(right):

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/animati...
LayoutTests/svg/animations/script-tests/animate-css-xml-attributeType.js:5: var
polygon = createSVGElement("polygon");
Land this separately so that it's obivous that the change in itself is benign?

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
File LayoutTests/svg/css/parse-length.html (right):

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
LayoutTests/svg/css/parse-length.html:37: //testComputed("width", "  100",
"100px");
Remove or uncomment (i.e. either add when adding 'width', or update expectations
at that time.)

Same for 'width' tests below.

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
LayoutTests/svg/css/parse-length.html:52: // testComputed("x", "1ex",
"12.8000001907349px"); // enable this again once http://crbug.com/441840 is
fixed
Maybe let this run, and have it be "XFAIL" (with a FIXME, so that when the fix
for that lands it'll be noticed)

[Erik Dahlström (inactive), 2015-02-06 12:10:31]

New patchsets have been uploaded after l-g-t-m from fs@opera.com

[fs, 2015-02-06 12:28:54]

https://codereview.chromium.org/896773002/diff/160001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.h (right):

https://codereview.chromium.org/896773002/diff/160001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.h:70: float convertValueToUserUnits(float,
SVGLengthMode, SVGLengthType fromUnit, ExceptionState&) const;
If https://codereview.chromium.org/901193002/ lands before this, it's possible
you get a conflict here and in the .cpp file. Should be a trivial modify-add
thing though.

[Erik Dahlström (inactive), 2015-02-06 13:19:54]

See https://codereview.chromium.org/890123003 for the test change.

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/animati...
File LayoutTests/svg/animations/script-tests/animate-css-xml-attributeType.js
(right):

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/animati...
LayoutTests/svg/animations/script-tests/animate-css-xml-attributeType.js:5: var
polygon = createSVGElement("polygon");
On 2015/02/06 11:54:00, fs wrote:
> Land this separately so that it's obivous that the change in itself is benign?

Done.

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
File LayoutTests/svg/css/parse-length.html (right):

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
LayoutTests/svg/css/parse-length.html:37: //testComputed("width", "  100",
"100px");
On 2015/02/06 11:54:00, fs wrote:
> Remove or uncomment (i.e. either add when adding 'width', or update
expectations
> at that time.)
> 
> Same for 'width' tests below.

Done.

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
LayoutTests/svg/css/parse-length.html:52: // testComputed("x", "1ex",
"12.8000001907349px"); // enable this again once http://crbug.com/441840 is
fixed
On 2015/02/06 11:54:00, fs wrote:
> Maybe let this run, and have it be "XFAIL" (with a FIXME, so that when the fix
> for that lands it'll be noticed)

Not possible since it only fails on mac.

[Erik Dahlström (inactive), 2015-02-06 13:21:23]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-06 13:22:08]

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

[fs, 2015-02-06 13:31:56]

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
File LayoutTests/svg/css/parse-length.html (right):

https://codereview.chromium.org/896773002/diff/140001/LayoutTests/svg/css/par...
LayoutTests/svg/css/parse-length.html:52: // testComputed("x", "1ex",
"12.8000001907349px"); // enable this again once http://crbug.com/441840 is
fixed
On 2015/02/06 13:19:53, Erik Dahlström wrote:
> On 2015/02/06 11:54:00, fs wrote:
> > Maybe let this run, and have it be "XFAIL" (with a FIXME, so that when the
fix
> > for that lands it'll be noticed)
> 
> Not possible since it only fails on mac.

Acknowledged.

[commit-bot: I haz the power, 2015-02-06 15:08:10]

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

[commit-bot: I haz the power, 2015-02-06 15:08:12]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[Erik Dahlström (inactive), 2015-02-06 15:10:21]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-06 15:11:15]

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

[commit-bot: I haz the power, 2015-02-06 17:08:14]

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

[commit-bot: I haz the power, 2015-02-06 17:08:16]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/49691)

[Erik Dahlström (inactive), 2015-02-08 03:17:11]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-08 03:17:19]

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

[Erik Dahlström (inactive), 2015-02-08 03:17:27]

The CQ bit was unchecked by ed@opera.com

[Erik Dahlström (inactive), 2015-02-08 04:45:40]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-08 04:45:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896773002/200001

[commit-bot: I haz the power, 2015-02-08 04:54:54]

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

[commit-bot: I haz the power, 2015-02-08 04:54:56]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)

[Erik Dahlström (inactive), 2015-02-08 04:57:22]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-08 04:58:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896773002/220001

[commit-bot: I haz the power, 2015-02-08 05:05:54]

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

[commit-bot: I haz the power, 2015-02-08 05:05:56]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41933)

[Erik Dahlström (inactive), 2015-02-12 21:49:02]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-12 21:49:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896773002/280001

[commit-bot: I haz the power, 2015-02-12 21:56:19]

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

[commit-bot: I haz the power, 2015-02-12 21:56:22]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[Erik Dahlström (inactive), 2015-02-12 22:05:51]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-12 22:06:36]

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

[commit-bot: I haz the power, 2015-02-12 22:16:53]

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

[commit-bot: I haz the power, 2015-02-12 22:16:55]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[fs, 2015-02-13 08:41:36]

The CQ bit was checked by fs@opera.com

[fs, 2015-02-13 08:41:38]

lgtm

[commit-bot: I haz the power, 2015-02-13 08:42:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896773002/320001

[commit-bot: I haz the power, 2015-02-13 08:49:13]

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

[commit-bot: I haz the power, 2015-02-13 08:49:15]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[Erik Dahlström (inactive), 2015-02-14 01:11:15]

New patchsets have been uploaded after l-g-t-m from fs@opera.com

[Erik Dahlström (inactive), 2015-02-14 01:33:01]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-02-14 01:34:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896773002/340001

[commit-bot: I haz the power, 2015-02-14 02:29:56]

Committed patchset #18 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190180