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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted 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 98590: Patch
https://bugs.webkit.org/attachment.cgi?id=98590&action=review

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

Looks almost good, r=me with one condition: if it turns out my worries about
markForLayoutAndParentResourceInvalidation are wrong, and this is the only
place where this is needed, then go ahead and land it otherwhise let's discuss
:-)

> Source/WebCore/ChangeLog:11
> +	   Test: svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg

The other new test is missing here.

> Source/WebCore/ChangeLog:18
> +	   * page/FrameView.cpp: delegate to setupInitialView
> +	   (WebCore::FrameView::scrollToAnchor):
> +	   * svg/SVGAElement.cpp: allow navigating to internal <view> targets
> +	   (WebCore::SVGAElement::defaultEventHandler):
> +	   * svg/SVGSVGElement.cpp:
> +	   (WebCore::SVGSVGElement::setupInitialView): initialize current view
depending on anchor

Please use proper sentences as well here.

> Source/WebCore/page/FrameView.cpp:1460
> +    m_frame->document()->setCSSTarget(anchorNode); // Setting to null will
clear the current target.

You can move the comment to its own line before that statement - I think this
is preferred style.

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

You can write if (SVGSVGElement* svg = ...) { here to save another line :-)

> Source/WebCore/svg/SVGSVGElement.cpp:628
> +void SVGSVGElement::setupInitialView(const String& name, Element*
anchorNode)

Sorry for not spotting it ealier but "name" is not very descriptive.
"viewIdentifier" ? "viewTarget" ? I'm sure you can come up with a better name.

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

Add return; here.

> Source/WebCore/svg/SVGSVGElement.cpp:653
> +    } else if (hadUseCurrentView) {
> +	   currentView()->setTransform(emptyString());

You can save this branch by using if (!hadUseCurrentView) early exit.

> Source/WebCore/svg/SVGSVGElement.cpp:655
> +	   if (RenderObject* object = renderer())
> +	      
RenderSVGResource::markForLayoutAndParentResourceInvalidation(object);

Why do you only need this invalidation here, and not for the other branches?