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

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.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 98496: Patch
https://bugs.webkit.org/attachment.cgi?id=98496&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98496&action=review

r- because of a missing DRT test.

> Source/WebCore/svg/SVGSVGElement.cpp:630
> +    bool hadUseCurrentView = m_useCurrentView;

no need to set it here. Variable is not used for if and else if case. And now
that I look at it, you don't need it at all

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

Seems to be a FIXME.

> Source/WebCore/svg/SVGSVGElement.cpp:634
> +	   if (!currentView()->parseViewSpec(name))

can we avoid the parsing of svgView( a second time in parseViewSpec?

> Source/WebCore/svg/SVGSVGElement.cpp:639
> +	       RefPtr<SVGViewElement> viewElement =
anchorNode->hasTagName(SVGNames::viewTag) ?
static_cast<SVGViewElement*>(anchorNode) : 0;

Do you need to ref count it? You call .get() multiple times afterwards.

> Source/WebCore/svg/SVGSVGElement.cpp:645
> +		   if (element->hasTagName(SVGNames::svgTag)) {
> +		       RefPtr<SVGSVGElement> svg =
static_cast<SVGSVGElement*>(element);
> +		       svg->inheritViewAttributes(viewElement.get());
> +		   }

We do nothing if the neares viewport element is not a svg-element? What else
can happen and shouldn't we do something in this case?

> Source/WebCore/svg/SVGSVGElement.cpp:653
> +    // FIXME: need to decide which <svg> to focus on, and zoom to that one

You take the first SVG parent, can you write this here?

> Source/WebCore/svg/SVGSVGElement.cpp:654
> +    // FIXME: need to actually "highlight" the viewTarget(s)

Wat does it mean? What is 's'?

> LayoutTests/ChangeLog:6
> +	   SVG1.1SE test linking-uri-01-b.svg fails
> +	   https://bugs.webkit.org/show_bug.cgi?id=63322

I can see it without the changes. So your test is definitely not enough to test
the new behavior, can add another DRT test?