Bug 63283

Created attachment 100243 [details] Patch

Comment on attachment 100243 [details] Patch Attachment 100243 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9013176 New failing tests: http/tests/misc/slow-loading-mask.html http/tests/navigation/javascriptlink-frames.html svg/custom/invalid-url-reference.svg svg/transforms/text-with-mask-with-svg-transform.svg fast/blockflow/japanese-rl-selection.html fast/backgrounds/background-leakage.html fast/box-shadow/scaled-box-shadow.html fast/backgrounds/repeat/negative-offset-repeat.html svg/W3C-SVG-1.1/struct-use-01-t.svg transforms/2d/hindi-rotated.html svg/repaint/filter-repaint.svg fast/blockflow/japanese-lr-selection.html

Created attachment 100244 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Is comparing to the document->baseURI() the right thing? It doesn't seem to match other implementations. In particular, this sort of thing: <base href="some-other-document"> .... <rect style="fill(#something)"> has the rect fill with some-other-document#something in both Gecko and Presto.

It also doesn't match WebKit behavior for <a href>.

(In reply to comment #4) > Is comparing to the document->baseURI() the right thing? It doesn't seem to match other implementations. In particular, this sort of thing: > > <base href="some-other-document"> > .... > <rect style="fill(#something)"> > > has the rect fill with some-other-document#something in both Gecko and Presto. I don't think we support external references like that at all, that is why we still have the external <use> bug. My patch does not try to address that however :) Unfortunately this could mean this patch only makes a small step in the right direction, not a complete fix. On the other hand I do not know SVGs that use external references for fill? Do you know any or have testcases? Cheers, Rob.

(In reply to comment #5) > It also doesn't match WebKit behavior for <a href>. Can you explain some more? Do you mean that external references are allowed for <a> (obviously) but not for fill property? Note that <a href> handling uses baseURI internally as well. Cheers, Rob.

Comment on attachment 100243 [details] Patch 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.

Comment on attachment 100243 [details] Patch 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.

> I don't think we support external references like that at all Yes, but with your patch you would treat this as an internal reference was my point. > Do you know any or have testcases? Testcase is trivial to construct from the attached testcase. Just move the <defs> to a separate SVG file. > Note that <a href> handling uses baseURI internally as well. That's clearly false in terms of determining whether the reference is same-document (as opposed to resolving the URI). Consider this HTML document: <!DOCTYPE html> <base href="http://www.example.com"> <a href="#foo">Click me</a> <div style="height: 5000px"></div> <div id="foo">Do you see me?</div> Clicking the link will load example.com, not scroll to the "Do you see me?" text as it would for a same-document reference. Your patch, however, would treat a URI like that for an SVG fill as same-document in this case.

Created attachment 100413 [details] Patch

Hi Niko, (In reply to comment #9) > (From update of attachment 100243 [details]) > 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? I left the name open still, how about extractFragmentIdentifier? It is all about the fragmentIdentifer after all. > A boolean parameter like "allowExternal" should be avoided. Use an enum. > enum ExtractTargetMode { > AllowExternalReferences, > ForbidExternalReferences > }; > ... Good idea, fixed. > > 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. I keep forgetting this one :) Fixed. > > 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. The optimize thing was made a lot easier by an observation: most of the users of getTarget are of the <uri>, not <funcUri> kind! Also the css parser does actual parsing of url() so all call sites have url() removed before calling getTarget. In fact we were too tolerant because xlink:href="url(#foo)" on for example <use> was allowed, doesn't work in Opera and FF. Note that this does not completely fix the testcases Boris gave. I think this is a step in the right direction though, let's decide whether to keep this bug or open a new one for the problem of referencing external fragment identifiers. Cheers, Rob.

Comment on attachment 100413 [details] Patch Attachment 100413 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9020011 New failing tests: svg/custom/invalid-url-reference.svg

Created attachment 100416 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

> In fact we were too tolerant because xlink:href="url(#foo)" on for example > <use> was allowed That's not "tolerant"; it's just buggy. In general, I would really prefer it if you'd err on the side of making things that should not work per spec actually not work, even if you don't make things like external references work. That reduces the amount of well-poisoning caused by WebKit (especially on mobile sites) and eases later implementation of the missing features. And note that the behavior implemented here is _still_ inconsistent with how <a href> is handled in WebKit...

(In reply to comment #15) > > In fact we were too tolerant because xlink:href="url(#foo)" on for example > > <use> was allowed > > That's not "tolerant"; it's just buggy. I agree, we've been very "liberal" over the past years - which is not a good thing. Recently (a year ago) I've started with making our SVG DOM primitive types very strict, aka. we throw a lot more exceptions, just like you do - if eg. arguments are wrong, or missing,, etc.. Revisiting the fragment reference evaluation code and making it strict is a logical next step in that regard. So I agree, we should be much more strict about these things. > > In general, I would really prefer it if you'd err on the side of making things that should not work per spec actually not work, even if you don't make things like external references work. Just to get us right: we're willing to add support for external references - we'll definately come to that this year. > That reduces the amount of well-poisoning caused by WebKit (especially on mobile sites) and eases later implementation of the missing features. True. > > And note that the behavior implemented here is _still_ inconsistent with how <a href> is handled in WebKit... I've checked your testcase in WebKit trunk: <!DOCTYPE html> <base href="http://www.example.com"> <a href="#foo">Click me</a> <div style="height: 5000px"></div> <div id="foo">Do you see me?</div> Clicking this link goes to example.com#foo as expected. Robs patch does not change this behaviour in any way - he just alters the way SVG*Elements deriving from SVGURIReference resolve the uris. You've convinced me that the approach by just checking whether the passed target URI starts with "#" to determine whether it's a same-document origin is insufficient. http://www.w3.org/TR/SVG/linking.html#IRIReference says: "An IRI can also address a particular element within an XML document by including an IRI fragment identifier as part of the IRI. An IRI which includes an IRI fragment identifier consists of an optional base IRI, followed by a "#" character, followed by the IRI fragment identifier. For example, the following IRI can be used to specify the element whose ID is "Lamppost" within file someDrawing.svg: http://example.com/someDrawing.svg#Lamppost" Most interessting is: An IRI which includes an IRI fragment identifier consists of an optional base IRI, followed by a "#" character, followed by the IRI fragment identifier. I think what we have to do is: - Check whether target contains a '#' character. If true, and the position is > 0, extract the base uri string from position (0 .. PosOf#-1). Otherwhise use the <base> elements URI as base uri. That will always form an uri of type "baseURI#fragment". Then we can compare baseURI with our documents URI, if that matches the fragment reference is within the document, and the target element can be looked up with getElementById. - If there's no '#' character in the IRI string, bail out. The SVGURIReference::getTarget currently is mostly used in conjunction with document()->getElementById(SVGURIReference::getTarget(myXlinkHrefAttr)) - this will not work together with external documents, in future. I'm questioning the whole method now. We should rather have sth. like "SVGElement* targetElementFromIRIString(SVGElement* contextElement, onst String& iriString)". It will do the checks I described above (lookup '#' char, extract base uri (or use from <base> element)) and: If mode == AllowExternalReferences: ask document->getElementById(fragmentIdentifier), if the (synthesized) baseURI of the fragment string and the document base uri match, otherwise return 0 (FIXME: This would be the place than to request external documents in future). If mode == ForbidExternalReferences: if baseURIs match, lookup element, otherwise return 0. (This mode is only used by SVGSMILElement, as we only want to animate local elements). This should unify the handling of <a href> and SVGURIReference, and prepares us further to support external references.

Hi Niko, (In reply to comment #16) > > I'm questioning the whole method now. We should rather have sth. like > "SVGElement* targetElementFromIRIString(SVGElement* contextElement, onst String& iriString)". > > It will do the checks I described above (lookup '#' char, extract base uri (or use from <base> element)) and: > If mode == AllowExternalReferences: ask document->getElementById(fragmentIdentifier), if the (synthesized) baseURI of the fragment string and the document base uri match, otherwise return 0 (FIXME: This would be the place than to request external documents in future). > If mode == ForbidExternalReferences: if baseURIs match, lookup element, otherwise return 0. > (This mode is only used by SVGSMILElement, as we only want to animate local elements). > > This should unify the handling of <a href> and SVGURIReference, and prepares us further to support external references. I agree with this, right now every time we use getTarget we just want to use it to find an Element in the doc (using getElementById). I am happy this is cleared up now, plus the fact that we do not have to worry about funcUri, just iri. Will work on a new patch. Cheers, Rob.

> Clicking this link goes to example.com#foo as expected. Right, so it's not treated as a same-document reference. > Robs patch does not change this behaviour in any way Right. But it changes the way URI references are handled in SVG in a way that makes xlink:href="#foo" in SVG in the same situation be treated as a same-document reference (which it was already, of course, so the fact that it's treated same-document is not a change, but as long as you're changing the code anyway....). In any case, your description of what probably needs to be done here (starting "I think what we have to do") sounds about right. Note that the "optional base IRI" can be relative. > and the document base uri match Except this isn't what <a href> does. It compares to the document URI, not the document base URI.

Created attachment 100611 [details] Patch

Comment on attachment 100611 [details] Patch Attachment 100611 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9023458 New failing tests: fast/backgrounds/size/contain-and-cover.html svg/custom/invalid-url-reference.svg fast/backgrounds/svg-as-background-2.html svg/wicd/test-scalable-background-image1.xhtml svg/as-image/svg-as-background-with-relative-size.html svg/wicd/test-scalable-background-image2.xhtml svg/custom/embedding-external-svgs.xhtml

Created attachment 100625 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 100656 [details] Patch

Comment on attachment 100656 [details] Patch Attachment 100656 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9017518 New failing tests: svg/custom/invalid-url-reference.svg

Created attachment 100660 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 101543 [details] Patch

The handling of same-document vs not here is still not consistent with <a href>. Is that consistency just a non-goal?

Comment on attachment 101543 [details] Patch Attachment 101543 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9192493 New failing tests: svg/custom/invalid-url-reference.svg

Comment on attachment 101543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101543&action=review Everything looks good, except the ChangeLog which needs more love. Boris: I discussed with Rob on IRC, we don't see your concerns. The <html:a> example with the <base> uri works for <svg:a> as well - the <base> URI is respected for both types of anchor elements. Robs patch fixes IRI reference resolution in the same way - so I don't see what's still inconsistent... Can you please elaborate... > Source/WebCore/ChangeLog:9 > + Be more strict when trying to get fragment identifier ; only return > + the fragment identifier if it is contained in the url and the url is valid. That should be much more detailed, no? Especially regarding the <base> uri resolving etc. Basically summarize our discussion... That will help a lot looking at it again in future... > Source/WebCore/svg/SVGAltGlyphElement.cpp:93 > + String target; > + Element* element = targetElementFromIRIString(fastGetAttribute(XLinkNames::hrefAttr), document(), &target); Passing String pointers? > Source/WebCore/svg/SVGURIReference.cpp:52 > + if (start != notFound) { Early exit if start == notFound. > Source/WebCore/svg/SVGURIReference.cpp:53 > + String rel = url.substring(start); needs a better name, w/o abbrevations. > Source/WebCore/svg/SVGURIReference.cpp:69 > + if (fragmentIdentifier) > + *fragmentIdentifier = id; Pass fragmentIdentifier, as reference, avoids the need for the temporary String variable, as well as dereference operator hacks. > Source/WebCore/svg/SVGUseElement.cpp:522 > - String id = SVGURIReference::getTarget(href()); > - Element* targetElement = treeScope()->getElementById(id); > + Element* targetElement = SVGURIReference::targetElementFromIRIString(href(), document(), 0, AllowExternalReferences); Ah I see now why you passed Strings as pointers. Do we really need that? Doesn't hurt to store id here, no?

> The <html:a> example with the <base> uri works for <svg:a> as well - > the <base> URI is respected for both types of anchor elements. > Robs patch fixes IRI reference resolution in the same way The _resolution_ is the same. The decision about whether it's a same-document reference is not. <html:a> treats same-document references by scrolling and other-document references by loading a new page. SVG effects should be treating same-document references by using getElementById and other-document references by loading the other document and then calling getElementById on it. So to be clear, my issue is with this line: + || equalIgnoringFragmentIdentifier(kurl, document->baseURI())) For <html:a> the equivalent code uses the document URI, not the base URI, as the second argument.

(In reply to comment #29) > SVG effects should be treating same-document references by using getElementById and other-document references by loading the other document and then calling getElementById on it. We haven't implemented support yet for loading external references at all? (TODO) Are you concerned that Robs patch is not correct for external referenced iris? > So to be clear, my issue is with this line: > > + || equalIgnoringFragmentIdentifier(kurl, document->baseURI())) > > For <html:a> the equivalent code uses the document URI, not the base URI, as the second argument. Which equivalent code? What code are you looking at?

> We haven't implemented support yet for loading external references at all? Yes, but the attached patch makes references that are same-document in Gecko and Presto not be same-document in WebKit and vice versa. > What code are you looking at? I'm not that familiar with the WebKit codebase, but FrameLoader::shouldReload and NavigationScheduler::scheduleLocationChange look related. There's a similar check in FrameLoader::loadInSameDocument too, but at that point you already know you're doing a same-document scroll.

(In reply to comment #31) > > We haven't implemented support yet for loading external references at all? > > Yes, but the attached patch makes references that are same-document in Gecko and Presto not be same-document in WebKit and vice versa. Can you construct a testcase? I think you're mixing up something.... but if you'd have a testcase Rob could easily check it. I'd rather get this right and cleared up before landing anything. > > > What code are you looking at? > > I'm not that familiar with the WebKit codebase, but FrameLoader::shouldReload and NavigationScheduler::scheduleLocationChange look related. There's a similar check in FrameLoader::loadInSameDocument too, but at that point you already know you're doing a same-document scroll. Um, that's not really related to SVGs handling of IRI references and or anchors. (No worries, I know it's not your codebase :-)

Created attachment 102051 [details] Testcase showing difference in SVG same-document reference behavior > Can you construct a testcase? Here's a testcase that shows a black square in Presto and Gecko but will show a red square in WebKit with the proposed patch because Presto and Gecko treat this reference as an external reference while WebKit will treat it as a same-document reference.

I can construct a testcase going in the other direction too (where WebKit will treat a reference as not same-document while Presto and Gecko will treat it as same-document), but that involves knowing the URI of the testcase while writing it, so makes it hard to attach to Bugzilla. Let me know if you absolutely need it for some reason. > Um, that's not really related to SVGs handling of IRI references and or > anchors. It's not related to the former, but it looks to me like it might be related to the latter. If not, where does WebKit decide whether following an anchor should scroll or do a new load?

(In reply to comment #33) > Created an attachment (id=102051) [details] > Testcase showing difference in SVG same-document reference behavior > > > Can you construct a testcase? > > Here's a testcase that shows a black square in Presto and Gecko but will show a red square in WebKit with the proposed patch because Presto and Gecko treat this reference as an external reference while WebKit will treat it as a same-document reference. Okay, let's examine what happens: RenderSVGResourceContainer* paintingResourceFromSVGPaint() is used to lookup the resource associated with a certain fill/stroke IRI. 161 id = SVGURIReference::fragmentIdentifierFromIRIString(paintUri, document); 162 RenderSVGResourceContainer* container = getRenderSVGResourceContainerById(document, id); The default extractTargetMode is ForbidExternalReferences: static String fragmentIdentifierFromIRIString(const String&, Document*, ExtractTargetMode = ForbidExternalReferences); 51 size_t start = url.find('#'); 52 if (start != notFound) { 53 String rel = url.substring(start); 54 KURL base = start ? KURL(document->baseURI(), url.substring(0, start)) : document->baseURI(); 55 KURL kurl(base, rel); 56 if ((mode == AllowExternalReferences && kurl.isValid()) 57 || equalIgnoringFragmentIdentifier(kurl, document->baseURI())) 58 return url.substring(start + 1); 59 } For your style="fill: url(#orange_red)" example with a <base> element provided in the <html> following happens. String rel is empty, that means KURL base will be initialized to the base uri "www.example.com". KURL kurl will be "http://www.example.com#orange_red". Now that mode is ForbidExternalReferences here, it will check whether the document->baseURI() (example.com) matches the kurl, when ignoring the fragment identifier. This will work, and we'll incorrectly identify this "orange_red" reference as same-document reference. Thanks Boris for pointing this out! The bug is that we're comparing against the document->baseURI() instead of the documentURI itself. Rob, can you fix it? And please include the new testcase from Boris as well.

Hi Niko, Boris, (In reply to comment #35) > (In reply to comment #33) > > Created an attachment (id=102051) [details] [details] > > Testcase showing difference in SVG same-document reference behavior > > > > > Can you construct a testcase? > > > > Here's a testcase that shows a black square in Presto and Gecko but will show a red square in WebKit with the proposed patch because Presto and Gecko treat this reference as an external reference while WebKit will treat it as a same-document reference. > > Okay, let's examine what happens: > RenderSVGResourceContainer* paintingResourceFromSVGPaint() is used to lookup the resource associated with a certain fill/stroke IRI. > > 161 id = SVGURIReference::fragmentIdentifierFromIRIString(paintUri, document); > 162 RenderSVGResourceContainer* container = getRenderSVGResourceContainerById(document, id); > > The default extractTargetMode is ForbidExternalReferences: > static String fragmentIdentifierFromIRIString(const String&, Document*, ExtractTargetMode = ForbidExternalReferences); > > 51 size_t start = url.find('#'); > 52 if (start != notFound) { > 53 String rel = url.substring(start); > 54 KURL base = start ? KURL(document->baseURI(), url.substring(0, start)) : document->baseURI(); > 55 KURL kurl(base, rel); > 56 if ((mode == AllowExternalReferences && kurl.isValid()) > 57 || equalIgnoringFragmentIdentifier(kurl, document->baseURI())) > 58 return url.substring(start + 1); > 59 } > > For your style="fill: url(#orange_red)" example with a <base> element provided in the <html> following happens. > > String rel is empty, that means KURL base will be initialized to the base uri "www.example.com". > KURL kurl will be "http://www.example.com#orange_red". > Now that mode is ForbidExternalReferences here, it will check whether the document->baseURI() (example.com) matches the kurl, when ignoring the fragment identifier. > > This will work, and we'll incorrectly identify this "orange_red" reference as same-document reference. > Thanks Boris for pointing this out! > > The bug is that we're comparing against the document->baseURI() instead of the documentURI itself. > Rob, can you fix it? And please include the new testcase from Boris as well. I came to the same conclusion last night, after trying out the testcase. Also from me, thanks Boris for pointing this out! I added the testcase locally, made some other improvements and will upload a new patch later today. Cheers, Rob.

Hi Niko, (In reply to comment #28) > (From update of attachment 101543 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101543&action=review > > Everything looks good, except the ChangeLog which needs more love. > Boris: I discussed with Rob on IRC, we don't see your concerns. > > The <html:a> example with the <base> uri works for <svg:a> as well - the <base> URI is respected for both types of anchor elements. Robs patch fixes IRI reference resolution in the same way - so I don't see what's still inconsistent... Can you please elaborate... > > > Source/WebCore/ChangeLog:9 > > + Be more strict when trying to get fragment identifier ; only return > > + the fragment identifier if it is contained in the url and the url is valid. > > That should be much more detailed, no? Especially regarding the <base> uri resolving etc. Basically summarize our discussion... > That will help a lot looking at it again in future... I tried to to this now. > > Source/WebCore/svg/SVGAltGlyphElement.cpp:93 > > + String target; > > + Element* element = targetElementFromIRIString(fastGetAttribute(XLinkNames::hrefAttr), document(), &target); > > Passing String pointers? Correct. > > Source/WebCore/svg/SVGURIReference.cpp:52 > > + if (start != notFound) { > > Early exit if start == notFound. Fixed. > > Source/WebCore/svg/SVGURIReference.cpp:53 > > + String rel = url.substring(start); > > needs a better name, w/o abbrevations. Fixed. > > Source/WebCore/svg/SVGURIReference.cpp:69 > > + if (fragmentIdentifier) > > + *fragmentIdentifier = id; > > Pass fragmentIdentifier, as reference, avoids the need for the temporary String variable, as well as dereference operator hacks. See below. > > Source/WebCore/svg/SVGUseElement.cpp:522 > > - String id = SVGURIReference::getTarget(href()); > > - Element* targetElement = treeScope()->getElementById(id); > > + Element* targetElement = SVGURIReference::targetElementFromIRIString(href(), document(), 0, AllowExternalReferences); > > Ah I see now why you passed Strings as pointers. Do we really need that? Doesn't hurt to store id here, no? I can do this, it is just that many call sites need to be changed, hence the 0 passing. So we allocate a String object for nothing in some occasions. So there are pro's and con's to both, what do you think? Cheers, Rob.

Created attachment 102137 [details] Patch

> The bug is that we're comparing against the document->baseURI() instead of > the documentURI itself. Yes, that's what comment 4 said. ;)

Comment on attachment 102137 [details] Patch Attachment 102137 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9210045 New failing tests: svg/custom/invalid-url-reference.svg svg/custom/linking-base-external-reference.xhtml

(In reply to comment #39) > > The bug is that we're comparing against the document->baseURI() instead of > > the documentURI itself. > > Yes, that's what comment 4 said. ;) Fun! If you'd have used the review tool to annotate your comment to the right line (equalIgnroingFragmentIdentifier) I would have noticed it earlier! :-) Anyhow, great that we've reached conclusion here...

Ah, I'll try that next time. Not used to having that available in a bugzilla.

Comment on attachment 102137 [details] Patch 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::fragmentIdentifierFromIRIString(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_string, 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::fragmentIdentifierFromIRIString(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...

Comment on attachment 102137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102137&action=review > Source/WebCore/css/SVGCSSStyleSelector.cpp:396 > - svgstyle->setMarkerEndResource(SVGURIReference::getTarget(s)); > + svgstyle->setMarkerEndResource(SVGURIReference::fragmentIdentifierFromIRIString(s, m_element->document())); Oops, we silently assume same-document references here! If we just extract the fragment identifier here, we're going to ignore any external URL. We should store the whole IRI string here, and lookup the targetelmenetForIRIString later in rendering/ when we actually use the marker end resource. That affects all usages of fragmentIdentifierFromIRIString - thery're all assuming same-doc refs, which is wrong. (Parts from our IRC discussion)

Created attachment 102371 [details] Patch

(In reply to comment #44) > (From update of attachment 102137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102137&action=review > > > Source/WebCore/css/SVGCSSStyleSelector.cpp:396 > > - svgstyle->setMarkerEndResource(SVGURIReference::getTarget(s)); > > + svgstyle->setMarkerEndResource(SVGURIReference::fragmentIdentifierFromIRIString(s, m_element->document())); > > Oops, we silently assume same-document references here! If we just extract the fragment identifier here, we're going to ignore any external URL. > We should store the whole IRI string here, and lookup the targetelmenetForIRIString later in rendering/ when we actually use the marker end resource. > > That affects all usages of fragmentIdentifierFromIRIString - thery're all assuming same-doc refs, which is wrong. > (Parts from our IRC discussion) I tried to incorporate it in my patch, but it is quite tricky. I propose to leave it out for now and do it in bug 65344, I can move the comment to that bug as well as reminder. Cheers, Rob.

(In reply to comment #46) > > That affects all usages of fragmentIdentifierFromIRIString - thery're all assuming same-doc refs, which is wrong. > > (Parts from our IRC discussion) > > I tried to incorporate it in my patch, but it is quite tricky. I propose to leave it out for now and do it in bug 65344, I can move the comment to that bug as well as reminder. Also note that it would change a lot of the expected results. Cheers, Rob.

Comment on attachment 102371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102371&action=review Almost there, still needs some love! > Source/WebCore/css/CSSFontFaceSource.cpp:135 > + if (!m_externalSVGFontElement) { > + String fragmentIdentifier; This needs a fixme linked to the external refs bug report as well. > Source/WebCore/css/SVGCSSStyleSelector.cpp:364 > + svgstyle->setMarkerStartResource(SVGURIReference::fragmentIdentifierFromIRIString(s, m_element->document())); You're still storing the fragment identifier in the RenderStyle, instead of the pure iri string? I thought you wanted to change that in preparation for external refs. > Source/WebCore/rendering/svg/SVGResources.cpp:162 > - id = SVGURIReference::getTarget(paintUri); > + id = SVGURIReference::fragmentIdentifierFromIRIString(paintUri, document); > RenderSVGResourceContainer* container = getRenderSVGResourceContainerById(document, id); This code is wrong as well, every code path that just wants to extract the id (just like in cSSFontFaceSource) is wrong. You basically want to have getRenderSVGResourceContainerByIRIString, which looks up directly an Element. > Source/WebCore/svg/SVGPaint.cpp:168 > + return referenceId == SVGURIReference::fragmentIdentifierFromIRIString(m_uri, node() ? node()->document() : 0); Likely wrong as well, for the same reason as above. > Source/WebCore/svg/SVGURIReference.cpp:48 > +String SVGURIReference::fragmentIdentifierFromIRIString(const String& url, Document* document) This method needs to be private and should have no users except targetElementFromIRIString.

(In reply to comment #46) > > I tried to incorporate it in my patch, but it is quite tricky. I propose to leave it out for now and do it in bug 65344, I can move the comment to that bug as well as reminder. Oops, I missed that comment, sure if you want to work on it soon, no problem! r=me then!

Landed in r92047.