Bug 160361 – Drop SVGDocument as per the SVG2 specification

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Chris Dumez]

Drop SVGDocument as per the SVG2 specification:
- https://www.w3.org/TR/SVG2/struct.html#InterfaceDocumentExtensions

SVGDocument has been merged into Document.

Chrome and Edge have dropped SVGDocument already, Firefox has not.

[Chris Dumez]

Created attachment 284913 [details]
WIP Patch

[Build Bot]

Comment on attachment 284913 [details]
WIP Patch

Attachment 284913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1776071

New failing tests:
svg/custom/manually-parsed-svg-allowed-in-dashboard.html

[Build Bot]

Created attachment 284921 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284913 [details]
WIP Patch

Attachment 284913 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1776080

New failing tests:
svg/custom/manually-parsed-svg-allowed-in-dashboard.html

[Build Bot]

Created attachment 284922 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 285531 [details]
Patch

[Build Bot]

Comment on attachment 285531 [details]
Patch

Attachment 285531 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1830854

New failing tests:
svg/custom/frame-getSVGDocument.html

[Build Bot]

Created attachment 285532 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285531 [details]
Patch

Attachment 285531 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1830866

New failing tests:
svg/custom/frame-getSVGDocument.html

[Build Bot]

Created attachment 285533 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285531 [details]
Patch

Attachment 285531 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1830868

New failing tests:
svg/custom/frame-getSVGDocument.html

[Build Bot]

Created attachment 285534 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 285531 [details]
Patch

Attachment 285531 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1830871

New failing tests:
svg/custom/frame-getSVGDocument.html

[Build Bot]

Created attachment 285535 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 285539 [details]
Patch

[Chris Dumez]

Created attachment 285540 [details]
Patch

[Darin Adler]

Comment on attachment 285540 [details]
Patch

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

Seems OK if it compiles.

> Source/WebCore/dom/Document.h:426
> +    SVGSVGElement* svgRootElement() const;

Does this have to be a member of Document? How about making it a static member function in the SVGSVGElement class that takes a Document& instead? Or is this needed for the bindings?

> Source/WebCore/svg/SVGDocument.h:35
> +    static SVGSVGElement* rootElement(Document&);

Why are we defining this? Who is calling it?

> Source/WebCore/svg/SVGDocument.idl:26
> +partial interface Document {
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
>      readonly attribute SVGSVGElement? rootElement;
> +#endif
>  };

What code does this compile down to? I don’t understand how it works.

[Chris Dumez]

Comment on attachment 285540 [details]
Patch

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

>> Source/WebCore/dom/Document.h:426
>> +    SVGSVGElement* svgRootElement() const;
> 
> Does this have to be a member of Document? How about making it a static member function in the SVGSVGElement class that takes a Document& instead? Or is this needed for the bindings?

No, we don't have to add this getter here. The only requirement for the bindings is that we have the static rootElement(Document&) function on SVGDocument. I can drop this getter and only use the static function on SVGDocument. I just thought this looked nicer at call sites.

>> Source/WebCore/svg/SVGDocument.h:35
>> +    static SVGSVGElement* rootElement(Document&);
> 
> Why are we defining this? Who is calling it?

This one is for the bindings.

[Darin Adler]

Comment on attachment 285540 [details]
Patch

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

>>> Source/WebCore/dom/Document.h:426
>>> +    SVGSVGElement* svgRootElement() const;
>> 
>> Does this have to be a member of Document? How about making it a static member function in the SVGSVGElement class that takes a Document& instead? Or is this needed for the bindings?
> 
> No, we don't have to add this getter here. The only requirement for the bindings is that we have the static rootElement(Document&) function on SVGDocument. I can drop this getter and only use the static function on SVGDocument. I just thought this looked nicer at call sites.

How about a static function on SVGSVGElement instead? Do we need an SVGDocument class at all long term?

>>> Source/WebCore/svg/SVGDocument.h:35
>>> +    static SVGSVGElement* rootElement(Document&);
>> 
>> Why are we defining this? Who is calling it?
> 
> This one is for the bindings.

How do the bindings know what function to call? I don’t see the string SVGDocument anywhere in the IDL file.

[Chris Dumez]

Created attachment 285543 [details]
Patch

[Chris Dumez]

Comment on attachment 285540 [details]
Patch

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

>>>> Source/WebCore/svg/SVGDocument.h:35
>>>> +    static SVGSVGElement* rootElement(Document&);
>>> 
>>> Why are we defining this? Who is calling it?
>> 
>> This one is for the bindings.
> 
> How do the bindings know what function to call? I don’t see the string SVGDocument anywhere in the IDL file.

I believe it uses the IDL filename (in this case SVGElement.idl).

[Chris Dumez]

Comment on attachment 285540 [details]
Patch

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

>>>> Source/WebCore/dom/Document.h:426
>>>> +    SVGSVGElement* svgRootElement() const;
>>> 
>>> Does this have to be a member of Document? How about making it a static member function in the SVGSVGElement class that takes a Document& instead? Or is this needed for the bindings?
>> 
>> No, we don't have to add this getter here. The only requirement for the bindings is that we have the static rootElement(Document&) function on SVGDocument. I can drop this getter and only use the static function on SVGDocument. I just thought this looked nicer at call sites.
> 
> How about a static function on SVGSVGElement instead? Do we need an SVGDocument class at all long term?

This would require adding support some new IDL extended attribute and tweaking the bindings generator to specify where the static function is defined. Currently, the bindings generator relies on the IDL filename (in this case SVGDocument).
I don't know if it is worth the trouble. Is my latest patch acceptable?

[Darin Adler]

(In reply to comment #22)
> This would require adding support some new IDL extended attribute and
> tweaking the bindings generator to specify where the static function is
> defined. Currently, the bindings generator relies on the IDL filename (in
> this case SVGDocument).
> I don't know if it is worth the trouble. Is my latest patch acceptable?

Sure this is fine. I didn't realize that the bindings generator used the name of the file for partial interfaces in this fashion. It's completely reasonable to go with that. If we were planning to remove the class SVGDocument, then SVGSVGElement would be a good home for this single function.

[Chris Dumez]

Created attachment 285545 [details]
Patch

[Chris Dumez]

Created attachment 285546 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 285546 [details]
Patch

Clearing flags on attachment: 285546

Committed r204246: <http://trac.webkit.org/changeset/204246>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.