[SVG] Fix root element length values handling.

[SVG] Fix root element length values handling.

SVGLengthContext::determineViewport() currently asserts that we're not
resolving lengths for the topmost element, but there's nothing to
prevent such calls.

The CL updates determineViewport() to handle root elements geracefully
(using their current viewport). It also changes the signature slightly
to operate directly on a FloatSize, reducing some of the boiler-plate
client code.

BUG=317484
R=pdr@chromium.org,schenney@chromium.org

[pdr., 2013-11-11 21:03:13]

LGTM

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
Source/core/svg/SVGLengthContext.cpp:163: FloatSize viewportSize;
Nit: I think we prefer to return the FloatSize and then check isEmpty. Up to
you, but I think it's easier to understand than out parameters.

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
Source/core/svg/SVGLengthContext.cpp:175: return value /
sqrtf(viewportSize.diagonalLengthSquared() / 2) * 100;
square root of length squared? Please refactor for speed :)

[f(malita), 2013-11-11 21:14:28]

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
Source/core/svg/SVGLengthContext.cpp:163: FloatSize viewportSize;
On 2013/11/11 21:03:14, pdr wrote:
> Nit: I think we prefer to return the FloatSize and then check isEmpty. Up to
> you, but I think it's easier to understand than out parameters.

Doh, good point - if we only have one out-param now, why not just return it?
Possibly an extra copy, but I'm guessing RVO takes care of it.

Will do.

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
Source/core/svg/SVGLengthContext.cpp:175: return value /
sqrtf(viewportSize.diagonalLengthSquared() / 2) * 100;
On 2013/11/11 21:03:14, pdr wrote:
> square root of length squared? Please refactor for speed :)

The name is misleading, there's no redundant squaring (what it's trying to say
is diagonalLengthBeforeSqrtf: w^2 + h^2) :)

[pdr., 2013-11-11 21:16:30]

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
Source/core/svg/SVGLengthContext.cpp:175: return value /
sqrtf(viewportSize.diagonalLengthSquared() / 2) * 100;
On 2013/11/11 21:14:29, Florin Malita wrote:
> On 2013/11/11 21:03:14, pdr wrote:
> > square root of length squared? Please refactor for speed :)
> 
> The name is misleading, there's no redundant squaring (what it's trying to say
> is diagonalLengthBeforeSqrtf: w^2 + h^2) :)

Gah, I just spaced out here. You're right. Carry on :)

[f(malita), 2013-11-11 21:50:05]

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
Source/core/svg/SVGLengthContext.cpp:163: FloatSize viewportSize;
On 2013/11/11 21:14:29, Florin Malita wrote:
> On 2013/11/11 21:03:14, pdr wrote:
> > Nit: I think we prefer to return the FloatSize and then check isEmpty. Up to
> > you, but I think it's easier to understand than out parameters.
> 
> Doh, good point - if we only have one out-param now, why not just return it?
> Possibly an extra copy, but I'm guessing RVO takes care of it.
> 
> Will do.

On second thought, this causes a subtle change in semantics: we can no longer
differentiate between "cannot compute the viewport" (for which
SVGLengthContext::convertValueFromUserUnitsToPercentag throws an exception) and
"a valid, empty viewport" (no exception). Maybe it's OK to conflate the two, but
I'm thinking we shouldn't do it in this patch. What say you?

[pdr., 2013-11-11 21:57:11]

On 2013/11/11 21:50:05, Florin Malita wrote:
>
https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
> File Source/core/svg/SVGLengthContext.cpp (right):
> 
>
https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
> Source/core/svg/SVGLengthContext.cpp:163: FloatSize viewportSize;
> On 2013/11/11 21:14:29, Florin Malita wrote:
> > On 2013/11/11 21:03:14, pdr wrote:
> > > Nit: I think we prefer to return the FloatSize and then check isEmpty. Up
to
> > > you, but I think it's easier to understand than out parameters.
> > 
> > Doh, good point - if we only have one out-param now, why not just return it?
> > Possibly an extra copy, but I'm guessing RVO takes care of it.
> > 
> > Will do.
> 
> On second thought, this causes a subtle change in semantics: we can no longer
> differentiate between "cannot compute the viewport" (for which
> SVGLengthContext::convertValueFromUserUnitsToPercentag throws an exception)
and
> "a valid, empty viewport" (no exception). Maybe it's OK to conflate the two,
but
> I'm thinking we shouldn't do it in this patch. What say you?

Can you try SVGRect? As far as I can tell, we added it just to carry that bit.

[pdr., 2013-11-11 21:58:19]

On 2013/11/11 21:57:11, pdr wrote:
> On 2013/11/11 21:50:05, Florin Malita wrote:
> >
>
https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
> > File Source/core/svg/SVGLengthContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/67483003/diff/1/Source/core/svg/SVGLengthCont...
> > Source/core/svg/SVGLengthContext.cpp:163: FloatSize viewportSize;
> > On 2013/11/11 21:14:29, Florin Malita wrote:
> > > On 2013/11/11 21:03:14, pdr wrote:
> > > > Nit: I think we prefer to return the FloatSize and then check isEmpty.
Up
> to
> > > > you, but I think it's easier to understand than out parameters.
> > > 
> > > Doh, good point - if we only have one out-param now, why not just return
it?
> > > Possibly an extra copy, but I'm guessing RVO takes care of it.
> > > 
> > > Will do.
> > 
> > On second thought, this causes a subtle change in semantics: we can no
longer
> > differentiate between "cannot compute the viewport" (for which
> > SVGLengthContext::convertValueFromUserUnitsToPercentag throws an exception)
> and
> > "a valid, empty viewport" (no exception). Maybe it's OK to conflate the two,
> but
> > I'm thinking we shouldn't do it in this patch. What say you?
> 
> Can you try SVGRect? As far as I can tell, we added it just to carry that bit.

On second thought--you need a size, not a rect. LGTM without this, but we should
file a bug to create a FloatSize+valid/invalid bit.

[f(malita), 2013-11-11 22:16:42]

On 2013/11/11 21:58:19, pdr wrote:
> On second thought--you need a size, not a rect. LGTM without this, but we
should
> file a bug to create a FloatSize+valid/invalid bit.

opened http://crbug.com/317892

[commit-bot: I haz the power, 2013-11-11 22:18:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/67483003/1

[f(malita), 2013-11-12 13:47:56]

On 2013/11/11 22:18:53, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/fmalita%40chromium.org/67483003/1

Weird: I tried committing this manually, but git cl dcommit threw some error;
then tried again and it was complaining about the -expected file being already
in the tree. After syncing, it appears that the CL has landed after all (but no
metadata was updated):
https://src.chromium.org/viewvc/blink?revision=161749&view=revision