[webkit-reviews] review denied: [Bug 15162] hit testing does not respect clip paths : [Attachment 57681] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 15162: hit testing does not respect clip paths
https://bugs.webkit.org/show_bug.cgi?id=15162

Attachment 57681: Patch
https://bugs.webkit.org/attachment.cgi?id=57681&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
In general that looks fine, though I'd like to see the PE_FILL stuff
encapsulated. How about passing style()->pointerEvents() and HitTestRequest ot
PointerEventsHitRules? It could either grab the style()->pointerEvents() or set
it to PE_FILL when request.svgClipContent() is true? Several other comments
leading to r-:

WebCore/rendering/RenderPath.h:50
 +	bool fillContains(const FloatPoint&, bool requiresFill = true, WindRule
= RULE_NONZERO) const;
Please use a parameter name here, looks odd.

WebCore/rendering/RenderSVGResourceClipper.cpp:289
 +	    IntPoint hitPoint = IntPoint();
This is unncessary, no need to call the assignment operator.

WebCore/rendering/RenderSVGResourceClipper.h:58
 +	bool hitTestClipContent(const FloatRect&, FloatPoint);
Please pass FloatPoint as const-reference as well.

LayoutTests/svg/dynamic-updates/script-tests/SVGClipPath-influences-hitTesting.
js:26
 +  var rectElementFG = createSVGElement("rect");
Why not name it foreground/background instead of using abbrevations?