[webkit-reviews] review denied: [Bug 63283] URL references are completely broken in SVG : [Attachment 102137] 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 102137: Patch
https://bugs.webkit.org/attachment.cgi?id=102137&action=review

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


Next round of comments :-)

> LayoutTests/svg/custom/invalid-url-reference.svg:3
> +  There should be no red

Better make this an inline comment, this way we can potentially share the
expected.png

> LayoutTests/svg/custom/invalid-url-reference.svg:16
> +    <rect x="0" y="0" width="100" height="100" style="fill:
url(http://www.example.com/something#orange_red)"/>

The name of this test is misleading, after all the url is valid! Can you come
up with a better name? (After all this is about not picking up the
same-document reference...)

> LayoutTests/svg/custom/linking-base-external-reference.xhtml:4
> +  There should be no red

Ditto.

> LayoutTests/svg/custom/linking-base-external-reference.xhtml:11
> +				    stop-opacity:1"/>

The stop opacity is useless in all cases just whip it everywhere.

> LayoutTests/svg/custom/uri-reference-handling.svg:12
> +    <stop offset="0%" style="stop-color:rgb(255,0, 0);"/>

How about using red everywhere instead of this rgb(255,0,0) ?

> LayoutTests/svg/custom/uri-reference-handling.svg:18
> +<rect width="50" height="50"
fill="url(../custom/uri-reference-handling.svg#green)"/>
> +<rect x="50" width="50" height="50"
fill="url(http://www.example.org/test#red) green"/>
> +<rect id="rect3" y="50" width="50" height="50" fill="red"/>
> +<rect x="50" y="50" width="50" height="50" fill="url(no_fragment_identifier)
green"/>

Can you make each of them have a padding of 10px?
0x0 - 50x50
60x60 - 110x110
...

Easier to see that the final image is composed of several rects.

> Source/WebCore/ChangeLog:8
> +	   Change getTarget to be more strict about iri resolving. In
particular, test for same-document or

s/getTarget/SVGURIReference::getTarget/

> Source/WebCore/ChangeLog:11
> +	   external referencing, the latter case we do not handle for now.
Accept as same-document if the
> +	   iri when absolute equals the document url, or if the iri is relative
when the relative part combined
> +	   with the base uri equals the document url.

If the iri when? Please rephrase :-)

> Source/WebCore/ChangeLog:12
> +	   For convenience a method is added to get the target of the iri, if
found.

s/to get the target/to lookup the element/

> Source/WebCore/ChangeLog:14
> +	   funcIRI handling is not needed since CSS parser strips this for us.
> +

If you mention funcIRI, you should include the definition of SVG iris,
otherwhise this is confusing.

> Source/WebCore/css/CSSCursorImageValue.cpp:75
> +	   if (SVGCursorElement* cursorElement =
resourceReferencedByCursorElement(SVGURIReference::fragmentIdentifierFromIRIStr
ing(url, referencedElement->document()), referencedElement->treeScope()))

Can't you encapsulate this call right within resourceReferencedByCursorElement,
to avoid repeating this? (you just have to pass the Element instead of the
TreeScope to that method).

> Source/WebCore/css/CSSFontFaceSource.cpp:135
> +		       m_externalSVGFontElement =
m_font->getSVGFontById(SVGURIReference::fragmentIdentifierFromIRIString(m_strin
g, fontSelector->m_document, AllowExternalReferences));

Why are external refs allowed here, and not in cursors? If there's a reason it
should be mentioned in the ChangeLog and in a comment.

> Source/WebCore/css/CSSFontSelector.h:70
> +    friend class CSSFontFaceSource;

How about adding a public Document* accessor instead of adding friendship?

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

External refs are forbidden here?
I think we should change the default to be "AllowExternalReferences", that
would make the exceptions easier to spot (aka. those who disallow external
refs).

Can you make a list of forbidden ones, with spec refs? :-)

> Source/WebCore/svg/SVGURIReference.cpp:58
> +    if ((mode == AllowExternalReferences && kurl.isValid()) ||
equalIgnoringFragmentIdentifier(kurl, document->url()))

How about you add a comment here, as in the ChangeLog, what these checks are
for.

> Source/WebCore/svg/SVGURIReference.cpp:70
> +    // FIXME: handle external references properly

Handle external references.
We're supposed to use full phrases in comments.
Ideally you'd include a bug link here, to the relevant bug.

> Source/WebCore/svg/SVGURIReference.h:48
> +    static String fragmentIdentifierFromIRIString(const String&, Document*,
ExtractTargetMode = ForbidExternalReferences);
> +    static Element* targetElementFromIRIString(const String&, Document*,
String* = 0, ExtractTargetMode = ForbidExternalReferences);

I think we should assure that we're using the right default here from the
beginning.
Even if we don't support external references right now...