[webkit-reviews] review denied: [Bug 63283] URL references are completely broken in SVG : [Attachment 102137] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 28 11:21:22 PDT 2011
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...
More information about the webkit-reviews
mailing list