SVG doesn't recognize rem units

SVG doesn't recognize rem units.

Adds support for the 'rem' unit in SVG <length> values.

BUG=368598, 470449
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193355

[Shanmuga Pandi, 2015-03-26 12:45:17]

shanmuga.m@samsung.com changed reviewers:
+ pdr@chromium.org, timloh@chromium.org

[Shanmuga Pandi, 2015-03-26 12:45:17]

Please review it.

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

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

[Erik Dahlström (inactive), 2015-03-26 16:11:04]

I think we should try to support rem units in all the svg <length> properties at
once, it's very confusing if it's only supported in some properties.

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
File LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt (right):

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt:7: LayoutSVGPath
{line} at (8,8) size 100x100 [stroke={[type=SOLID] [color=#FF0000] [stroke
width=32.00]}] [fill={[type=SOLID] [color=#000000]}] [x1=0.00] [y1=0.00]
[x2=100.00] [y2=100.00]
Why not make this an svg reftest instead, as in using
stroke-width-rem-type-expected.svg?

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
File LayoutTests/svg/stroke/stroke-width-rem-type.html (right):

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
LayoutTests/svg/stroke/stroke-width-rem-type.html:5: <line x1="0" y1="0"
x2="100" y2="100" style="stroke:rgb(255,0,0);stroke-width:2rem" />
Please add a test for:
* stroke-width using rem units but specified as an attribute

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
LayoutTests/svg/stroke/stroke-width-rem-type.html:6: Sorry, your browser does
not support inline SVG.
Is this text really necessary?

[Timothy Loh, 2015-03-27 09:51:03]

https://codereview.chromium.org/1031223003/diff/1/Source/core/css/parser/CSSP...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1031223003/diff/1/Source/core/css/parser/CSSP...
Source/core/css/parser/CSSPropertyParser.cpp:8149: if (validPrimitive) {
Can we just do the same thing as parseValue (parsedValue =
parseValidPrimitive(id, value);)? Otherwise we'll still miss heaps of other unit
types (vw, ch, etc.).

[Erik Dahlström (inactive), 2015-03-27 10:05:01]

One more thing, you need to make sure that that any new <length> units aren't
exposed on the SVG DOM interfaces. This will require some tests.

See e.g https://codereview.chromium.org/303263008 and
https://codereview.chromium.org/447193002 for how to do that.

[Shanmuga Pandi, 2015-03-27 11:42:38]

On 2015/03/27 09:51:03, Timothy Loh wrote:
>
https://codereview.chromium.org/1031223003/diff/1/Source/core/css/parser/CSSP...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1031223003/diff/1/Source/core/css/parser/CSSP...
> Source/core/css/parser/CSSPropertyParser.cpp:8149: if (validPrimitive) {
> Can we just do the same thing as parseValue (parsedValue =
> parseValidPrimitive(id, value);)? Otherwise we'll still miss heaps of other
unit
> types (vw, ch, etc.).

Currently SVG<Length> supports only the below types. By adding same as
parseValidPrimitive, will solve SVG attributes like "stroke-width" etc. But not
on SVG "width and height" property. All the unit types needs to be added.
For the first step, I am planning to support "rem" unit type.

 enum SVGLengthType {
    LengthTypeUnknown = 0,
    LengthTypeNumber,
    LengthTypePercentage,
    LengthTypeEMS,
    LengthTypeEXS,
    LengthTypePX,
    LengthTypeCM,
    LengthTypeMM,
    LengthTypeIN,
    LengthTypePT,
    LengthTypePC
}

Please let me know your opinion.

[Shanmuga Pandi, 2015-03-27 11:44:23]

On 2015/03/27 10:05:01, Erik Dahlström wrote:
> One more thing, you need to make sure that that any new <length> units aren't
> exposed on the SVG DOM interfaces. This will require some tests.
> 
> See e.g https://codereview.chromium.org/303263008 and
> https://codereview.chromium.org/447193002 for how to do that.

So with below LengthTypes, do we need to expose only till "LengthTypePC" ?

 enum SVGLengthType {
    LengthTypeUnknown = 0,
    LengthTypeNumber,
    LengthTypePercentage,
    LengthTypeEMS,
    LengthTypeEXS,
    LengthTypePX,
    LengthTypeCM,
    LengthTypeMM,
    LengthTypeIN,
    LengthTypePT,
    LengthTypePC
}

[Erik Dahlström (inactive), 2015-03-27 11:57:28]

On 2015/03/27 11:44:23, Shanmuga Pandi wrote:
> On 2015/03/27 10:05:01, Erik Dahlström wrote:
> > One more thing, you need to make sure that that any new <length> units
aren't
> > exposed on the SVG DOM interfaces. This will require some tests.
> > 
> > See e.g https://codereview.chromium.org/303263008 and
> > https://codereview.chromium.org/447193002 for how to do that.
> 
> So with below LengthTypes, do we need to expose only till "LengthTypePC" ?
> 
>  enum SVGLengthType {
>     LengthTypeUnknown = 0,
>     LengthTypeNumber,
>     LengthTypePercentage,
>     LengthTypeEMS,
>     LengthTypeEXS,
>     LengthTypePX,
>     LengthTypeCM,
>     LengthTypeMM,
>     LengthTypeIN,
>     LengthTypePT,
>     LengthTypePC
> }

To the SVGLength DOM interface yes. Internally it's fine to add whatever is
needed, it might be nice if we could use the css unit enums directly here.

Bug 470449 is a smaller part of http://crbug.com/368598, might be good to link
those bugs.

[Shanmuga Pandi, 2015-03-30 14:40:14]

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
File LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt (right):

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt:7: LayoutSVGPath
{line} at (8,8) size 100x100 [stroke={[type=SOLID] [color=#FF0000] [stroke
width=32.00]}] [fill={[type=SOLID] [color=#000000]}] [x1=0.00] [y1=0.00]
[x2=100.00] [y2=100.00]
On 2015/03/26 16:11:03, Erik Dahlström wrote:
> Why not make this an svg reftest instead, as in using
> stroke-width-rem-type-expected.svg?

Acknowledged.

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
File LayoutTests/svg/stroke/stroke-width-rem-type.html (right):

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
LayoutTests/svg/stroke/stroke-width-rem-type.html:5: <line x1="0" y1="0"
x2="100" y2="100" style="stroke:rgb(255,0,0);stroke-width:2rem" />
On 2015/03/26 16:11:03, Erik Dahlström wrote:
> Please add a test for:
> * stroke-width using rem units but specified as an attribute

Done.

[Erik Dahlström (inactive), 2015-03-30 15:14:19]

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
File LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt (right):

https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt:7: LayoutSVGPath
{line} at (8,8) size 100x100 [stroke={[type=SOLID] [color=#FF0000] [stroke
width=32.00]}] [fill={[type=SOLID] [color=#000000]}] [x1=0.00] [y1=0.00]
[x2=100.00] [y2=100.00]
On 2015/03/30 14:40:14, Shanmuga Pandi wrote:
> On 2015/03/26 16:11:03, Erik Dahlström wrote:
> > Why not make this an svg reftest instead, as in using
> > stroke-width-rem-type-expected.svg?
> 
> Acknowledged.

Still looks like this uses png references, and I think that's generally
something to avoid. 

If the expected files were svgs where the rem unit values had been replaced by
the expected values expressed in some other unit (em or px), that seems less
likely to cause flakiness. Also, fewer binary files in the git tree == good.

[Shanmuga Pandi, 2015-03-31 05:59:57]

On 2015/03/30 15:14:19, Erik Dahlström wrote:
>
https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
> File LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt (right):
> 
>
https://codereview.chromium.org/1031223003/diff/1/LayoutTests/svg/stroke/stro...
> LayoutTests/svg/stroke/stroke-width-rem-type-expected.txt:7: LayoutSVGPath
> {line} at (8,8) size 100x100 [stroke={[type=SOLID] [color=#FF0000] [stroke
> width=32.00]}] [fill={[type=SOLID] [color=#000000]}] [x1=0.00] [y1=0.00]
> [x2=100.00] [y2=100.00]
> On 2015/03/30 14:40:14, Shanmuga Pandi wrote:
> > On 2015/03/26 16:11:03, Erik Dahlström wrote:
> > > Why not make this an svg reftest instead, as in using
> > > stroke-width-rem-type-expected.svg?
> > 
> > Acknowledged.
> 
> Still looks like this uses png references, and I think that's generally
> something to avoid. 
> 
> If the expected files were svgs where the rem unit values had been replaced by
> the expected values expressed in some other unit (em or px), that seems less
> likely to cause flakiness. Also, fewer binary files in the git tree == good.

I have removed .png files and added svg ref-test instead.

[Erik Dahlström (inactive), 2015-03-31 09:20:50]

Thanks, this is starting to look better.

I'd like to see a repaint test where the <body> or <svg> changes font-size and
some other element using rem units is affected by it by and painted correctly.

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/fast/svg/sv...
File LayoutTests/fast/svg/svglength.html (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/fast/svg/sv...
LayoutTests/fast/svg/svglength.html:16: "" : 1,
indentation seems a bit off here (and below)

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/css/svg...
File LayoutTests/svg/css/svg-length-rem-type.svg (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/css/svg...
LayoutTests/svg/css/svg-length-rem-type.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="30rem" height="30rem">
I think this should set a font-size explicitly, since otherwise it's relying on
browser font-size defaults (which may change).

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
File LayoutTests/svg/stroke/stroke-array-rem-type-expected.svg (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
LayoutTests/svg/stroke/stroke-array-rem-type-expected.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
height="300" width="300">
Missing a font-size here.

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
File LayoutTests/svg/stroke/stroke-array-rem-type.svg (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
LayoutTests/svg/stroke/stroke-array-rem-type.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
height="300" width="300">
This test would be better if the root had a font-size and the g had a different
font-size, to show that rem actually uses the correct one when resolving.

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:302: const Document& document =
currentContext->layoutObject()->document();
This warrants another test, along the lines of the following:

test.svg:
<svg font-size="100">
  <use xlink:href="other.svg#rect"/>
</svg>

other.svg:
<svg font-size="50">
  <rect id="rect" width="1rem" height="1rem"/>
</svg>

Which of the font-size values should be used when resolving the rem units when
rendering test.svg?

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: return hasExposedLengthUnit() ?
target()->valueAsString() : String::number(0);
Not all <length> properties have 0 (with no unit) as the default (lacuna) value,
e.g https://svgwg.org/svg2-draft/pservers.html#RadialGradientElementCXAttribute.

See http://jsfiddle.net/ffwn1k4t/ and http://jsfiddle.net/ffwn1k4t/1/.

It looks like returning 0 here for "unknown" values is what blink used to do, so
it isn't a functional change really. However, I do think that gecko is more
correct in returning "50%" for cx.

[Shanmuga Pandi, 2015-04-01 15:01:38]

Added repaint test also.

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/fast/svg/sv...
File LayoutTests/fast/svg/svglength.html (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/fast/svg/sv...
LayoutTests/fast/svg/svglength.html:16: "" : 1,
On 2015/03/31 09:20:49, Erik Dahlström wrote:
> indentation seems a bit off here (and below)

Done.

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/css/svg...
File LayoutTests/svg/css/svg-length-rem-type.svg (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/css/svg...
LayoutTests/svg/css/svg-length-rem-type.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
width="30rem" height="30rem">
On 2015/03/31 09:20:50, Erik Dahlström wrote:
> I think this should set a font-size explicitly, since otherwise it's relying
on
> browser font-size defaults (which may change).

Done.

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
File LayoutTests/svg/stroke/stroke-array-rem-type-expected.svg (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
LayoutTests/svg/stroke/stroke-array-rem-type-expected.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
height="300" width="300">
On 2015/03/31 09:20:50, Erik Dahlström wrote:
> Missing a font-size here.

Done.

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
File LayoutTests/svg/stroke/stroke-array-rem-type.svg (right):

https://codereview.chromium.org/1031223003/diff/40001/LayoutTests/svg/stroke/...
LayoutTests/svg/stroke/stroke-array-rem-type.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
height="300" width="300">
On 2015/03/31 09:20:50, Erik Dahlström wrote:
> This test would be better if the root had a font-size and the g had a
different
> font-size, to show that rem actually uses the correct one when resolving.

Done.

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:302: const Document& document =
currentContext->layoutObject()->document();
On 2015/03/31 09:20:50, Erik Dahlström wrote:
> This warrants another test, along the lines of the following:
> 
> test.svg:
> <svg font-size="100">
>   <use xlink:href="other.svg#rect"/>
> </svg>
> 
> other.svg:
> <svg font-size="50">
>   <rect id="rect" width="1rem" height="1rem"/>
> </svg>
> 
> Which of the font-size values should be used when resolving the rem units when
> rendering test.svg?


I had checked this test case, and found test.svg receives rect size as
(1px,1px), due to below reasons.
ElementResolveContext sets m_rootElementStyle as null, since 
both document.layoutStyle() and documentElement->layoutStyle() returns NULL.

And SVGLengthContext::convertValueFromREMSToUserUnits never got hit in this
scenario.

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: return hasExposedLengthUnit() ?
target()->valueAsString() : String::number(0);
On 2015/03/31 09:20:50, Erik Dahlström wrote:
> Not all <length> properties have 0 (with no unit) as the default (lacuna)
value,
> e.g
https://svgwg.org/svg2-draft/pservers.html#RadialGradientElementCXAttribute.
> 
> See http://jsfiddle.net/ffwn1k4t/ and http://jsfiddle.net/ffwn1k4t/1/.
> 
> It looks like returning 0 here for "unknown" values is what blink used to do,
so
> it isn't a functional change really. However, I do think that gecko is more
> correct in returning "50%" for cx.

Added TODO note. I will work on this issue seperately.

[Erik Dahlström (inactive), 2015-04-02 07:58:41]

lgtm w/nits.

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:302: const Document& document =
currentContext->layoutObject()->document();
On 2015/04/01 15:01:37, Shanmuga Pandi wrote:
> On 2015/03/31 09:20:50, Erik Dahlström wrote:
> > This warrants another test, along the lines of the following:
> > 
> > test.svg:
> > <svg font-size="100">
> >   <use xlink:href="other.svg#rect"/>
> > </svg>
> > 
> > other.svg:
> > <svg font-size="50">
> >   <rect id="rect" width="1rem" height="1rem"/>
> > </svg>
> > 
> > Which of the font-size values should be used when resolving the rem units
when
> > rendering test.svg?
> 
> 
> I had checked this test case, and found test.svg receives rect size as
> (1px,1px), due to below reasons.
> ElementResolveContext sets m_rootElementStyle as null, since 
> both document.layoutStyle() and documentElement->layoutStyle() returns NULL.
> 
> And SVGLengthContext::convertValueFromREMSToUserUnits never got hit in this
> scenario.

Ok, I suspected that it would be broken for that case. Could you please file a
bug on this, and set it to be blocked on issues 470449 and 423312.

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: return hasExposedLengthUnit() ?
target()->valueAsString() : String::number(0);
On 2015/04/01 15:01:37, Shanmuga Pandi wrote:
> On 2015/03/31 09:20:50, Erik Dahlström wrote:
> > Not all <length> properties have 0 (with no unit) as the default (lacuna)
> value,
> > e.g
> https://svgwg.org/svg2-draft/pservers.html#RadialGradientElementCXAttribute.
> > 
> > See http://jsfiddle.net/ffwn1k4t/ and http://jsfiddle.net/ffwn1k4t/1/.
> > 
> > It looks like returning 0 here for "unknown" values is what blink used to
do,
> so
> > it isn't a functional change really. However, I do think that gecko is more
> > correct in returning "50%" for cx.
> 
> Added TODO note. I will work on this issue seperately.
> 

Acknowledged.

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:272: if (currentContext->layoutObject()) {
Do we really need this loop? Why not just fetch the document directly from
m_context->document()?

The same applies to the method below as well.

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: // TODO: Not all <length> properties
have 0 (with no unit) as the default (lacuna) value, Need to return default
value instead of 0
Nit: should be TODO(name), see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments.

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:127: target()->setValueAsString(oldValue,
ASSERT_NO_EXCEPTION); // rollback t o old value
Nit: s/t o/to/

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

On 2015/03/27 09:51:03, Timothy Loh wrote:
>
https://codereview.chromium.org/1031223003/diff/1/Source/core/css/parser/CSSP...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1031223003/diff/1/Source/core/css/parser/CSSP...
> Source/core/css/parser/CSSPropertyParser.cpp:8149: if (validPrimitive) {
> Can we just do the same thing as parseValue (parsedValue =
> parseValidPrimitive(id, value);)? Otherwise we'll still miss heaps of other
unit
> types (vw, ch, etc.).

That is true, but I think doing it in small steps is fine. The end goal is to
unify the parsing completely, and to use the css length parser assuming the
performance is good enough (the css bison parser was too slow in comparison to
the svg length parser, but I'm hoping that new css parser is, or will be, fast
enough).

Another nit for Shanmuga, please update the review description, I find this part
a bit hard to read "Added support rem type for svg<length> and other svg
attributes".
How about: "Adds support for the 'rem' unit in SVG <length> values."?

Not sure if there are any changes necessary for the animation side of this to
work, could you please check? E.g animate some attribute from 20px to 100rem, or
similar. I'm fairly sure that it will work though.

[Shanmuga Pandi, 2015-04-02 13:06:51]

Checked the animated side of SVG with the below content.

<!DOCTYPE html>
<html style="font-size:10px">
<body style="font-size:5px">
<svg width="500" height="500">
<circle cx="30" cy="30" r="0.2em" style="stroke: none; fill: #0000ff;">
    <animate attributeName="r" attributeType="XML"
             from="0.2em"  to="2rem"
             begin="0s" dur="5s"
             fill="remove" repeatCount="indefinite"/>
</circle>
</svg>
</body>
</html>

And It is animated as expected.

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:272: if (currentContext->layoutObject()) {
On 2015/04/02 07:58:41, Erik Dahlström wrote:
> Do we really need this loop? Why not just fetch the document directly from
> m_context->document()?
> 
> The same applies to the method below as well.

Done.

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: // TODO: Not all <length> properties
have 0 (with no unit) as the default (lacuna) value, Need to return default
value instead of 0
On 2015/04/02 07:58:41, Erik Dahlström wrote:
> Nit: should be TODO(name), see
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments.

Done.

https://codereview.chromium.org/1031223003/diff/60001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:127: target()->setValueAsString(oldValue,
ASSERT_NO_EXCEPTION); // rollback t o old value
On 2015/04/02 07:58:41, Erik Dahlström wrote:
> Nit: s/t o/to/

Done.

[Shanmuga Pandi, 2015-04-02 13:21:37]

https://codereview.chromium.org/1031223003/diff/40001/Source/core/svg/SVGLeng...
> Source/core/svg/SVGLengthContext.cpp:302: const Document& document =
> currentContext->layoutObject()->document();
> On 2015/04/01 15:01:37, Shanmuga Pandi wrote:
> > On 2015/03/31 09:20:50, Erik Dahlström wrote:
> > > This warrants another test, along the lines of the following:
> > > 
> > > test.svg:
> > > <svg font-size="100">
> > >   <use xlink:href="other.svg#rect"/>
> > > </svg>
> > > 
> > > other.svg:
> > > <svg font-size="50">
> > >   <rect id="rect" width="1rem" height="1rem"/>
> > > </svg>
> > > 
> > > Which of the font-size values should be used when resolving the rem units
> when
> > > rendering test.svg?
> > 
> > 
> > I had checked this test case, and found test.svg receives rect size as
> > (1px,1px), due to below reasons.
> > ElementResolveContext sets m_rootElementStyle as null, since 
> > both document.layoutStyle() and documentElement->layoutStyle() returns NULL.
> > 
> > And SVGLengthContext::convertValueFromREMSToUserUnits never got hit in this
> > scenario.
> 
> Ok, I suspected that it would be broken for that case. Could you please file a
> bug on this, and set it to be blocked on issues 470449 and 423312.
> 

 Filed bug 473117, I dont see any option to add "blocked on"

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

Still lgtm w/nit.

Thanks, I set issue 473117 as blocked on the two other issues mentioned
previously.

https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: // TODO(shanmuga.m@samsung.com): Not
all <length> properties have 0 (with no unit) as the default (lacuna) value,
Need to return default value or previous valid value instead of 0
Returning the previous value would be entirely wrong.

[Shanmuga Pandi, 2015-04-02 13:43:58]

https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthTearOff.cpp (right):

https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthTearOff.cpp:111: // TODO(shanmuga.m@samsung.com): Not
all <length> properties have 0 (with no unit) as the default (lacuna) value,
Need to return default value or previous valid value instead of 0
On 2015/04/02 13:37:25, Erik Dahlström wrote:
> Returning the previous value would be entirely wrong.

I have checked the behavior in gecko with the following code.

document.querySelector("#svg").width.baseVal.valueAsString = "100px";
document.querySelector("#svg").width.baseVal.valueAsString = "1asda";

document.body.innerHTML += "<br> svg.width=" +
document.querySelector("#svg").width.baseVal.valueAsString;

gecko returns "100px" which is previous value, rather than default value "100%".

I wonder which behavior is correct? If you really feel, it is completely wrong,
let me know I will correct it. Thanks.

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

On 2015/04/02 13:43:58, Shanmuga Pandi wrote:
>
https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
> File Source/core/svg/SVGLengthTearOff.cpp (right):
> 
>
https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
> Source/core/svg/SVGLengthTearOff.cpp:111: //
mailto:TODO(shanmuga.m@samsung.com): Not
> all <length> properties have 0 (with no unit) as the default (lacuna) value,
> Need to return default value or previous valid value instead of 0
> On 2015/04/02 13:37:25, Erik Dahlström wrote:
> > Returning the previous value would be entirely wrong.
> 
> I have checked the behavior in gecko with the following code.
> 
> document.querySelector("#svg").width.baseVal.valueAsString = "100px";
> document.querySelector("#svg").width.baseVal.valueAsString = "1asda";
> 
> document.body.innerHTML += "<br> svg.width=" +
> document.querySelector("#svg").width.baseVal.valueAsString;
> 
> gecko returns "100px" which is previous value, rather than default value
"100%".
> 
> I wonder which behavior is correct? If you really feel, it is completely
wrong,
> let me know I will correct it. Thanks.

What Firefox is doing atm is the way I'd expect it to work.

In this example the assignment (in SVGLengthTeatOff::setValueAsString) will have
detected the "invalid" value and rolled back to the previous one. I'm thinking
of the case where you have <svg width="10rem"/> and then read it out with
width.baseVal.valueAsString. It's not the responsibility of the getter to
(possibly) rollback to a previous value because that would mean it would have to
store it somewhere.

[Shanmuga Pandi, 2015-04-02 13:55:44]

On 2015/04/02 13:51:09, Erik Dahlström wrote:
> On 2015/04/02 13:43:58, Shanmuga Pandi wrote:
> >
>
https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
> > File Source/core/svg/SVGLengthTearOff.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1031223003/diff/80001/Source/core/svg/SVGLeng...
> > Source/core/svg/SVGLengthTearOff.cpp:111: //
> mailto:TODO(shanmuga.m@samsung.com): Not
> > all <length> properties have 0 (with no unit) as the default (lacuna) value,
> > Need to return default value or previous valid value instead of 0
> > On 2015/04/02 13:37:25, Erik Dahlström wrote:
> > > Returning the previous value would be entirely wrong.
> > 
> > I have checked the behavior in gecko with the following code.
> > 
> > document.querySelector("#svg").width.baseVal.valueAsString = "100px";
> > document.querySelector("#svg").width.baseVal.valueAsString = "1asda";
> > 
> > document.body.innerHTML += "<br> svg.width=" +
> > document.querySelector("#svg").width.baseVal.valueAsString;
> > 
> > gecko returns "100px" which is previous value, rather than default value
> "100%".
> > 
> > I wonder which behavior is correct? If you really feel, it is completely
> wrong,
> > let me know I will correct it. Thanks.
> 
> What Firefox is doing atm is the way I'd expect it to work.
> 
> In this example the assignment (in SVGLengthTeatOff::setValueAsString) will
have
> detected the "invalid" value and rolled back to the previous one. I'm thinking
> of the case where you have <svg width="10rem"/> and then read it out with
> width.baseVal.valueAsString. It's not the responsibility of the getter to
> (possibly) rollback to a previous value because that would mean it would have
to
> store it somewhere.

Acknowledged and corrected.
Thanks for reviewing.

[Shanmuga Pandi, 2015-04-06 14:59:57]

On 2015/04/02 13:55:44, Shanmuga Pandi wrote:

Please review it. Thanks

[Timothy Loh, 2015-04-08 00:07:18]

On 2015/04/06 14:59:57, Shanmuga Pandi wrote:
> On 2015/04/02 13:55:44, Shanmuga Pandi wrote:
> 
> Please review it. Thanks

If ed@ is fine with this then just go ahead and land it. No need to get more
reviewers, I trust Erik knows what he's doing :-)

[Shanmuga Pandi, 2015-04-08 06:15:06]

On 2015/04/08 00:07:18, Timothy Loh wrote:
> On 2015/04/06 14:59:57, Shanmuga Pandi wrote:
> > On 2015/04/02 13:55:44, Shanmuga Pandi wrote:
> > 
> > Please review it. Thanks
> 
> If ed@ is fine with this then just go ahead and land it. No need to get more
> reviewers, I trust Erik knows what he's doing :-)

@timloh, 
  Yes. I trust Erik too. Hope he will land this patch. :-)

[Timothy Loh, 2015-04-08 06:17:44]

On 2015/04/08 06:15:06, Shanmuga Pandi wrote:
> On 2015/04/08 00:07:18, Timothy Loh wrote:
> > On 2015/04/06 14:59:57, Shanmuga Pandi wrote:
> > > On 2015/04/02 13:55:44, Shanmuga Pandi wrote:
> > > 
> > > Please review it. Thanks
> > 
> > If ed@ is fine with this then just go ahead and land it. No need to get more
> > reviewers, I trust Erik knows what he's doing :-)
> 
> @timloh, 
>   Yes. I trust Erik too. Hope he will land this patch. :-)

There's a commit box (or button) that you can click to land the patch

[Shanmuga Pandi, 2015-04-08 06:18:41]

The CQ bit was checked by shanmuga.m@samsung.com

[Shanmuga Pandi, 2015-04-08 06:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from ed@opera.com
Link to the patchset: https://codereview.chromium.org/1031223003/#ps100001
(title: "Fixed small nit")

[commit-bot: I haz the power, 2015-04-08 06:19:07]

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

[commit-bot: I haz the power, 2015-04-08 06:28:10]

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

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

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

[Timothy Loh, 2015-04-08 06:29:13]

On 2015/04/08 06:28:11, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

rs-lgtm, I guess you need more OWNERs :-)

[Shanmuga Pandi, 2015-04-08 09:30:51]

The CQ bit was unchecked by shanmuga.m@samsung.com

[Shanmuga Pandi, 2015-04-08 09:30:51]

The CQ bit was checked by shanmuga.m@samsung.com

[Shanmuga Pandi, 2015-04-08 09:30:52]

The patchset sent to the CQ was uploaded after l-g-t-m from ed@opera.com,
timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1031223003/#ps120001
(title: ""fixed windows build failure"")

[commit-bot: I haz the power, 2015-04-08 09:31:37]

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

[commit-bot: I haz the power, 2015-04-08 10:00:29]

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

[commit-bot: I haz the power, 2015-04-08 10:00:31]

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...)
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[Shanmuga Pandi, 2015-04-08 11:14:37]

The CQ bit was checked by shanmuga.m@samsung.com

[Shanmuga Pandi, 2015-04-08 11:14:38]

The patchset sent to the CQ was uploaded after l-g-t-m from ed@opera.com,
timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1031223003/#ps140001
(title: "Fixes builderror")

[commit-bot: I haz the power, 2015-04-08 11:14:56]

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

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

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193355