Implement SVGGeometryElement

Implement SVGGeometryElement

Implement SVGGeometryElement as described in https://svgwg.org/svg2-draft/types.html#InterfaceSVGGeometryElement

Use existing nodeAtPoint functionality from RenderSVGShape to implement the isPointInFill/Stroke methods of the interface.

Because of this feature update svg2-inheritance.html and add tests for it in svg-ellipse.xhtml.

TESTS=svg/dom/svg2-inheritance.html, svg/hittest/svg-ellipse.xhtml
BUG=315802
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161828

[rwlbuis, 2013-11-06 20:03:15]

First stab at this. I couldn't find anything on isPointInFill/Stroke behavior
combined with clipping, so clipping is ignored for now.

[Inactive, 2013-11-06 20:10:34]

Would be nice to add Samsung copyright for new files :)

[rwlbuis, 2013-11-06 20:38:43]

Fixed copyrights and added krit as reviewer.

[pdr., 2013-11-09 00:20:08]

Overall an excellent change. Mostly nits but lets do one more round.

Can you update the change description to point to the spec and describe in a
little more detail what this change does?

https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/dom/svg2-...
File LayoutTests/svg/dom/svg2-inheritance-expected.txt (right):

https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/dom/svg2-...
LayoutTests/svg/dom/svg2-inheritance-expected.txt:31: FAIL SVGClipPathElement
should inherit SVGElement but got SVGGraphicsElement instead
Can we pull the SVGClipPathElement changes into a separate patch?

https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/hittest/s...
File LayoutTests/svg/hittest/svg-ellipse.xhtml (right):

https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/hittest/s...
LayoutTests/svg/hittest/svg-ellipse.xhtml:59: resultString += "Testing
isPointInFill/isPointInStroke\n";
I don't think we have sufficient test coverage of isPointInFill/isPointInStroke
with the various values of pointer-events. Can you copy this test and replace
the elementFromPoint() checks with isPointInFill and isPointInStroke checks?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
File Source/core/svg/SVGGeometryElement.cpp (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:44: // FIXME: Eventually we should
support isPointInFill for detached elements.
Is this comment correct? I'm not sure we can check isPointInFill for detached
elements.

Should this comment say "FIXME: Eventually we should support isPointInFill for
display:none elements"?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:45: if (!renderer() ||
!renderer()->isSVGShape())
Can we replace isSVGShape with isSVGGeometry and rename RenderSVGShape to
RenderSVGGeometry?
(Possibly as a followup?)

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:48: HitTestRequest
request(HitTestRequest::SVGClipContent);
Why is SVGClipContent used here?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:49: PointerEventsHitRules
hitRules(PointerEventsHitRules::SVG_PATH_HITTESTING, request,
renderer()->style()->pointerEvents());
Should we rename "PointerEventsHitRules::SVG_PATH_HITTESTING" to
"PointerEventsHitRules::SVG_GEOMETRY_HITTESTING"?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:52: return
static_cast<RenderSVGShape*>(renderer())->nodeAtFloatPointInternal(request,
point, hitRules);
Danger danger!
Please use toSVGShape() instead of a static_cast

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
File Source/core/svg/SVGGeometryElement.h (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.h:30: virtual ~SVGGeometryElement();
Is this needed?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
File Source/core/svg/SVGGeometryElement.idl (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.idl:32: boolean   isPointInFill(SVGPoint
point);
nit: extra spaces

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.idl:33: boolean   isPointInStroke(SVGPoint
point);
Can you add a test of checking isPointInStroke() and isPointInFill() making sure
an exception is thrown if no point is provided?

[krit, 2013-11-09 02:20:17]

Just a briefly look

https://codereview.chromium.org/62943002/diff/50001/Source/core/rendering/svg...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/rendering/svg...
Source/core/rendering/svg/RenderSVGShape.h:82: friend class SVGGeometryElement;
The new IDL was designed to avoid things like that. Could you please elaborate
why we still need to "friend class" SVGGeometryElement please?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
File Source/core/svg/SVGGeometryElement.cpp (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:5: * modify it under the terms of the GNU
Library General Public
Samsung is using LGPL and not BSD? Interesting.

[rwlbuis, 2013-11-11 16:54:58]

Address krit's comments.

https://codereview.chromium.org/62943002/diff/50001/Source/core/rendering/svg...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/rendering/svg...
Source/core/rendering/svg/RenderSVGShape.h:82: friend class SVGGeometryElement;
On 2013/11/09 02:20:18, krit wrote:
> The new IDL was designed to avoid things like that. Could you please elaborate
> why we still need to "friend class" SVGGeometryElement please?

I just did not want to expose nodeAtFloatPointInternal, but I guess it may be
better then adding the friend decl. Maybe I need to rename Internal to Helper or
something?

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
File Source/core/svg/SVGGeometryElement.cpp (right):

https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
Source/core/svg/SVGGeometryElement.cpp:5: * modify it under the terms of the GNU
Library General Public
On 2013/11/09 02:20:18, krit wrote:
> Samsung is using LGPL and not BSD? Interesting.

I guess I copied the wrong file, will fix.

[rwlbuis, 2013-11-11 16:58:41]

On 2013/11/09 00:20:08, pdr wrote:
> Overall an excellent change. Mostly nits but lets do one more round.
> 
> Can you update the change description to point to the spec and describe in a
> little more detail what this change does?

Done.

>
https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/dom/svg2-...
> File LayoutTests/svg/dom/svg2-inheritance-expected.txt (right):
> 
>
https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/dom/svg2-...
> LayoutTests/svg/dom/svg2-inheritance-expected.txt:31: FAIL SVGClipPathElement
> should inherit SVGElement but got SVGGraphicsElement instead
> Can we pull the SVGClipPathElement changes into a separate patch?

Sure I'll remove that part.

>
https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/hittest/s...
> File LayoutTests/svg/hittest/svg-ellipse.xhtml (right):
> 
>
https://codereview.chromium.org/62943002/diff/50001/LayoutTests/svg/hittest/s...
> LayoutTests/svg/hittest/svg-ellipse.xhtml:59: resultString += "Testing
> isPointInFill/isPointInStroke\n";
> I don't think we have sufficient test coverage of
isPointInFill/isPointInStroke
> with the various values of pointer-events. Can you copy this test and replace
> the elementFromPoint() checks with isPointInFill and isPointInStroke checks?

Sure.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> File Source/core/svg/SVGGeometryElement.cpp (right):
> 
>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.cpp:44: // FIXME: Eventually we should
> support isPointInFill for detached elements.
> Is this comment correct? I'm not sure we can check isPointInFill for detached
> elements.
> 
> Should this comment say "FIXME: Eventually we should support isPointInFill for
> display:none elements"?

I think both are valid, the detached part means the opposite of attached, so
either display:none or rendering tree removal I guess. Anyway display:none is
clearer so I'll change it to that.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.cpp:45: if (!renderer() ||
> !renderer()->isSVGShape())
> Can we replace isSVGShape with isSVGGeometry and rename RenderSVGShape to
> RenderSVGGeometry?
> (Possibly as a followup?)

Sure, as a followup.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.cpp:48: HitTestRequest
> request(HitTestRequest::SVGClipContent);
> Why is SVGClipContent used here?

Tbh I am not sure how clipping is involved, it is not mentioned at all in the
spec so maybe it should be ignored?

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.cpp:49: PointerEventsHitRules
> hitRules(PointerEventsHitRules::SVG_PATH_HITTESTING, request,
> renderer()->style()->pointerEvents());
> Should we rename "PointerEventsHitRules::SVG_PATH_HITTESTING" to
> "PointerEventsHitRules::SVG_GEOMETRY_HITTESTING"?

Sounds good.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.cpp:52: return
> static_cast<RenderSVGShape*>(renderer())->nodeAtFloatPointInternal(request,
> point, hitRules);
> Danger danger!
> Please use toSVGShape() instead of a static_cast

Will fix.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> File Source/core/svg/SVGGeometryElement.h (right):
> 
>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.h:30: virtual ~SVGGeometryElement();
> Is this needed?

No, will remove.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> File Source/core/svg/SVGGeometryElement.idl (right):
> 
>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.idl:32: boolean   isPointInFill(SVGPoint
> point);
> nit: extra spaces

Fixed.

>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> Source/core/svg/SVGGeometryElement.idl:33: boolean   isPointInStroke(SVGPoint
> point);
> Can you add a test of checking isPointInStroke() and isPointInFill() making
sure
> an exception is thrown if no point is provided?

The idl does not specify an exception?

[rwlbuis, 2013-11-11 17:51:54]

On 2013/11/11 16:58:41, rwlbuis wrote:
>
https://codereview.chromium.org/62943002/diff/50001/Source/core/svg/SVGGeomet...
> > Source/core/svg/SVGGeometryElement.idl:33: boolean  
isPointInStroke(SVGPoint
> > point);
> > Can you add a test of checking isPointInStroke() and isPointInFill() making
> sure
> > an exception is thrown if no point is provided?
> 
> The idl does not specify an exception?

Nm, I assume now you meant a js exception, not an idl induced exception.

[rwlbuis, 2013-11-11 20:42:06]

Hopefully this patch addresses all remarks!

[pdr., 2013-11-11 21:10:08]

On 2013/11/11 20:42:06, rwlbuis wrote:
> Hopefully this patch addresses all remarks!

LGTM!

[commit-bot: I haz the power, 2013-11-11 21:14:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/62943002/200001

[commit-bot: I haz the power, 2013-11-11 23:48:10]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[commit-bot: I haz the power, 2013-11-12 00:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/62943002/200001

[commit-bot: I haz the power, 2013-11-12 00:59:03]

Failed to trigger a try job on linux_blink_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-11-12 00:59:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/62943002/410001

[commit-bot: I haz the power, 2013-11-12 03:14:13]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-12 03:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/62943002/410001

[commit-bot: I haz the power, 2013-11-12 04:46:39]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-12 14:31:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/62943002/620001

[commit-bot: I haz the power, 2013-11-12 17:16:20]

Change committed as 161828