Adding support of viewport units to SVG

Adding support of viewport units to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
Committed: https://crrev.com/c864b01c55de4e1bb440600edeec7e6e9f76ec0a
Cr-Commit-Position: refs/heads/master@{#368047}

[Shanmuga Pandi, 2015-12-22 08:29:05]

Description was changed from

==========
Adding support of viewportunits to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

to

==========
Adding support of viewportunits to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
R=SamsungPeerReview
==========

[Shanmuga Pandi, 2015-12-22 08:29:05]

shanmuga.m@samsung.com changed reviewers:
+ rob.buis@samsung.com

[Shanmuga Pandi, 2015-12-22 08:29:20]

On 2015/12/22 08:29:05, Shanmuga Pandi wrote:
> mailto:shanmuga.m@samsung.com changed reviewers:
> + mailto:rob.buis@samsung.com

PTAL!!!

[rwlbuis, 2015-12-22 16:41:49]

How does this code relate to CSSToLengthConversionData? It seems similar code is
done there, would like to avoid duplication if possible.

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:105: static float
viewportWidthPercent(const FloatSize& viewportSize)
These 4 helper functions should be inline and should move to just above where
they are used.

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:113: }
You can probably unify above two by passing a dimension, instead or
width/height. For instance something like a float widthOrHeight param.

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:364: float
SVGLengthContext::convertValueFromUserUnitsToViewportUnits(CSSPrimitiveValue::UnitType
toUnit, float value) const
Do we need to take zoom into account in these 2 methods?

[Shanmuga Pandi, 2015-12-23 09:01:00]

Have updated the code.
Please check..

And CSSToLengthConversionData have similar code , but not same. Similar
conversion APIs are available for other CSS units in SVGLengthContext.cpp.

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:105: static float
viewportWidthPercent(const FloatSize& viewportSize)
On 2015/12/22 16:41:49, rwlbuis wrote:
> These 4 helper functions should be inline and should move to just above where
> they are used.

Done.

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:113: }
On 2015/12/22 16:41:49, rwlbuis wrote:
> You can probably unify above two by passing a dimension, instead or
> width/height. For instance something like a float widthOrHeight param.

Done.

https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:364: float
SVGLengthContext::convertValueFromUserUnitsToViewportUnits(CSSPrimitiveValue::UnitType
toUnit, float value) const
On 2015/12/22 16:41:49, rwlbuis wrote:
> Do we need to take zoom into account in these 2 methods?

Acknowledged.

[Shanmuga Pandi, 2015-12-23 09:02:01]

Description was changed from

==========
Adding support of viewportunits to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
R=SamsungPeerReview
==========

to

==========
Adding support of viewportunits to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

[rwlbuis, 2015-12-23 16:39:08]

On 2015/12/23 09:01:00, Shanmuga Pandi wrote:
> Have updated the code.
> Please check..
> 
> And CSSToLengthConversionData have similar code , but not same. Similar
> conversion APIs are available for other CSS units in SVGLengthContext.cpp.
> 
>
https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):
> 
>
https://codereview.chromium.org/1544543003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:113: }
> On 2015/12/22 16:41:49, rwlbuis wrote:
> > You can probably unify above two by passing a dimension, instead or
> > width/height. For instance something like a float widthOrHeight param.
> 
> Done.

I meant combining the two functions into one, and giving it a float
widthOrHeight param.
The current solution is hard to read IMHO.

[Shanmuga Pandi, 2015-12-28 06:51:15]

Updated. Hope it is Okay now :)

[Shanmuga Pandi, 2016-01-04 07:07:21]

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

[Shanmuga Pandi, 2016-01-04 07:07:21]

PTAL !!

[fs, 2016-01-04 19:40:05]

Looks ok. Some suggested improvements follow.

(Also, add a space in "viewportunits" in subject/description?)

https://codereview.chromium.org/1544543003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGLength-viewport-units.js
(right):

https://codereview.chromium.org/1544543003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/script-tests/SVGLength-viewport-units.js:1:
description("This test checks the Viewport Units for SVGLength");
Drop 'the'. Could we fold this into the html file though and avoid this
unpleasant script-tests diversion? Maybe use testharness.js too while we're at
it.

https://codereview.chromium.org/1544543003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1544543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:376: switch (toUnit) {
I think this would be slightly if it was using same approach as dimensionForMode
- i.e have a helper that returns either the viewport width/height/min/man and
then apply that to |value| as needed. Should get rid of the duplication between
the "From" and "To" cases.

[Shanmuga Pandi, 2016-01-05 10:01:00]

Description was changed from

==========
Adding support of viewportunits to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

to

==========
Adding support of viewport units to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

[Shanmuga Pandi, 2016-01-05 10:04:49]

Thanks for the review and helping to improve the patch. 
Done the changes.

Happy new year :)

https://codereview.chromium.org/1544543003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/1544543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGLengthContext.cpp:376: switch (toUnit) {
On 2016/01/04 19:40:05, fs (OoO) wrote:
> I think this would be slightly if it was using same approach as
dimensionForMode
> - i.e have a helper that returns either the viewport width/height/min/man and
> then apply that to |value| as needed. Should get rid of the duplication
between
> the "From" and "To" cases.

Done.

[fs, 2016-01-07 09:29:14]

On 2016/01/05 at 10:04:49, shanmuga.m wrote:
> Thanks for the review and helping to improve the patch. 
> Done the changes.
> 
> Happy new year :)

Likewise!

LGTM

[Shanmuga Pandi, 2016-01-07 09:31:59]

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

[commit-bot: I haz the power, 2016-01-07 09:32:38]

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

[commit-bot: I haz the power, 2016-01-07 10:48:03]

Description was changed from

==========
Adding support of viewport units to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

to

==========
Adding support of viewport units to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

[commit-bot: I haz the power, 2016-01-07 10:48:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-07 10:48:58]

Description was changed from

==========
Adding support of viewport units to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598
==========

to

==========
Adding support of viewport units to SVG

Adding support of viewport units such as vw, vh, vmin, vmax
to SVGLength

BUG=368598

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

[commit-bot: I haz the power, 2016-01-07 10:48:58]

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