[webkit-reviews] review denied: [Bug 63283] URL references are completely broken in SVG : [Attachment 102371] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 63283: URL references are completely broken in SVG
https://bugs.webkit.org/show_bug.cgi?id=63283

Attachment 102371: Patch
https://bugs.webkit.org/attachment.cgi?id=102371&action=review

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


Almost there, still needs some love!

> Source/WebCore/css/CSSFontFaceSource.cpp:135
> +		   if (!m_externalSVGFontElement) {
> +		       String fragmentIdentifier;

This needs a fixme linked to the external refs bug report as well.

> Source/WebCore/css/SVGCSSStyleSelector.cpp:364
> +	      
svgstyle->setMarkerStartResource(SVGURIReference::fragmentIdentifierFromIRIStri
ng(s, m_element->document()));

You're still storing the fragment identifier in the RenderStyle, instead of the
pure iri string? I thought you wanted to change that in preparation for
external refs.

> Source/WebCore/rendering/svg/SVGResources.cpp:162
> -    id = SVGURIReference::getTarget(paintUri);
> +    id = SVGURIReference::fragmentIdentifierFromIRIString(paintUri,
document);
>      RenderSVGResourceContainer* container =
getRenderSVGResourceContainerById(document, id);

This code is wrong as well, every code path that just wants to extract the id
(just like in cSSFontFaceSource) is wrong.
You basically want to have getRenderSVGResourceContainerByIRIString, which
looks up directly an Element.

> Source/WebCore/svg/SVGPaint.cpp:168
> +	   return referenceId ==
SVGURIReference::fragmentIdentifierFromIRIString(m_uri, node() ?
node()->document() : 0);

Likely wrong as well, for the same reason as above.

> Source/WebCore/svg/SVGURIReference.cpp:48
> +String SVGURIReference::fragmentIdentifierFromIRIString(const String& url,
Document* document)

This method needs to be private and should have no users except
targetElementFromIRIString.