Move pathLength attribute from SVGPathElement to SVGGeometryElement.

Move pathLength attribute from SVGPathElement to SVGGeometryElement.

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

BUG=596043
Committed: https://crrev.com/6fd1cbb3c5a636ee3830729e4b26ca1d5d78c8d6
Cr-Commit-Position: refs/heads/master@{#435194}

[Shanmuga Pandi, 2016-11-24 06:58:53]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 06:59:34]

Dry run: 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-11-24 09:00:44]

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

[commit-bot: I haz the power, 2016-11-24 09:00:45]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Shanmuga Pandi, 2016-11-24 09:30:52]

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

[Shanmuga Pandi, 2016-11-24 09:30:53]

PTAL!!!

Thanks

[foolip, 2016-11-24 09:56:09]

lgtm % nits, but please wait for fs@ as I don't really know SVG :)

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/wpt/svg/historical-expected.txt
(right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/wpt/svg/historical-expected.txt:22: FAIL
SVGUnitTypes mixin interface must not be exposed assert_false: expected false
got true
FWIW, this will be fixed by https://github.com/w3c/web-platform-tests/pull/4219
and then only requiredFeatures will be left.

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.h:1394:
V8SVGGeometryElement_PathLength_AttributeGetter = 1692,
Continuing to measure this won't be useful, I think, so drop [Measure] from the
IDL file instead. It was added in https://codereview.chromium.org/1586623002

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:34: [Measure] readonly
attribute SVGAnimatedNumber pathLength;
Please add SameObject here as well. It doesn't change the generated bindings.

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: unsigned long
getPathSegAtLength(float distance);
What's this thing again?

[fs, 2016-11-24 11:58:00]

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.h (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGGeometryElement.h:52: SVGAnimatedNumber*
pathLength() const { return m_pathLength.get(); }
I think we should hook this up to rendering too. the easiest way to achieve that
is likely to make the pathLengthScaleFactor method virtual. (A large portion of
the implementations would be the same though, only that we have a possibility to
use a cached length in SVGPathElement.)

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: unsigned long
getPathSegAtLength(float distance);
On 2016/11/24 at 09:56:09, foolip wrote:
> What's this thing again?

Something we probably should've removed together with pathSegList... (a
"reshaped" version of this is available in the paths draft:
https://svgwg.org/specs/paths/#InterfaceSVGPathElement)

[Shanmuga Pandi, 2016-11-28 10:14:47]

PTAL!!

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.h:1394:
V8SVGGeometryElement_PathLength_AttributeGetter = 1692,
On 2016/11/24 09:56:09, foolip wrote:
> Continuing to measure this won't be useful, I think, so drop [Measure] from
the
> IDL file instead. It was added in https://codereview.chromium.org/1586623002

Done.

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.h (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGGeometryElement.h:52: SVGAnimatedNumber*
pathLength() const { return m_pathLength.get(); }
On 2016/11/24 11:57:59, fs wrote:
> I think we should hook this up to rendering too. the easiest way to achieve
that
> is likely to make the pathLengthScaleFactor method virtual. (A large portion
of
> the implementations would be the same though, only that we have a possibility
to
> use a cached length in SVGPathElement.)

Done.

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGGeometryElement.idl:34: [Measure] readonly
attribute SVGAnimatedNumber pathLength;
On 2016/11/24 09:56:09, foolip wrote:
> Please add SameObject here as well. It doesn't change the generated bindings.

Done.

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: unsigned long
getPathSegAtLength(float distance);
On 2016/11/24 11:57:59, fs wrote:
> On 2016/11/24 at 09:56:09, foolip wrote:
> > What's this thing again?
> 
> Something we probably should've removed together with pathSegList... (a
> "reshaped" version of this is available in the paths draft:
> https://svgwg.org/specs/paths/#InterfaceSVGPathElement)

I will add a TODO to remove it and a use
counter for measure on separate CL.

[fs, 2016-11-28 10:29:42]

Could you add a (simple) test for pathLength applying to a non-<path> element
(<rect> ought to be easy enough?) No need to add tests for the various
edge-cases (should have coverage already), just a simple smoke test.

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:82: if
(!element()->isSVGGeometryElement() ||
This is expected to be false always (see constructor), so it's not needed.

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

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:145: if
(!layoutObject())
I think you could DCHECK this instead. But I think the asPath() implementations
does that already, so this could as well be dropped.

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:147: float
computedPathLength = asPath().length();
Optionally, pathLengthScaleFactor could be non-virtual, and a new method
"compute(d)PathLength" be added instead. That would share more code (and avoid
virtual dispatch in the general case.)

[Shanmuga Pandi, 2016-11-28 12:24:14]

PTAL!!
Thanks

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:82: if
(!element()->isSVGGeometryElement() ||
On 2016/11/28 10:29:42, fs wrote:
> This is expected to be false always (see constructor), so it's not needed.

Done.

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

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:145: if
(!layoutObject())
On 2016/11/28 10:29:42, fs wrote:
> I think you could DCHECK this instead. But I think the asPath()
implementations
> does that already, so this could as well be dropped.

Done.

https://codereview.chromium.org/2525233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:147: float
computedPathLength = asPath().length();
On 2016/11/28 10:29:42, fs wrote:
> Optionally, pathLengthScaleFactor could be non-virtual, and a new method
> "compute(d)PathLength" be added instead. That would share more code (and avoid
> virtual dispatch in the general case.)

Done.

[fs, 2016-11-28 12:42:47]

lgtm

https://codereview.chromium.org/2525233002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/stroke/pathlength-with-geometry-element.html
(right):

https://codereview.chromium.org/2525233002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/stroke/pathlength-with-geometry-element.html:3:
<rect width="50" height="50" pathLength="4" style="fill:rgb(0,0,255);"
stroke-dashoffset="1" stroke-dasharray="1 1" stroke="black" stroke-width="10"/>
Nit: fill="blue" for brevity (ditto in the ref.)

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

https://codereview.chromium.org/2525233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:149: if
(!layoutObject())
Nit: DCHECK

[Shanmuga Pandi, 2016-11-28 14:16:13]

https://codereview.chromium.org/2525233002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/stroke/pathlength-with-geometry-element.html
(right):

https://codereview.chromium.org/2525233002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/stroke/pathlength-with-geometry-element.html:3:
<rect width="50" height="50" pathLength="4" style="fill:rgb(0,0,255);"
stroke-dashoffset="1" stroke-dasharray="1 1" stroke="black" stroke-width="10"/>
On 2016/11/28 12:42:47, fs wrote:
> Nit: fill="blue" for brevity (ditto in the ref.)

Done.

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

https://codereview.chromium.org/2525233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGGeometryElement.cpp:149: if
(!layoutObject())
On 2016/11/28 12:42:47, fs wrote:
> Nit: DCHECK

Done.

[Shanmuga Pandi, 2016-11-28 14:16:22]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 14:16:36]

Dry run: 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-11-28 15:55:21]

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

[commit-bot: I haz the power, 2016-11-28 15:55:22]

Dry run: This issue passed the CQ dry run.

[Shanmuga Pandi, 2016-11-29 04:50:14]

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

[Shanmuga Pandi, 2016-11-29 04:50:15]

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

[commit-bot: I haz the power, 2016-11-29 04:50:35]

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-11-29 04:57:19]

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

[commit-bot: I haz the power, 2016-11-29 04:57:21]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Shanmuga Pandi, 2016-11-29 05:01:20]

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

[Shanmuga Pandi, 2016-11-29 05:01:20]

timvolodine@chromium.org: Please review changes in android_webview

[foolip, 2016-11-29 13:14:26]

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: unsigned long
getPathSegAtLength(float distance);
On 2016/11/24 11:57:59, fs wrote:
> On 2016/11/24 at 09:56:09, foolip wrote:
> > What's this thing again?
> 
> Something we probably should've removed together with pathSegList... (a
> "reshaped" version of this is available in the paths draft:
> https://svgwg.org/specs/paths/#InterfaceSVGPathElement)

Can you file an issue for that, describing what it does and if it needs to be
measured to assess risk?

[fs, 2016-11-29 13:26:09]

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):

https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: unsigned long
getPathSegAtLength(float distance);
On 2016/11/29 at 13:14:26, foolip wrote:
> On 2016/11/24 11:57:59, fs wrote:
> > On 2016/11/24 at 09:56:09, foolip wrote:
> > > What's this thing again?
> > 
> > Something we probably should've removed together with pathSegList... (a
> > "reshaped" version of this is available in the paths draft:
> > https://svgwg.org/specs/paths/#InterfaceSVGPathElement)
> 
> Can you file an issue for that, describing what it does and if it needs to be
measured to assess risk?

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=669498

[timvolodine, 2016-11-29 15:31:49]

On 2016/11/29 05:01:20, Shanmuga Pandi wrote:
> mailto:timvolodine@chromium.org: Please review changes in android_webview

android_webview/.. -- lgtm

[foolip, 2016-11-29 15:33:41]

On 2016/11/29 13:26:09, fs wrote:
>
https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGPathElement.idl (right):
> 
>
https://codereview.chromium.org/2525233002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGPathElement.idl:30: unsigned long
> getPathSegAtLength(float distance);
> On 2016/11/29 at 13:14:26, foolip wrote:
> > On 2016/11/24 11:57:59, fs wrote:
> > > On 2016/11/24 at 09:56:09, foolip wrote:
> > > > What's this thing again?
> > > 
> > > Something we probably should've removed together with pathSegList... (a
> > > "reshaped" version of this is available in the paths draft:
> > > https://svgwg.org/specs/paths/#InterfaceSVGPathElement)
> > 
> > Can you file an issue for that, describing what it does and if it needs to
be
> measured to assess risk?
> 
> Filed https://bugs.chromium.org/p/chromium/issues/detail?id=669498

Thanks!

[Shanmuga Pandi, 2016-11-30 05:20:50]

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

[commit-bot: I haz the power, 2016-11-30 05:21:27]

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-11-30 09:11:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480483250934640,
"parent_rev": "1ac1a248a9765f80e3470880176b9d8020942046", "commit_rev":
"0bc2df10b3252411dd30644906774151569f8fcb"}

[commit-bot: I haz the power, 2016-11-30 09:12:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-30 09:16:05]

Description was changed from

==========
Move pathLength attribute from SVGPathElement to SVGGeometryElement.

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

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

to

==========
Move pathLength attribute from SVGPathElement to SVGGeometryElement.

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

BUG=596043

Committed: https://crrev.com/6fd1cbb3c5a636ee3830729e4b26ca1d5d78c8d6
Cr-Commit-Position: refs/heads/master@{#435194}
==========

[commit-bot: I haz the power, 2016-11-30 09:16:07]

Patchset 4 (id:??) landed as
https://crrev.com/6fd1cbb3c5a636ee3830729e4b26ca1d5d78c8d6
Cr-Commit-Position: refs/heads/master@{#435194}