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

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


In general this looks fine, some comments though:

> Source/WebCore/svg/SVGURIReference.cpp:47
> -String SVGURIReference::getTarget(const String& url)
> +String SVGURIReference::getTarget(const String& funcUri, Document* document,
bool allowExternal)

How about naming it, extractTargetURI, if you're at it?
A boolean parameter like "allowExternal" should be avoided. Use an enum.
enum ExtractTargetMode {
    AllowExternalReferences,
    ForbidExternalReferences
};
...

> Source/WebCore/svg/SVGURIReference.cpp:49
> +    if (funcUri.find('#') == notFound)

This code path is _very_ hot. If you're at it, I'd propose to make it more
efficient (like your old work years ago where you made the path parsing more
efficient, by directly operating on the const UChar* characters buffer).

> Source/WebCore/svg/SVGURIReference.cpp:50
> +	   return String();

s/String()/emptyString()/ - use the shared empty string for more efficiency.

> Source/WebCore/svg/SVGURIReference.cpp:53
>      if (url.startsWith("url(")) { // URI References, ie. fill:url(#target)

The initial decision here should be:
first character is 'u' or '#'.
Remember to always optimize for the common path, so checking wheter
funcUri.find('#') returns notFound as first thing is inefficient, as you're
doing avoidable extra-work for valid references.