Bug 12579 – WebKit fails SVG xml:base test

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

WebKit fails SVG xml:base test

I'm not sure if WebKit supports xml:base and <image> is just broken, or if WebKit is missing xml:base support entirely.

[Rob Buis]

Created attachment 13474 [details]
First attempt

I reused some kdom code for this. Not all baseURI tests work, maybe related to entity problems, but I didnt investigate so far.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 13474 [details]
First attempt

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

[Rob Buis]

Hi Darin,

(In reply to comment #2)
> (From update of attachment 13474 [details] [edit])
> 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.

I think only the Node constructor is generated, the properties not AFAICS?

> 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.

Possible.

> +            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.

Possible. I need to check the kdom method KURL::isRelative.

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

I am not seeing how. The baseURI algorithm may need multiple parent iterations, while completeURL just uses the current node. It is described here:

http://www.w3.org/TR/2001/REC-xmlbase-20010627/

> 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.

I hope I am right, but I honestly see how :)

> 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?

Well it seems different due to xml:base usage at least.
 
> -            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().

That seems a bit better, I did that locally now.

> 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?

I think xmlbase usage is a bit more complex than completeURL. Please let me know if I got that wrong though, as it is pretty fundamental for the patch :) I'll probably attach a new patch soon, since the attached and virtual suggestions sound great to me.
Cheers,

Rob.

[Rob Buis]

Created attachment 13481 [details]
Make baseURI virtual

This patch makes baseURI virtual as suggested by Darin, and also uses attached() instead of parsing().
Cheers,

Rob.

[Darin Adler]

Comment on attachment 13481 [details]
Make baseURI virtual

+    void setDocumentURI(const String &);

Should not be a space here in String&.

+    KURL xmlbase(static_cast<const Element*>(this)->getAttribute(XMLNames::baseAttr).deprecatedString());

Since you do using at the top of the file there's no need to specify XMLNames:: here. There's no need for the static_cast. This is in the Element class.

r=me

[Rob Buis]

Created attachment 13518 [details]
Improved patch

I fixed Darin's issues but found some things that I think need another small review. First I forgot to add changed result for fast/dom/Window/window-properties.svg, that is now in this patch. Another, bigger issue is that struct-use-01.svg regressed (I didnt notice it at first since <use> was disabled), that fix is the change that I think deserves some more reviewing.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 13518 [details]
Improved patch

Oh, my bad, I just changed the window-properties test, so this might be hard to land.

r=me

[Rob Buis]

Landed in r20028.
Darin, I couldn't find baseURI/documentURI in expected output for window-properties after rerunning, I hope that is correct.

(In reply to comment #7)
> (From update of attachment 13518 [details] [edit])
> Oh, my bad, I just changed the window-properties test, so this might be hard to
> land.
> 
> r=me
> 

[Rob Buis]

Landed in r20028.