[webkit-reviews] review denied: [Bug 12499] External <use> xlink:href references do not work : [Attachment 120886] Draft patch

[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 120886: Draft patch
https://bugs.webkit.org/attachment.cgi?id=120886&action=review

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


Thanks Reni for taking this over, it obviously needs lots of new testcases, as
Dirk already pointed out, but here's a first round of code comments:

> LayoutTests/ChangeLog:8
> +	   * svg/custom/resources/rgb.svg: Wrapped <rect> elements in <g> and
added id attributes.

This will affect lots of more testcases, all test cases in svg/batik import
external files, and thus will change.
I'd like to see the full set of changes, even when generated on non-mac
platforms, to see how it affects those tests.

> Source/WebCore/ChangeLog:9
> +	   Support external references on <use> by introducing
CachedSVGDocument,
> +	   which handles document subresources.

In a final version of this patch, this ChangeLog needs to be much more verbose,
as this is an important new feature.

> Source/WebCore/GNUmakefile.list.am:2456
> +	   Source/WebCore/loader/cache/CachedSVGDocument.cpp \

Formatting.

> Source/WebCore/loader/cache/CachedResourceClient.h:42
> +	   SVGDocumentType,

Why is this not conditionalized, or rather why is it done in Cachedresource.h
at all? It should be consistent.

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:41
> +    setAccept("image/svg+xml");

Hm, can you avoid the temp strings. Or is this a char*, I didn't check?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:80
> +    String decodedText;
> +    if (m_data) {
> +	   decodedText = m_decoder->decode(m_data->data(), m_data->size());
> +	   decodedText += m_decoder->flush();
> +    }
> +

That can be rewritten using StringBuilder, to avoid reallocations when using
operator+=.
if (m_data) {
   StringBuilder builder;
   builder->append(....);
   m_document->setContent(builder.toString());
} else
    m_document->setContent(emptyString());

> Source/WebCore/svg/SVGUseElement.cpp:155
> +	       if (!href().startsWith("#")) {

There should be better ways to detect that, or at least a central
implementation like "static inline bool isExternalURIReference", maybe even in
SVGURIReference.

> Source/WebCore/svg/SVGUseElement.cpp:193
> +    if (href().startsWith("#"))

Ditto.

> Source/WebCore/svg/SVGUseElement.cpp:482
> +    Document* doc = referencedDocument();

s/doc/document.

> Source/WebCore/svg/SVGUseElement.cpp:543
> +//	 targetElement = doc->getElementById(m_resourceId);

Leftover?

> Source/WebCore/svg/SVGUseElement.cpp:866
> +	   Document* doc = referencedDocument();

s/doc/document/

> Source/WebCore/svg/SVGUseElement.cpp:925
> +	   Document* doc = referencedDocument();

Ditto.

> Source/WebCore/svg/SVGUseElement.cpp:1100
> +void SVGUseElement::setSVGDocument()

This needs a better name, here and in CachedSVGDocument.