[webkit-reviews] review canceled: [Bug 12499] External <use> xlink:href references do not work : [Attachment 124547] Proposed patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has canceled 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 124547: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=124547&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124547&action=review


This looks much better!  Thanks.  I have a few comments, but there's mostly
just me trying to understand.  The only real important one is to use
response().url() rather than url() when creating the document.	I think we're
ready to split this up into manageable pieces and land it.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:370
> +#if ENABLE(SVG)
> +    case CachedResource::SVGDocumentResource:
> +#endif

Treating this as an image seems ok.  We should confirm with the working group
(but we don't need to block this patch on that).  Would you be willing to email
public-webappsec about this question?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:65
> +    if (!allDataReceived)
> +	   return;

So, there's no incremental rendering.

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:67
> +    if (data) {

You've checked that |data| accumulates (and isn't just the last block)?  I
don't remember off-hand.

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:92
> +void CachedSVGDocument::createDocument()
> +{
> +    if (m_document)
> +	   return;
> +
> +    m_document = SVGDocument::create(0, url());
> +}

Why do you create this document with this function?  I would have expected you
to create the document right before calling m_document->setContent().

Also, this should be response().url() so that we get the final URL, which is
another reason we need to wait until after we've gotten a response before
creating the document.

> Source/WebCore/loader/cache/CachedSVGDocument.h:38
> +    CachedSVGDocument(const ResourceRequest&);

Please add the keyword explicit to one-argument constructors.

> Source/WebCore/page/ContentSecurityPolicy.h:109
> +

This change seem spurious.

> Source/WebCore/platform/network/chromium/ResourceRequest.h:49
> +	       TargetIsSVGResource,

I bet there's a matching enum in the Chromium WebKit API.  I'm surprised there
isn't a COMPILE_ASSERT failures with this patch.

> Source/WebCore/svg/SVGUseElement.cpp:107
> +	  m_cachedDocument->removeClient(this);

Bad indent.