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

[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 131597: Proposed second part
https://bugs.webkit.org/attachment.cgi?id=131597&action=review

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


Looks much better, still some minor things to clean up:

>
LayoutTests/platform/mac-lion/svg/W3C-SVG-1.2-Tiny/struct-use-recursion-01-t-ex
pected.txt:1
> +layer at (0,0) size 800x600

The location of these test results is wrong. Lion results should be in
platform/mac/svg, not in platform/mac-lion/svg.
Also a ChangeLog seems to be missing for LayoutTests.

> LayoutTests/svg/custom/use-referencing-an-image-expected.svg:4
> +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%"
height="100%"
> +  viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg"
> +  xmlns:xlink="http://www.w3.org/1999/xlink"
xmlns:xe="http://www.w3.org/2001/xml-events">

You should strip off all unneeded variables, like xmlns:xlink, xmlns:xe,
width/height xml:id/baseProfile/version here.

> LayoutTests/svg/custom/use-referencing-an-image.svg:3
> +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%"
height="100%"
> +  viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg"

Ditto.

> LayoutTests/svg/custom/use-referencing-indirectly-itself-expected.svg:3
> +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%"
height="100%"
> +  viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg"

Ditto.

> LayoutTests/svg/custom/use-referencing-indirectly-itself-expected.svg:6
> +  <rect id="greenRect" width="100" height="100" fill="green"/>

id is not need here, please make those as minimal as possible.

> LayoutTests/svg/custom/use-referencing-indirectly-itself.svg:4
> +  viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg"
> +  xmlns:xlink="http://www.w3.org/1999/xlink"
xmlns:xe="http://www.w3.org/2001/xml-events">

Ditto.

> LayoutTests/svg/custom/use-referencing-itself-expected.svg:3
> +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%"
height="100%"
> +  viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg"

Ditto.

> LayoutTests/svg/custom/use-referencing-itself.svg:4
> +  viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg"
> +  xmlns:xlink="http://www.w3.org/1999/xlink"
xmlns:xe="http://www.w3.org/2001/xml-events">

Ditto.

> Source/WebCore/platform/KURL.cpp:1391
> +    if (a.m_queryEnd != b.m_queryEnd) {
> +	   return false;}

Please revert this.

> Source/WebCore/svg/SVGURIReference.cpp:85
> +    String id = "";

Do you need an empty string here? If so use String id = emptyString(); But I
don't think that's need here, String id; should do it.

> Source/WebCore/svg/SVGURIReference.cpp:106
> +	   return 0; // Non-existing external resource

trailing period missing.

> Source/WebCore/svg/SVGURIReference.h:47
> +	   if (!uri.startsWith("#")) {

Reverse the logic here.
if (uri,startsWith('#'))
    return false;

> Source/WebCore/svg/SVGUseElement.cpp:231
> +	   if (isExternalURIReference(href(), document())) {

I'd cache the result of isExternalURIREference in a boolean, to avoid calling
it twice.