[webkit-reviews] review requested: [Bug 12499] External <use> xlink:href references do not work : [Attachment 130373] Proposed second part
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 6 06:59:58 PST 2012
Renata Hodovan <reni at webkit.org> has asked for review:
Bug 12499: External <use> xlink:href references do not work
https://bugs.webkit.org/show_bug.cgi?id=12499
Attachment 130373: Proposed second part
https://bugs.webkit.org/attachment.cgi?id=130373&action=review
------- Additional Comments from Renata Hodovan <reni at webkit.org>
> > LayoutTests/svg/custom/struct-use-recursion-02-t.svg:1
> > +<?xml version="1.0" encoding="UTF-8"?>
>
> Where did you grab these from? If its SVG 1.1 2nd edition testsuite, these
should go to svg/W3C-SVG-1.1-SE/
They were moved into a new W3C-SVG-1.2-Tiny directory because they come from
that testsuite.
> > LayoutTests/svg/dynamic-updates/SVGUseElement-dom-href2-attr-expected.txt:1
> > +CONSOLE MESSAGE: line 10: ReferenceError: Can't find variable: repaintTest
>
> Oops. This testcase doesn't work.
Sorry, updated.
> > Source/WebCore/svg/SVGUseElement.cpp:268
> > + SVGUseElement* correspondingUseElement =
static_cast<SVGUseElement*>(correspondingElement);
> > + if (correspondingUseElement->cachedDocumentIsStillLoading())
>
> Huh, how can you be sure its a SVGUseElement? Even if this is debug code,
this crashes :-) You need to check the hasTagName first, before casting.
Right. Fixed.
> > Source/WebCore/svg/SVGUseElement.cpp:746
> > + RefPtr<SVGSVGElement> svgElement =
SVGSVGElement::create(SVGNames::svgTag, document);
>
> How about you move the assert(document) into referencedDocument. Then you
could just do a s/document()/referencedDocument()/ and save introducing local
variables here.
Good idea. Done.
> > Source/WebCore/svg/SVGUseElement.cpp:921
> > + Element* parent = parentElement();
> > + if (!parent->renderer())
> > + return;
> > +
> > +
RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent->renderer(
));
>
> I asked this before I think: why the parent->renderer() only? Why not this?
> Is this needed at all, when doing invalidateShadowTree(). I would have
expected that this is enough. If not, then
markForLayoutAndParentResourceInvalidation(renderer()), but not
parent->renderer().
Really, we don't need it anymore. It's thrown off.
> > Source/WebCore/svg/SVGUseElement.cpp:934
> > + if (node->correspondingUseElement()) {
>
> if (SVGUseElement* use = node->correspondingUseElement()) {
> ..
> Saves calling it twice.
Done.
More information about the webkit-reviews
mailing list