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

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


Looks good in general! Still needs an iteration, as ToT changed SVGUseElements
shadow tree construction.

> Source/WebCore/ChangeLog:20
> +	   of the size of that test refactor they will be commited in a
follow-up patch.

Okay, once the code is fine, you should still upload one big final patch, that
shows that you modified the chromium expectations file correctly (aka. cr-linux
turns green)

> Source/WebCore/svg/SVGURIReference.cpp:59
> +    else
> +	   base = start ? KURL(document->baseURI(), url.substring(0, start)) :
document->baseURI();

I'd prefer:
else if (start) {
    base = KURL(d...
else {
   base = document->baseURI();

Isn't there a generic function for that? Just guessing, but we should verify
that.

> Source/WebCore/svg/SVGUseElement.cpp:1109
> +   
static_cast<RenderSVGShadowTreeRootContainer*>(renderer())->markShadowTreeForRe
creation();
> +    renderer()->updateFromElement();

This needs to be updated, within the new SVGUseElement implementation that
landed some hours ago.
SVGUseElement is now properly integrated within the standard ShadowTree.

You'll likely find that your code needs to be changed now, but the attachment
and rebuilding should be much more straight-forward & logical.