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

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


Test results look very promising! I think there's a design flaw related to the
SVGDocument creation, see below:

> Source/WebCore/ChangeLog:10
> +

You already said it: the ChangeLog is too short :-)

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

Indentation.

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

You forgot about my last comment here: either conditionalize both
SVGDocumentType & Resource or none, not a mixture.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:420
> -    
> +

Unnecessary.

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:26
> +// Currently, we only need CachedSVGDocument for SVG documents.

This can be removed IMHO.

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:67
> +	   String decodedText;
> +	   decodedText = m_decoder->decode(data->data(), data->size());
> +	   decodedText += m_decoder->flush();
> +	   m_document->setContent(decodedText);

You should switch to StringBuilder here. I think I already mentioned that.

> Source/WebCore/page/ContentSecurityPolicy.h:72
> +    bool allowSVGDocumentFromSource(const KURL&) const;

No SVG conditional here? Why?

> Source/WebCore/svg/SVGURIReference.cpp:70
> +

The newline can go as well.

> Source/WebCore/svg/SVGUseElement.cpp:95
> -    
> +

I'd remove the lines alltogether.

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

This...

> Source/WebCore/svg/SVGUseElement.cpp:167
> +		       m_cachedDocument->setFrame(document()->frame());

Aha, that's the suspicious part you found suspicious. You store the originating
documents Frame in CachedSVGDocument::m_frame, and once
CachedSVGDocument::data() is called, you pass it to the newly created
SVGDocument.

Let's assume the approach is right in general, you still have an implementation
bug here:
There doesn't seem to be any place that clears this Frame again, that means
CachedSVGDocument will hold a RefPtr<Frame> keeping the original document
alive.

The approach can't be right though. Think of 1.svg referencing foo.svg, and
2.svg referncing foo.svg
When I open Safari, browse to 1.svg, a CachedSVGDocument of foo.svg is created
and its underlying SVGDocument is associated with 1.svg.
Next I browse to 2.svg, which now doesn't load foo.svg from the network, but
from the cache and sees the same CachedSVGDocument than 1.svg saw, but still
associated with 1.svg.

This implies you're always marrying the external document to the first
originating document that references it.
We need to discuss this on IRC I think - in the meanwhile we should check how
other parts of WebCore solve this for HTML.

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

... should go in an extra helper function, either static inline here, or on
SVGURIReference.

> Source/WebCore/svg/SVGUseElement.cpp:339
> +    SVGUseElement* correspondingUseElement =
static_cast<SVGUseElement*>(correspondingElement);

This needs a comment, I don't understand why its needed at present.

> Source/WebCore/svg/SVGUseElement.cpp:341
> +    if ((correspondingUseElement->m_cachedDocument
> +	       && correspondingUseElement->m_cachedDocument->isLoading()))

Outer braces can be omitted.

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

s/doc/document/ No abbrev. pls ;-)

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

Ditto.

> Source/WebCore/svg/SVGUseElement.cpp:552
> -    Element* targetElement =
SVGURIReference::targetElementFromIRIString(href(), document());
> +    String targetHref;
> +    if (!href().startsWith("#")) {
> +	   KURL url(document()->baseURI(), href());
> +	   targetHref = url.string();
> +    } else
> +	   targetHref = href();

This should be documented, and reorderered:
if (href().startsWith("#")) { .. } else { .. } to avoid one !.
Also it should make use of the aforementioned new helper function, to avoid
repeating the startsWith(..) logic in several places.

> Source/WebCore/svg/SVGUseElement.cpp:797
> +    Element* targetElement =
SVGURIReference::targetElementFromIRIString(use->href(), doc);

Why is the same logic regarding targetHref not needed here, as it is in
buildShadowAndInstanceTree? This should be documented.

> Source/WebCore/svg/SVGUseElement.cpp:877
> +	   if (use->m_cachedDocument && use->m_cachedDocument->isLoading())

I've already seen that above, should be refactored. Maybe a self-explainatory
name for the function can avoid a comment for this alltogether.
if (cachedDocumentIsStillLoading()) ?

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

s/doc/document/

> Source/WebCore/svg/SVGUseElement.cpp:883
> +	   Element* targetElement =
SVGURIReference::targetElementFromIRIString(use->href(), doc);

Again the use->href() question.

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

s/doc/document/

> Source/WebCore/svg/SVGUseElement.cpp:1015
> +		      && static_cast<SVGUseElement*>(target)->m_cachedDocument
> +		      &&
static_cast<SVGUseElement*>(target)->m_cachedDocument->isLoading()));

The function appears again :-)

> Source/WebCore/svg/SVGUseElement.cpp:1122
> +    if (renderer()) {

If we have no renderer, why do we need to notify the parent->renderer()?
Nothing changed in that case. I think you can early exit if (!renderer())

> Source/WebCore/svg/SVGUseElement.h:48
> +    ~SVGUseElement();

This needs to be virtual!

> Source/WebCore/svg/SVGUseElement.h:110
> +    static void updateContainerOffset(SVGElementInstance* targetInstance);

I don't think we need the targetInstance name here.