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

[bugzilla-daemon at webkit.org]

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

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


>> Source/WebCore/loader/cache/CachedSVGDocument.cpp:65
>> +	    decodedText.append(m_decoder->flush());
> 
> Why do we always flush?  Usually we flush only at the end.

Why do we always flush?  Usually we flush only at the end.

>> Source/WebCore/loader/cache/CachedSVGDocument.cpp:90
>> +	m_document = SVGDocument::create(0, url());
>> +	m_document->setSecurityOrigin(SecurityOrigin::create(parentUrl));
> 
> Woah there.  You shouldn't need to call setSecurityOrigin.  That's bad news
bears.
> 
> Also, is url() the URL that we request or the one that we finally get the
document from (e.g., at the end of the redirect chain)?  It's very important
that we set the URL of the document to the one at the end of the redirection
chain.

Woah there.  You shouldn't need to call setSecurityOrigin.  That's bad news
bears.

Also, is url() the URL that we request or the one that we finally get the
document from (e.g., at the end of the redirect chain)?  It's very important
that we set the URL of the document to the one at the end of the redirection
chain.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:686
>> +bool ContentSecurityPolicy::allowSVGDocumentFromSource(const KURL& url)
const
>> +{
>> +	DEFINE_STATIC_LOCAL(String, type, ("svg"));
>> +	return
checkSourceAndReportViolation(operativeDirective(m_svgSrc.get()), url, type);
>> +}
> 
> We shouldn't just make up new Content-Security-Policy directives.  Probably
this should go under another directive...  We can ask the working group which
one.

We shouldn't just make up new Content-Security-Policy directives.  Probably
this should go under another directive...  We can ask the working group which
one.

>> Source/WebCore/svg/SVGUseElement.cpp:163
>> +		    KURL url(document()->baseURI(), href());
> 
> Why not just use document()->completeURL(href()) ?

Why not just use document()->completeURL(href()) ?

>> Source/WebCore/svg/SVGUseElement.cpp:168
>> +			   
m_cachedDocument->createDocument(document()->url());
> 
> You shouldn't need to pass in document()->url() here.  A cached document can
be re-used in many contexts.  You shouldn't need to tell it about the context
in which it is used.

You shouldn't need to pass in document()->url() here.  A cached document can be
re-used in many contexts.  You shouldn't need to tell it about the context in
which it is used.

>> Source/WebCore/svg/SVGUseElement.cpp:205
>> +	if (m_cachedDocument && m_cachedDocument->isLoaded())
>> +	    return m_cachedDocument->document();
> 
> Is this document mutable?  What happens when many SVGUseElements point to the
same CachedSVGDocument ?

Is this document mutable?  What happens when many SVGUseElements point to the
same CachedSVGDocument ?

>> Source/WebCore/svg/SVGUseElement.h:134
>> +	CachedSVGDocument* m_cachedDocument;
> 
> Should this be a CachedResourceHandle?

Should this be a CachedResourceHandle?