[webkit-reviews] review granted: [Bug 63322] SVG1.1SE test linking-uri-01-b.svg fails : [Attachment 98590] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jun 25 09:48:21 PDT 2011
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?
More information about the webkit-reviews
mailing list