[webkit-reviews] review denied: [Bug 12499] External <use> xlink:href references do not work : [Attachment 130341] Proposed second part

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Renata Hodovan
<reni at webkit.org>'s request for review:
Bug 12499: External <use> xlink:href references do not work
https://bugs.webkit.org/show_bug.cgi?id=12499

Attachment 130341: Proposed second part
https://bugs.webkit.org/attachment.cgi?id=130341&action=review

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


Almost there, next round of comments:

> 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/

> 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.

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href1-attr.js:1
> +// [Name] SVGFEUseElement-dom-href1-attr.js

Description is wrong.

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href1-attr.js:4
> +description("Tests dynamic updates of the 'href' attribute of the
SVGFEUseElement object")

Ditto.

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href1-attr.js:32

> +    useElement.setAttributeNS(xlinkNS, "xlink:href",
"../custom/resources/rgb.svg#R");

How about referencing #G? Green is better as pass color, than red ;-)

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:1
> +// [Name] SVGFEUseElement-dom-href1-attr.js

Description is wrong.

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:4
> +description("Tests dynamic updates of the 'href' attribute of the
SVGFEUseElement object")

Ditto.

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:9
> +rootSVGElement.appendChild(
> +defsElement);

Typo.

>
LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:22

> +rectElement.setAttribute("fill", "#408067");

Why not green?

> Source/WebCore/ChangeLog:16
> +	   SVGURIReference::targetElementFromIRIString() also need to be
extended. The creation
> +	   of baseURI should be based on the referenced document's URL instead
of the actual one and

"The baseURI computation needs to take the referenced documents URL into
account, instead of the current documents." ?

> Source/WebCore/svg/SVGUseElement.cpp:236
> +	   if (m_cachedDocument && !isExternalURIReference(href())) {

Aha, you handle the cleanup here, when switching from internal <-> external.
That seems fine.

> Source/WebCore/svg/SVGUseElement.cpp:246
> +	       || SVGExternalResourcesRequired::isKnownAttribute(attrName)) {

This should be left out. Whitespace only change.

> 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.

> 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.

> 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().

> Source/WebCore/svg/SVGUseElement.cpp:934
> +	   if (node->correspondingUseElement()) {

if (SVGUseElement* use = node->correspondingUseElement()) {
..
Saves calling it twice.