[webkit-reviews] review denied: [Bug 12499] External <use> xlink:href references do not work : [Attachment 129471] 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 129471: Proposed second part
https://bugs.webkit.org/attachment.cgi?id=129471&action=review

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


Nice patch, still another iteration needed. I've also noticed that some of the
svg/batik/ tests, don't include the correct CSS style sheet, that's why the
text is so large in eg. textSTyles-expected.png. You could fix these in
one-shot, as the tests already need rebaselining anyways.

> Source/WebCore/svg/SVGUseElement.cpp:90
> +    , m_cachedDocument(0)

This doesn't seem to be needed at all, is it?

> Source/WebCore/svg/SVGUseElement.cpp:164
> +	       if (isExternalURIReference(href())) {

You need to test transitions from xlink:href pointing to internal -> external
resource, and the other way round.
Currently if you change from an external href to an internal, m_cachedDocument
won't be released, if I read the code correctly.
This is important to get right!

Also if the <use> element itself gets removed from the tree
(removedFromDocument) shouldn't we release the document?
I'm thinking of a <use> element which loads a slooow external resource (say
over 1mb large). Now if we remove the <use> from the document, while the
external document is still loading, we could abort the load, and release
m_cachedDocument, no?
Whatever you decide, write a test for it :-)

> Source/WebCore/svg/SVGUseElement.cpp:647
> +	   if (use->cachedDocumentIsStillLoading())
> +	       return;

Shouldn't we detect this earlier? We end up expanding parts of the tree - and
then throw away the whole shadow tree, once the external document loaded.
I'd rather ASSERT here that the document is not loading, and detect this
earlier. (Walk whole instance tree to find instances corresponding to use
elements, and check if those use elements are still loading..)

> Source/WebCore/svg/SVGUseElement.cpp:770
> +		  || (target->nodeName() == SVGNames::useTag &&
(static_cast<SVGUseElement*>(target))->cachedDocumentIsStillLoading()));

This assertion change could go away, if you'd stop expanding parts of the tree
only.

> Source/WebCore/svg/SVGUseElement.cpp:879
> +    renderer()->updateFromElement();

This shouldn't be necessary anymore.

> Source/WebCore/svg/SVGUseElement.cpp:882
> +    ASSERT(parent);

No need to check that, you already tested inDocument().
This is also not 100% correct, you need to mark the <use> itself as needing
layout.
So RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer()) is
the way to go here.