[webkit-reviews] review denied: [Bug 12579] WebKit fails SVG xml:base test : [Attachment 13474] First attempt

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 12579: WebKit fails SVG xml:base test
http://bugs.webkit.org/show_bug.cgi?id=12579

Attachment 13474: First attempt
http://bugs.webkit.org/attachment.cgi?id=13474&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The Node class is one of the ones that has some automatically generated
bindings along with some manually-done legacy ones. Because of this, you should
put the baseURL property into Node.idl instead of making the changes to
kjs_dom.cpp and kjs_dom.h so you don't create more legacy for us to deal with
later.

To me it looks like baseURI should be a virtual function. It's a little strange
to use a case statement instead. Or at least there should be an inner part
that's a virtual function.

+	     xmlbase = KURL(static_cast<const
Element*>(this)->getAttribute(XMLNames::baseAttr).deprecatedString());
+
+	     if (!xmlbase.protocol().isEmpty())
+		 return xmlbase.url();

The above code seems like an inefficient way to do the job. Basically we're
just looking for a ":" but we convert both to and from a DeprecatedString. This
really cries out for some simple URL-related functions we can just call on
strings. But I guess it's no big deal.

Why isn't baseURI using the Document::completeURL function? It seems to be
replicating what it does.

Same comment for SVGImageLoader::updateFromElement. Why isn't it using
Document::completeURL? If it's really different than the standard relative URL
rules, then I'd like to see test cases demonstrating that.

Is the rule about resolving URLs really different for SVG that HTML? If so,
then can we put a new function on Document instead of giving SVGImageLoader its
own relative-URL completion code?

-	     if (attr->name().matches(XLinkNames::hrefAttr))
+	     if (attr->name().matches(XLinkNames::hrefAttr) &&
!ownerDocument()->parsing())

Are you sure parsing is the right check here? Maybe a better check is just to
see if you're attached, since you are doing the update in attach().

I think that baseURI and documentURI are not really needed to fix this. There's
no reason SVGImageLoader has to use new functions from the DOM standard just to
resolve a relative URL, is there?

review- because of a number of small questions here