[webkit-reviews] review canceled: [Bug 189499] [SVG] fragment-only url 'url(#fragment)' should be resolved against the current document with regardless to HTML <base> element : [Attachment 375779] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has canceled Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 189499: [SVG] fragment-only url 'url(#fragment)' should be resolved against
the current document with regardless to HTML <base> element
https://bugs.webkit.org/show_bug.cgi?id=189499

Attachment 375779: Patch

https://bugs.webkit.org/attachment.cgi?id=375779&action=review




--- Comment #22 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 375779
  --> https://bugs.webkit.org/attachment.cgi?id=375779
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

>> Source/WebCore/ChangeLog:8
>> +
> 
> Can you please add a comment explaining what you are fixing in this patch?
> Can you also mention that this patch is based on the Blink change
https://chromium.googlesource.com/chromium/src/+/e7d7225c33aa7fc42ee390125b01df
9167fad106%5E%21/?
> Is there a reason for not porting the change in
StyleBuilderConverter::convertClipPath()?

Will fix.
StyleBuilderConverter::convertClipPath has a issue it doesn't work with
external SVG references.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/StyleBuilderCon
verter.h?rev=248463#L594
However I will check it again.

>> Source/WebCore/ChangeLog:21
>> +	    (WebCore::SVGURIReference::fragmentIdentifierFromIRIString): Return
the fragemnt if the URL starts with '#'.
> 
> fragemnt

Will fix.

>> LayoutTests/svg/animations/local-url-target-reference.html:18
>> +  <set attributeName="fill" xlink:href="#rect" to="green"
dur="indefinite"/>
> 
> You do not have to include the xlink namespace in the href attribute.

Will fix.

>> LayoutTests/svg/custom/linking-base-external-reference.xhtml:-18
>> -</html>
> 
> What is the reason for removing this test?

This is a test case for the old spec.

>> LayoutTests/svg/custom/local-url-references-webkit.html:53
>> +</svg>
> 
> Why "webkit" is included in the name of the file? Does it test specific
WebKit functionalities? I think we should not do that.

I added local-url-references-webkit.html for WebKit issue (Comment 17). But, I
will remove this.

>> LayoutTests/svg/custom/local-url-references.html:53
>> +</svg>
> 
> What is the difference between this test and
local-url-references-webkit.html? Also this test and the other one are too long
to read. They both test almost all the SVG resources. Can you split them into
smaller ones?

I replaced <rect> with <polygon> to avoid Bug 200501.
Got it. I will split.