Bug 160361

Summary: Drop SVGDocument as per the SVG2 specification
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: SVGAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, dbates, rniwa, sabouhallawa, sam, zimmermann
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://www.w3.org/TR/SVG2/struct.html#InterfaceDocumentExtensions
Attachments:
Description Flags
WIP Patch
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2016-07-29 15:56:49 PDT
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.
Attachments
WIP Patch (21.54 KB, patch)
2016-07-29 16:18 PDT, Chris Dumez
no flags
Archive of layout-test-results from ews100 for mac-yosemite (990.47 KB, application/zip)
2016-07-29 17:15 PDT, Build Bot
no flags
Archive of layout-test-results from ews113 for mac-yosemite (1.43 MB, application/zip)
2016-07-29 17:22 PDT, Build Bot
no flags
Patch (20.59 KB, patch)
2016-08-07 13:30 PDT, Chris Dumez
no flags
Archive of layout-test-results from ews102 for mac-yosemite (825.81 KB, application/zip)
2016-08-07 14:25 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (914.68 KB, application/zip)
2016-08-07 14:29 PDT, Build Bot
no flags
Archive of layout-test-results from ews124 for ios-simulator-wk2 (823.82 KB, application/zip)
2016-08-07 14:36 PDT, Build Bot
no flags
Archive of layout-test-results from ews116 for mac-yosemite (1.45 MB, application/zip)
2016-08-07 14:41 PDT, Build Bot
no flags
Patch (21.75 KB, patch)
2016-08-07 15:47 PDT, Chris Dumez
no flags
Patch (21.82 KB, patch)
2016-08-07 15:49 PDT, Chris Dumez
no flags
Patch (20.79 KB, patch)
2016-08-07 17:16 PDT, Chris Dumez
no flags
Patch (20.80 KB, patch)
2016-08-07 17:38 PDT, Chris Dumez
no flags
Patch (20.83 KB, patch)
2016-08-07 17:47 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2016-07-29 16:18:23 PDT
Created attachment 284913 [details] WIP Patch
Build Bot
Comment 2 2016-07-29 17:15:16 PDT
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
Comment 3 2016-07-29 17:15:19 PDT
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 4 2016-07-29 17:22:51 PDT
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
Comment 5 2016-07-29 17:22:54 PDT
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
Comment 6 2016-08-07 13:30:48 PDT
Build Bot
Comment 7 2016-08-07 14:25:33 PDT
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
Comment 8 2016-08-07 14:25:37 PDT
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 9 2016-08-07 14:29:40 PDT
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
Comment 10 2016-08-07 14:29:44 PDT
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 11 2016-08-07 14:35:56 PDT
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
Comment 12 2016-08-07 14:36:00 PDT
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 13 2016-08-07 14:41:50 PDT
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
Comment 14 2016-08-07 14:41:54 PDT
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
Comment 15 2016-08-07 15:47:19 PDT
Chris Dumez
Comment 16 2016-08-07 15:49:32 PDT
Darin Adler
Comment 17 2016-08-07 16:17:13 PDT
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 18 2016-08-07 16:21:15 PDT
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 19 2016-08-07 17:08:06 PDT
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
Comment 20 2016-08-07 17:16:46 PDT
Chris Dumez
Comment 21 2016-08-07 17:18:20 PDT
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 22 2016-08-07 17:26:14 PDT
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
Comment 23 2016-08-07 17:35:51 PDT
(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
Comment 24 2016-08-07 17:38:52 PDT
Chris Dumez
Comment 25 2016-08-07 17:47:01 PDT
WebKit Commit Bot
Comment 26 2016-08-07 18:35:04 PDT
Comment on attachment 285546 [details] Patch Clearing flags on attachment: 285546 Committed r204246: <http://trac.webkit.org/changeset/204246>
WebKit Commit Bot
Comment 27 2016-08-07 18:35:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.