[webkit-reviews] review denied: [Bug 63322] SVG1.1SE test linking-uri-01-b.svg fails : [Attachment 98545] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 63322: SVG1.1SE test linking-uri-01-b.svg fails
https://bugs.webkit.org/show_bug.cgi?id=63322

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98545&action=review

Next rounds of comments ;-)

> Source/WebCore/page/FrameView.cpp:1464
> +	   SVGSVGElement* svg =
static_cast<SVGDocument*>(m_frame->document())->rootElement();

Out of curiosity: can this ever be null? Suppose we had an empty foo.svg
document and load that? Can you try it?

> Source/WebCore/svg/SVGAElement.cpp:217
> +		   // only allow navigation to internal <view> anchors

s/only/Only/ add trailing ".".

> Source/WebCore/svg/SVGSVGElement.cpp:631
> +	   // FIXME: We need to parse the xpointer reference here

Ideally you directly create a bug report, and link it here.
FIXME: XPointer references are ignored (https://bugs.webkit.org/...

Can you add a return here?

> Source/WebCore/svg/SVGSVGElement.cpp:632
> +    } else if (name.startsWith("svgView(")) {

And remove the } else if, make it a new if ( section.

> Source/WebCore/svg/SVGSVGElement.cpp:635
> +	   setUseCurrentView(true);

Hm, there are other places in SVGSVGElement that mutate the UseCurrentView
value (the inheritViewAttribues method). How does this interfer? Is this
related at all?

> Source/WebCore/svg/SVGSVGElement.cpp:645
> +	       }

Also add a return; after this line to avoid ...

> Source/WebCore/svg/SVGSVGElement.cpp:646
> +	   } else if (m_useCurrentView) {

.. this else if.

> Source/WebCore/svg/SVGSVGElement.cpp:654
> +    // FIXME: need to decide which <svg> to focus on, and zoom to that one
> +    // FIXME: need to actually "highlight" the viewTarget(s)

Can you make those valid english phrases, start with capital letter and end
with punctation.

> Source/WebCore/svg/SVGSVGElement.h:123
> +    void handleAnchorScroll(const String& name, Element* anchorNode);

The method name is very unfortunate. What does handle mean in that context?
Maybe "scrollToAnchor" is better?