SVG <use> should update when the referenced <path> is updating pathSegList.

SVG <use> should update when the referenced <path> is updating pathSegList.

When pathSegListChanged, no invalidation occurs for referenced svg <use> elements.
This change will do invalidation for those.

BUG=445857
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195340

[Shanmuga Pandi, 2015-05-13 12:05:03]

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

[Shanmuga Pandi, 2015-05-13 12:05:03]

Please review the changes.
Thanks

[fs, 2015-05-13 12:35:21]

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
File LayoutTests/svg/repaint/svg-pathseglist-change.html (right):

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:5: window.onload =
runRepaintAndPixelTest;
If this is indeed intended to be a repaint test, then I think just
runRepaintTest would suffice. If it's intended to be a ref-test then more
changes will be needed.

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:8: setTimeout(function() {
Why the setTimeout? (Not sure the rAF is needed either.)

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:9:
document.getElementById("path").pathSegList.getItem(2).y = 200;
I think we added "proper" indexing for pathseg lists, so just pathSegList[2].y =
... should work as well (and is preferred - at least by me...)

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:11: if (window.testRunner)
finishRepaintTest() checks this already.

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:19: <path id="path"
d="M50,50 L100,50 L100,100 Z" style="stroke: #006666; fill:none;"/>
How about just fill="green" instead - or is the stroke relevant in any way
(doesn't look like it)?

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:20: <use xlink:href="#path"
y="0" x="100"/>
Drop y="0" (it's the default)

https://codereview.chromium.org/1134733005/diff/1/Source/core/svg/SVGPathElem...
File Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1134733005/diff/1/Source/core/svg/SVGPathElem...
Source/core/svg/SVGPathElement.cpp:253: SVGElement::InvalidationGuard
invalidationGuard(this);
Move this down a bit. (After invalidateSVGAttributes should be ok I think.)

[Shanmuga Pandi, 2015-05-13 13:00:40]

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
File LayoutTests/svg/repaint/svg-pathseglist-change.html (right):

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:5: window.onload =
runRepaintAndPixelTest;
On 2015/05/13 12:35:20, fs wrote:
> If this is indeed intended to be a repaint test, then I think just
> runRepaintTest would suffice. If it's intended to be a ref-test then more
> changes will be needed.

It is ref-test. So changed test case according to it.

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:9:
document.getElementById("path").pathSegList.getItem(2).y = 200;
On 2015/05/13 12:35:21, fs wrote:
> I think we added "proper" indexing for pathseg lists, so just pathSegList[2].y
=
> ... should work as well (and is preferred - at least by me...)

Done.

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:11: if (window.testRunner)
On 2015/05/13 12:35:20, fs wrote:
> finishRepaintTest() checks this already.

removed

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:19: <path id="path"
d="M50,50 L100,50 L100,100 Z" style="stroke: #006666; fill:none;"/>
On 2015/05/13 12:35:21, fs wrote:
> How about just fill="green" instead - or is the stroke relevant in any way
> (doesn't look like it)?

Done.

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:20: <use xlink:href="#path"
y="0" x="100"/>
On 2015/05/13 12:35:20, fs wrote:
> Drop y="0" (it's the default)

Done.

https://codereview.chromium.org/1134733005/diff/1/Source/core/svg/SVGPathElem...
File Source/core/svg/SVGPathElement.cpp (right):

https://codereview.chromium.org/1134733005/diff/1/Source/core/svg/SVGPathElem...
Source/core/svg/SVGPathElement.cpp:253: SVGElement::InvalidationGuard
invalidationGuard(this);
On 2015/05/13 12:35:21, fs wrote:
> Move this down a bit. (After invalidateSVGAttributes should be ok I think.)

Done.

[fs, 2015-05-13 13:20:51]

LGTM w/ the following addressed.

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
File LayoutTests/svg/repaint/svg-pathseglist-change.html (right):

https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
LayoutTests/svg/repaint/svg-pathseglist-change.html:5: window.onload =
runRepaintAndPixelTest;
On 2015/05/13 13:00:40, Shanmuga Pandi wrote:
> On 2015/05/13 12:35:20, fs wrote:
> > If this is indeed intended to be a repaint test, then I think just
> > runRepaintTest would suffice. If it's intended to be a ref-test then more
> > changes will be needed.
> 
> It is ref-test. So changed test case according to it.

Not sure svg/repaint/ is the best place for it (it's not actually a repaint bug
right?). svg/custom/ has lots of use-* tests, so maybe that's a better place
(and that prefix is certainly better.)

[Shanmuga Pandi, 2015-05-13 13:34:48]

On 2015/05/13 13:20:51, fs wrote:
> LGTM w/ the following addressed.
> 
>
https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
> File LayoutTests/svg/repaint/svg-pathseglist-change.html (right):
> 
>
https://codereview.chromium.org/1134733005/diff/1/LayoutTests/svg/repaint/svg...
> LayoutTests/svg/repaint/svg-pathseglist-change.html:5: window.onload =
> runRepaintAndPixelTest;
> On 2015/05/13 13:00:40, Shanmuga Pandi wrote:
> > On 2015/05/13 12:35:20, fs wrote:
> > > If this is indeed intended to be a repaint test, then I think just
> > > runRepaintTest would suffice. If it's intended to be a ref-test then more
> > > changes will be needed.
> > 
> > It is ref-test. So changed test case according to it.
> 
> Not sure svg/repaint/ is the best place for it (it's not actually a repaint
bug
> right?). svg/custom/ has lots of use-* tests, so maybe that's a better place
> (and that prefix is certainly better.)

Done. Thanks for review.

[Shanmuga Pandi, 2015-05-14 05:15:32]

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

[Shanmuga Pandi, 2015-05-14 05:15:32]

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/1134733005/#ps60001
(title: "Changed test case location and added prefix")

[commit-bot: I haz the power, 2015-05-14 05:15:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134733005/60001

[commit-bot: I haz the power, 2015-05-14 06:35:16]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195340