SVGLength.setValue should set the value to <NUMBER> (issue 2113943003 by shanmuga.m@samsung.com)

shanm...@samsung.com

unread,

Jul 1, 2016, 3:32:26 AM7/1/16

to rob....@samsung.com, f...@opera.com, p...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, kouhe...@chromium.org, f...@opera.com, fma...@chromium.org, blink-...@chromium.org, gyuyou...@chromium.org, sche...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com

Reviewers: rwlbuis, fs, pdr.
CL: https://codereview.chromium.org/2113943003/

Message:
PTAL!!

Thanks!

Description:
SVGLength.setValue should set the value to <NUMBER>

As per SVG2, SVGLength.setValue should set the
SVGLength's value to a <number> whose value is value.
https://svgwg.org/svg2-draft/types.html#__svg__SVGLength__value

This patch is supportive patch for calc() support for SVGLength.

BUG=558304

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+16, -8 lines):
A third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
M third_party/WebKit/Source/core/svg/SVGLength.cpp
M third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp

Index: third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
diff --git a/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html b/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
new file mode 100644
index 0000000000000000000000000000000000000000..fb8f80047271f753f9e9a545d32d65cf887a9a75
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<svg width="1" height="1" visibility="hidden">
+</svg>
+<script>
+test(function() {
+ var svgElement = document.querySelector("svg");
+ svgElement.setAttribute("width", "10em");
+ assert_equals(svgElement.width.baseVal.unitType, SVGLength.SVG_LENGTHTYPE_EMS);
+ svgElement.width.baseVal.value = 100;
+ assert_equals(svgElement.width.baseVal.value, 100);
+ assert_equals(svgElement.width.baseVal.unitType, SVGLength.SVG_LENGTHTYPE_NUMBER);
+}, "Tests setValue interface");
+</script>
Index: third_party/WebKit/Source/core/svg/SVGLength.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGLength.cpp b/third_party/WebKit/Source/core/svg/SVGLength.cpp
index 8d66d974af40ea2aa6faf4d7a18ca2a7ebf75b5a..9baa811f2c1f63d5550b0ecd1583a83abfce7394 100644
--- a/third_party/WebKit/Source/core/svg/SVGLength.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGLength.cpp
@@ -79,9 +79,7 @@ float SVGLength::value(const SVGLengthContext& context) const

void SVGLength::setValue(float value, const SVGLengthContext& context)
{
- m_value = CSSPrimitiveValue::create(
- context.convertValueFromUserUnits(value, unitMode(), m_value->typeWithCalcResolved()),
- m_value->typeWithCalcResolved());
+ m_value = CSSPrimitiveValue::create(value, CSSPrimitiveValue::UnitType::UserUnits);
}

bool isSupportedCSSUnitType(CSSPrimitiveValue::UnitType type)
Index: third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp b/third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp
index ebff1f65177f326445c48f10344a3d3bf6ab2c7d..6e6c413022bddc85986b453c8876968dbc001f3a 100644
--- a/third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp
@@ -138,11 +138,6 @@ void SVGLengthTearOff::setValue(float value, ExceptionState& exceptionState)
return;
}

- if (target()->isRelative() && !canResolveRelativeUnits(contextElement())) {
- exceptionState.throwDOMException(NotSupportedError, "Could not resolve relative length.");
- return;
- }
-
SVGLengthContext lengthContext(contextElement());
target()->setValue(value, lengthContext);
commitChange();

f...@opera.com

unread,

Jul 1, 2016, 5:08:06 AM7/1/16

to shanm...@samsung.com, p...@chromium.org, rob....@samsung.com, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

While I like the simplicity of these (new) semantics, it does change our
behavior slightly compared to every other browser. So we need to tread carefully
here. It might be worth looking for the origin of the change et.c.

> "SVGLength.setValue should set..."

s/setValue/value/g since we're talking about the 'value' property on the DOM
object.

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
File
third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
(right):

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html#newcode14
third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html:14:

}, "Tests setValue interface");

"setValue interface" => "SVGLength.value setter"

(this could also go as a <title>)

Similarly for the filename (setvalue-interface => value-setter)

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/Source/core/svg/SVGLength.cpp
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/Source/core/svg/SVGLength.cpp#newcode80
third_party/WebKit/Source/core/svg/SVGLength.cpp:80: void

SVGLength::setValue(float value, const SVGLengthContext& context)

This method is used in a few more places besides SVGLengthTearOff, so
adding a new setValueAsNumber or so would be the more cautious approach.
I think we should be able to eliminate the other setValue uses later on.

https://codereview.chromium.org/2113943003/

shanm...@samsung.com

unread,

Jul 1, 2016, 6:27:13 AM7/1/16

to f...@opera.com, p...@chromium.org, rob....@samsung.com, f...@opera.com, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

Done the changes!!!

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
File
third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html
(right):

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html#newcode14
third_party/WebKit/LayoutTests/svg/dom/SVGLength-setvalue-interface.html:14:
}, "Tests setValue interface");

On 2016/07/01 09:08:05, fs wrote:
> "setValue interface" => "SVGLength.value setter"
>
> (this could also go as a <title>)
>
> Similarly for the filename (setvalue-interface => value-setter)

Done.

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/Source/core/svg/SVGLength.cpp
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/2113943003/diff/1/third_party/WebKit/Source/core/svg/SVGLength.cpp#newcode80
third_party/WebKit/Source/core/svg/SVGLength.cpp:80: void
SVGLength::setValue(float value, const SVGLengthContext& context)

On 2016/07/01 09:08:05, fs wrote:
> This method is used in a few more places besides SVGLengthTearOff, so
adding a
> new setValueAsNumber or so would be the more cautious approach. I
think we
> should be able to eliminate the other setValue uses later on.

Done.

https://codereview.chromium.org/2113943003/

shanm...@samsung.com

unread,

Jul 1, 2016, 7:29:26 AM7/1/16

to f...@opera.com, p...@chromium.org, rob....@samsung.com, f...@opera.com, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

On 2016/07/01 09:08:05, fs wrote:

> While I like the simplicity of these (new) semantics, it does change our
> behavior slightly compared to every other browser. So we need to tread
carefully
> here. It might be worth looking for the origin of the change et.c.
>

Could you help me to trace the origin of the change ?

https://codereview.chromium.org/2113943003/

f...@opera.com

unread,

Jul 1, 2016, 7:34:35 AM7/1/16

to shanm...@samsung.com, p...@chromium.org, rob....@samsung.com, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

On 2016/07/01 at 11:29:26, shanmuga.m wrote:
> On 2016/07/01 09:08:05, fs wrote:
> > While I like the simplicity of these (new) semantics, it does change our
> > behavior slightly compared to every other browser. So we need to tread
carefully
> > here. It might be worth looking for the origin of the change et.c.
> >
>
>
> Could you help me to trace the origin of the change ?

The commit adding the spec text [1] referred to ACTION-3722 [2] which was filed
after a discussion on a F2F ~1.5y ago [3]. At a glance, the discussion there
doesn't seem to touch upon this in particular. Let's try it though. LGTM.

[1] https://github.com/w3c/svgwg/commit/2f9cc83a821281e88edf1dc2edc013b7b274d85b
[2] https://www.w3.org/Graphics/SVG/WG/track/actions/3722
[3] https://www.w3.org/2015/02/11-svg-minutes.html#item04

https://codereview.chromium.org/2113943003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 1, 2016, 7:41:05 AM7/1/16

to shanm...@samsung.com, f...@opera.com, p...@chromium.org, rob....@samsung.com, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2113943003/20001

https://codereview.chromium.org/2113943003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 1, 2016, 8:52:24 AM7/1/16

to shanm...@samsung.com, f...@opera.com, p...@chromium.org, rob....@samsung.com, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

Committed patchset #2 (id:20001)

https://codereview.chromium.org/2113943003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 1, 2016, 8:52:26 AM7/1/16

to shanm...@samsung.com, f...@opera.com, p...@chromium.org, rob....@samsung.com, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

CQ bit was unchecked.

https://codereview.chromium.org/2113943003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 1, 2016, 8:54:19 AM7/1/16

to shanm...@samsung.com, f...@opera.com, p...@chromium.org, rob....@samsung.com, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, fma...@chromium.org, gyuyou...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, rob....@samsung.com, sche...@chromium.org

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

https://codereview.chromium.org/2113943003/

Reply all

Reply to author

Forward