Move getTotalLength and getPointAtLength methods from SVGPathElement to SVGGeometryElement.

Move getTotalLength and getPointAtLength methods from SVGPathElement to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
Committed: https://crrev.com/887bbf6c0edcabfa012099379c2d68a3dc4afd3f
Cr-Commit-Position: refs/heads/master@{#427992}

[Shanmuga Pandi, 2016-10-13 07:27:41]

Description was changed from

==========
Move getTotolLength and getPointAtLength methods from SVGPathElement to
SVGGeometryElement.

BUG=596043
==========

to

==========
Move getTotolLength and getPointAtLength methods
from SVGPathElement to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement
BUG=596043
==========

[Shanmuga Pandi, 2016-10-13 07:31:15]

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

[Shanmuga Pandi, 2016-10-13 07:31:16]

PTAL!!

[Stephen Chennney, 2016-10-13 14:23:20]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2016-10-13 14:23:20]

The spec does not remove the getTotalLength or getPointAtLength functions from
SVGPathElement. See https://svgwg.org/specs/paths/

It's fine to add them to SVGGeometryElement as per the spec, but it's not OK to
remove them from SVGPathElement (which would break back compatibility in a major
way).

[fs, 2016-10-13 14:58:44]

On 2016/10/13 at 14:23:20, schenney wrote:
> The spec does not remove the getTotalLength or getPointAtLength functions from
SVGPathElement. See https://svgwg.org/specs/paths/

That spec is not up-to-date.
https://svgwg.org/svg2-draft/paths.html#InterfaceSVGPathElement is the relevant
one.

> It's fine to add them to SVGGeometryElement as per the spec, but it's not OK
to remove them from SVGPathElement (which would break back compatibility in a
major way).

The methods only move in the prototype chain, so unless you explicitly check for
it on the prototype of SVGPathElement it should not break anything.

[Stephen Chennney, 2016-10-13 15:17:51]

On 2016/10/13 14:58:44, fs wrote:
> On 2016/10/13 at 14:23:20, schenney wrote:
> > The spec does not remove the getTotalLength or getPointAtLength functions
from
> SVGPathElement. See https://svgwg.org/specs/paths/
> 
> That spec is not up-to-date.
> https://svgwg.org/svg2-draft/paths.html#InterfaceSVGPathElement is the
relevant
> one.

OK, thanks for resolving that confusion. The working group really needs to clean
up the spec links to avoid problems like that.

> > It's fine to add them to SVGGeometryElement as per the spec, but it's not OK
> to remove them from SVGPathElement (which would break back compatibility in a
> major way).
> 
> The methods only move in the prototype chain, so unless you explicitly check
for
> it on the prototype of SVGPathElement it should not break anything.

I presume we have tests that still call the SVGPathElement versions. Shanmuga,
could you please confirm that.

[Shanmuga Pandi, 2016-10-14 05:12:54]

On 2016/10/13 15:17:51, Stephen Chennney wrote:
> On 2016/10/13 14:58:44, fs wrote:
> > On 2016/10/13 at 14:23:20, schenney wrote:
> > > The spec does not remove the getTotalLength or getPointAtLength functions
> from
> > SVGPathElement. See https://svgwg.org/specs/paths/
> > 
> > That spec is not up-to-date.
> > https://svgwg.org/svg2-draft/paths.html#InterfaceSVGPathElement is the
> relevant
> > one.
> 
> OK, thanks for resolving that confusion. The working group really needs to
clean
> up the spec links to avoid problems like that.
> 
> > > It's fine to add them to SVGGeometryElement as per the spec, but it's not
OK
> > to remove them from SVGPathElement (which would break back compatibility in
a
> > major way).
> > 
> > The methods only move in the prototype chain, so unless you explicitly check
> for
> > it on the prototype of SVGPathElement it should not break anything.
> 
> I presume we have tests that still call the SVGPathElement versions. Shanmuga,
> could you please confirm that.

Yes. Those tests still will pass, since SVGPathElement inherits
SVGGeometryElement.
I have moved some of those testcases to SVGGeometryElement-totalLength.html and
SVGGeometryElement-totalLength.html.

[Shanmuga Pandi, 2016-10-24 09:00:17]

shanmuga.m@samsung.com changed reviewers:
+ fs@opera.com

[Shanmuga Pandi, 2016-10-24 09:00:17]

PTAL

[fs, 2016-10-24 10:48:53]

Please tidy up/fix spelling in subject+description ("getTotolLength";
line-wrapped first line)

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt:1404:
FAIL SVGPathElement interface: operation getTotalLength() assert_own_property:
interface prototype object missing non-static operation expected property
"getTotalLength" missing
Should get this test updated upstream.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-pointAtLength.html
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-pointAtLength.html:2:
<title>SVGGeometryElement.getPointAtLength method</title>
Maybe name the file ...-getPointAtLength too for consistency.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-pointAtLength.html:8:
var pathElement = document.createElementNS("http://www.w3.org/2000/svg",
"path");
Please apply the (relevant) feedback on the getTotalLength test here as well.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-pointAtLength.html:14:
return "(" + Math.round(point.x) + ", " + Math.round(point.y) + ")";
Maybe return an array instead and use assert_array_equals?

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:2:
<title>SVGGeometryElement.totalLength method</title>
getTotalLength (here, below, and in the filename for consistency)

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:8:
var pathElement = document.createElementNS("http://www.w3.org/2000/svg",
"path");
So here we're testing an element not in the document, while the other two tests
are with elements in the document. It might make sense to be consistent on this
point and have all the elements be in the document (and have another set of
tests for the "not in document" case.)

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:11:
pathElement.setAttributeNS(null, "d", string);
Could just use setAttribute.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:14:
return length;
return pathElement.getTotalLength();

?

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:23:
var svgElement = document.createElementNS("http://www.w3.org/2000/svg", "svg");
Make this is something that could either be handled by a setup(), or just
include a reasonable <svg> fragment in the markup?

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:35:
return length;
return rectElement.getTotalLength();

?

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:38:
assert_equals(getTotalLength('0', '0', '200', '300'), 1000);
Shouldn't need to explicitly "stringify" here, the second argument to
setAttribute should do that implicitly.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dynamic-updates/SVGGeometryElement-valid-arguments.html
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dynamic-updates/SVGGeometryElement-valid-arguments.html:2:
<title>Check invalid arguments of SVGGeometryElement methods</title>
Please put this test in svg/dom instead.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp (right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:93: if
(layoutObject())
Presumably an empty path yields a length of 0, so this could probably be written
as:

if (!layoutObject())
  return 0;
return asPath().length();

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:101: if
(layoutObject())
Somewhat similar to above, I think this could be written more succinctly as:

FloatPoint point;
if (layoutObject())
  point = asPath().pointAtLength(length);
return ...;

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:34: float
getTotalLength();
Could you reorder this so that it matches the spec IDL as much as possible (feel
free to also add the 'pathLength' attribute commented out.)

[Shanmuga Pandi, 2016-10-26 09:52:12]

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt:1404:
FAIL SVGPathElement interface: operation getTotalLength() assert_own_property:
interface prototype object missing non-static operation expected property
"getTotalLength" missing
On 2016/10/24 10:48:52, fs wrote:
> Should get this test updated upstream.

As of now , I don't have github write permission as per my org's internal
policy. :(

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-pointAtLength.html
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-pointAtLength.html:2:
<title>SVGGeometryElement.getPointAtLength method</title>
On 2016/10/24 10:48:52, fs wrote:
> Maybe name the file ...-getPointAtLength too for consistency.

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:2:
<title>SVGGeometryElement.totalLength method</title>
On 2016/10/24 10:48:53, fs wrote:
> getTotalLength (here, below, and in the filename for consistency)

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:8:
var pathElement = document.createElementNS("http://www.w3.org/2000/svg",
"path");
On 2016/10/24 10:48:52, fs wrote:
> So here we're testing an element not in the document, while the other two
tests
> are with elements in the document. It might make sense to be consistent on
this
> point and have all the elements be in the document (and have another set of
> tests for the "not in document" case.)

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:11:
pathElement.setAttributeNS(null, "d", string);
On 2016/10/24 10:48:52, fs wrote:
> Could just use setAttribute.

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:14:
return length;
On 2016/10/24 10:48:53, fs wrote:
> return pathElement.getTotalLength();
> 
> ?

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:23:
var svgElement = document.createElementNS("http://www.w3.org/2000/svg", "svg");
On 2016/10/24 10:48:52, fs wrote:
> Make this is something that could either be handled by a setup(), or just
> include a reasonable <svg> fragment in the markup?

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:35:
return length;
On 2016/10/24 10:48:52, fs wrote:
> return rectElement.getTotalLength();
> 
> ?

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-totalLength.html:38:
assert_equals(getTotalLength('0', '0', '200', '300'), 1000);
On 2016/10/24 10:48:52, fs wrote:
> Shouldn't need to explicitly "stringify" here, the second argument to
> setAttribute should do that implicitly.

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp (right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:93: if
(layoutObject())
On 2016/10/24 10:48:53, fs wrote:
> Presumably an empty path yields a length of 0, so this could probably be
written
> as:
> 
> if (!layoutObject())
>   return 0;
> return asPath().length();

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:101: if
(layoutObject())
On 2016/10/24 10:48:53, fs wrote:
> Somewhat similar to above, I think this could be written more succinctly as:
> 
> FloatPoint point;
> if (layoutObject())
>   point = asPath().pointAtLength(length);
> return ...;

Done.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:34: float
getTotalLength();
On 2016/10/24 10:48:53, fs wrote:
> Could you reorder this so that it matches the spec IDL as much as possible
(feel
> free to also add the 'pathLength' attribute commented out.)

I will make a separate patch for 'pathLength'.
And currently 'pathLength' having usecounter
(V8SVGPathElement_PathLength_AttributeGetter) , should I rename this useCounter
or add new usecounter?

[Shanmuga Pandi, 2016-10-26 09:55:34]

Description was changed from

==========
Move getTotolLength and getPointAtLength methods
from SVGPathElement to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement
BUG=596043
==========

to

==========
Move getTotalLength and getPointAtLength methods
from SVGPathElement to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement
BUG=596043
==========

[fs, 2016-10-26 11:00:03]

fs@opera.com changed reviewers:
+ foolip@chromium.org

[fs, 2016-10-26 11:00:04]

LGTM w/ nits

Please also fix up the subject (missing space) and description (proper git
commit format.)

You'll need someone to sign off of the webexposed changes, +foolip for that.
(Will need additional reviewer for android_webview too eventually.)

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt:1404:
FAIL SVGPathElement interface: operation getTotalLength() assert_own_property:
interface prototype object missing non-static operation expected property
"getTotalLength" missing
On 2016/10/26 at 09:52:12, Shanmuga Pandi wrote:
> On 2016/10/24 10:48:52, fs wrote:
> > Should get this test updated upstream.
> 
> As of now , I don't have github write permission as per my org's internal
policy. :(

Acknowledged.

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2413753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:34: float
getTotalLength();
On 2016/10/26 at 09:52:12, Shanmuga Pandi wrote:
> On 2016/10/24 10:48:53, fs wrote:
> > Could you reorder this so that it matches the spec IDL as much as possible
(feel
> > free to also add the 'pathLength' attribute commented out.)
> 
> I will make a separate patch for 'pathLength'.

Acknowledged.

> And currently 'pathLength' having usecounter
(V8SVGPathElement_PathLength_AttributeGetter) , should I rename this useCounter
or add new usecounter?

I'm not aware of any cases where counter were renamed, so I'd say add a new one.
(Although this case I suspect it wouldn't be much a problem to just rename.)

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html:42:
assert_array_equals(pointAtLength('0', '0', '200', '300'), [200, 100]);
Nit: '0' -> 0 et.c

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html:50:
function pointAtLength(string) {
Nit: string -> radius

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html:57:
assert_array_equals(pointAtLength('10'), [10, 0]);
Nit: '10' -> 10 et.c

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html:35:
assert_array_equals(pointAtLength('0', '0', '200', '300'), [0, 0]);
Nit: Same as in -attached.html

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html:42:
function pointAtLength(string) {
Nit: Same as in -attached.html

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html:49:
assert_array_equals(pointAtLength('10'), [0, 0]);
Nit: Same as in -attached.html

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:37: // TODO(foolip):
SVGPoint should be DOMPoint.
Nit: I think this can be kept at its original position (before/over
isPointInFill) since it applies to all methods using SVGPoint.

[foolip, 2016-10-26 11:05:22]

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt:1404:
FAIL SVGPathElement interface: operation getTotalLength() assert_own_property:
interface prototype object missing non-static operation expected property
"getTotalLength" missing
I wrote this test, it shouldn't be failing unless I did it wrong :)

[foolip, 2016-10-26 11:12:15]

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/historical-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/historical-expected.txt:34: PASS
SVGPathElement.prototype.getPointAtLength must be moved to
SVGGeometryElement.prototype
Yay.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt:1404:
FAIL SVGPathElement interface: operation getTotalLength() assert_own_property:
interface prototype object missing non-static operation expected property
"getTotalLength" missing
On 2016/10/26 11:05:22, foolip wrote:
> I wrote this test, it shouldn't be failing unless I did it wrong :)

Strike that, I clicked the wrong this, it's historical.html that I recently
added. Still, this means that interfaces.html is out of sync with the spec,
right? If you want to fix that in web-platform-tests, that'd be nice.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: [Measure] readonly
attribute SVGAnimatedNumber pathLength;
Can you move pathLength as well?

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:32: unsigned long
getPathSegAtLength(float distance);
This seems to be gone from the spec, can you add a TODO to remove it and a use
counter? (Separate CL probably.)

[Shanmuga Pandi, 2016-10-26 11:21:11]

Description was changed from

==========
Move getTotalLength and getPointAtLength methods
from SVGPathElement to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement
BUG=596043
==========

to

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement
to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
==========

[Shanmuga Pandi, 2016-10-26 11:28:28]

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/historical-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/historical-expected.txt:34: PASS
SVGPathElement.prototype.getPointAtLength must be moved to
SVGGeometryElement.prototype
On 2016/10/26 11:12:15, foolip wrote:
> Yay.

:)

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/svg/interfaces-expected.txt:1404:
FAIL SVGPathElement interface: operation getTotalLength() assert_own_property:
interface prototype object missing non-static operation expected property
"getTotalLength" missing
On 2016/10/26 11:12:15, foolip wrote:
> On 2016/10/26 11:05:22, foolip wrote:
> > I wrote this test, it shouldn't be failing unless I did it wrong :)
> 
> Strike that, I clicked the wrong this, it's historical.html that I recently
> added. Still, this means that interfaces.html is out of sync with the spec,
> right? If you want to fix that in web-platform-tests, that'd be nice.

As of now , I don't have github write permission as per my org's internal
policy. :(

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html:42:
assert_array_equals(pointAtLength('0', '0', '200', '300'), [200, 100]);
On 2016/10/26 11:00:04, fs wrote:
> Nit: '0' -> 0 et.c

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html:50:
function pointAtLength(string) {
On 2016/10/26 11:00:04, fs wrote:
> Nit: string -> radius

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-attached.html:57:
assert_array_equals(pointAtLength('10'), [10, 0]);
On 2016/10/26 11:00:03, fs wrote:
> Nit: '10' -> 10 et.c

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html
(right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html:35:
assert_array_equals(pointAtLength('0', '0', '200', '300'), [0, 0]);
On 2016/10/26 11:00:04, fs wrote:
> Nit: Same as in -attached.html

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html:42:
function pointAtLength(string) {
On 2016/10/26 11:00:04, fs wrote:
> Nit: Same as in -attached.html

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/SVGGeometryElement-getPointAtLength-detached.html:49:
assert_array_equals(pointAtLength('10'), [0, 0]);
On 2016/10/26 11:00:04, fs wrote:
> Nit: Same as in -attached.html

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:37: // TODO(foolip):
SVGPoint should be DOMPoint.
On 2016/10/26 11:00:04, fs wrote:
> Nit: I think this can be kept at its original position (before/over
> isPointInFill) since it applies to all methods using SVGPoint.

Done.

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: [Measure] readonly
attribute SVGAnimatedNumber pathLength;
On 2016/10/26 11:12:15, foolip wrote:
> Can you move pathLength as well?

Yes. I am making a separate patch that. :)

https://codereview.chromium.org/2413753004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:32: unsigned long
getPathSegAtLength(float distance);
On 2016/10/26 11:12:15, foolip wrote:
> This seems to be gone from the spec, can you add a TODO to remove it and a use
> counter? (Separate CL probably.)

Sure. I will make the patch for this.

[foolip, 2016-10-26 11:33:37]

webexposed changes lgtm

[Shanmuga Pandi, 2016-10-26 11:41:37]

shanmuga.m@samsung.com changed reviewers:
+ timvolodine@chromium.org

[Shanmuga Pandi, 2016-10-26 11:41:38]

timvolodine@chromium.org: Please review changes in android_webview

[timvolodine, 2016-10-26 18:01:30]

android_webview/.. lgtm

[foolip, 2016-10-26 22:36:04]

Description was changed from

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement
to SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
==========

to

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement to
SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
==========

[Shanmuga Pandi, 2016-10-27 05:33:01]

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

[Shanmuga Pandi, 2016-10-27 05:33:02]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2413753004/#ps80001
(title: "Align with review comments")

[commit-bot: I haz the power, 2016-10-27 05:33:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 06:47:52]

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

[commit-bot: I haz the power, 2016-10-27 06:47:53]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Shanmuga Pandi, 2016-10-27 09:05:03]

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

[commit-bot: I haz the power, 2016-10-27 09:05:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 10:25:47]

Description was changed from

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement to
SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
==========

to

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement to
SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
==========

[commit-bot: I haz the power, 2016-10-27 10:25:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-27 10:27:39]

Description was changed from

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement to
SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043
==========

to

==========
Move getTotalLength and getPointAtLength methods from SVGPathElement to
SVGGeometryElement.

spec:
https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

BUG=596043

Committed: https://crrev.com/887bbf6c0edcabfa012099379c2d68a3dc4afd3f
Cr-Commit-Position: refs/heads/master@{#427992}
==========

[commit-bot: I haz the power, 2016-10-27 10:27:40]

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