Remove SVGDocument binding

Remove SVGDocument binding

The SVG2 specification removes the SVGDocument interface and instead suggests the SVG document
to be a partial implementation on Document. This patch does that except it does a partial implement
of XMLDocument, since an SVG document is also an XML Document according to DOM4.
The createEvent specialization for SVG is now handled by Document. The rootElement method is still
on the native SVGDocument for now.
The GetSVGDocument interface now returns a Document instead of an SVGDocument as specified in SVG2 spec.

BUG=238366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166179

[Inactive, 2014-01-24 00:00:38]

https://codereview.chromium.org/146193003/diff/1/Source/core/html/HTMLFrameOw...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/146193003/diff/1/Source/core/html/HTMLFrameOw...
Source/core/html/HTMLFrameOwnerElement.cpp:116: Document*
HTMLFrameOwnerElement::getSVGDocument(ExceptionState& exceptionState) const
Why do we need to return a Document. We can keep returning an SVGDocument on
native side, right? I think we only need to return a Document on IDL side.

https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument.cpp
File Source/core/svg/SVGDocument.cpp (right):

https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument....
Source/core/svg/SVGDocument.cpp:42: SVGSVGElement*
SVGDocument::rootElement(const XMLDocument* doc)
We don't like abbreviations: doc -> document

https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument.idl
File Source/core/svg/SVGDocument.idl (right):

https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument....
Source/core/svg/SVGDocument.idl:22: partial interface XMLDocument {
Spec says we extend Document, not XMLDocument

[rwlbuis, 2014-01-24 00:14:59]

On 2014/01/24 00:00:38, Chris Dumez wrote:
>
https://codereview.chromium.org/146193003/diff/1/Source/core/html/HTMLFrameOw...
> File Source/core/html/HTMLFrameOwnerElement.cpp (right):
> 
>
https://codereview.chromium.org/146193003/diff/1/Source/core/html/HTMLFrameOw...
> Source/core/html/HTMLFrameOwnerElement.cpp:116: Document*
> HTMLFrameOwnerElement::getSVGDocument(ExceptionState& exceptionState) const
> Why do we need to return a Document. We can keep returning an SVGDocument on
> native side, right? I think we only need to return a Document on IDL side.

This particular one gives compilation problems when changed to return
SVGDocument:

gen/blink/bindings/V8HTMLEmbedElement.cpp: In function 'void
WebCore::HTMLEmbedElementV8Internal::getSVGDocumentMethod(const
v8::FunctionCallbackInfo<v8::Value>&)':
gen/blink/bindings/V8HTMLEmbedElement.cpp:284:102: error: no matching function
for call to
'WebCore::BindingSecurity::shouldAllowAccessToNode(WebCore::SVGDocument*,
WebCore::ExceptionState&)'
gen/blink/bindings/V8HTMLEmbedElement.cpp:284:102: note: candidate is:
In file included from gen/blink/bindings/V8HTMLEmbedElement.cpp:39:0,
                 from gen/blink/bindings/V8GeneratedBindings02.cpp:37:
../../third_party/WebKit/Source/bindings/v8/BindingSecurity.h:50:17: note:
static bool WebCore::BindingSecurity::shouldAllowAccessToNode(WebCore::Node*,
WebCore::ExceptionState&)
../../third_party/WebKit/Source/bindings/v8/BindingSecurity.h:50:17: note:   no
known conversion for argument 1 from 'WebCore::SVGDocument*' to 'WebCore::Node*'
In file included from gen/blink/bindings/V8GeneratedBindings02.cpp:37:0:
gen/blink/bindings/V8HTMLEmbedElement.cpp:289:65: error: conversion from
'WebCore::SVGDocument*' to non-scalar type 'WTF::RefPtr<WebCore::Document>'
requested

It seems to be because #include "SVGDocument.h" is not in
V8HTMLEmbedElement.cpp. And that may be because there is no SVGDocument
interface on idl anymore? Let me know if the bindings generator needs to be made
smarter or I am missing something...

>
https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument.cpp
> File Source/core/svg/SVGDocument.cpp (right):
> 
>
https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument....
> Source/core/svg/SVGDocument.cpp:42: SVGSVGElement*
> SVGDocument::rootElement(const XMLDocument* doc)
> We don't like abbreviations: doc -> document

Sure, fixed.

>
https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument.idl
> File Source/core/svg/SVGDocument.idl (right):
> 
>
https://codereview.chromium.org/146193003/diff/1/Source/core/svg/SVGDocument....
> Source/core/svg/SVGDocument.idl:22: partial interface XMLDocument {
> Spec says we extend Document, not XMLDocument

Right, fixed.

[Inactive, 2014-01-24 00:23:47]

There is likely tests failing in the test/ folder. I know I added a test when I
added XMLDocument that needs to be updated for example.

https://codereview.chromium.org/146193003/diff/70001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/146193003/diff/70001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3708: {
Maybe we should add a UseCounter for this because this is ugly and it'd be great
if we could get rid of it.

https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:116: Document*
HTMLFrameOwnerElement::getSVGDocument(ExceptionState& exceptionState) const
I don't think this change is needed.

https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.h:52: Document*
getSVGDocument(ExceptionState&) const;
I don't think this change is needed.

https://codereview.chromium.org/146193003/diff/70001/Source/core/svg/SVGDocum...
File Source/core/svg/SVGDocument.cpp (right):

https://codereview.chromium.org/146193003/diff/70001/Source/core/svg/SVGDocum...
Source/core/svg/SVGDocument.cpp:44: Element* elem = document->documentElement();
do we need an isSVGDocument() check or will this do the right thing without?

[Inactive, 2014-01-24 00:24:10]

On 2014/01/24 00:23:47, Chris Dumez wrote:
> There is likely tests failing in the test/ folder. I know I added a test when
I
> added XMLDocument that needs to be updated for example.

I meant fast/ folder.

[rwlbuis, 2014-01-24 00:40:18]

On 2014/01/24 00:23:47, Chris Dumez wrote:
> There is likely tests failing in the test/ folder. I know I added a test when
I
> added XMLDocument that needs to be updated for example.

We'll see.

>
https://codereview.chromium.org/146193003/diff/70001/Source/core/dom/Document...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/146193003/diff/70001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:3708: {
> Maybe we should add a UseCounter for this because this is ugly and it'd be
great
> if we could get rid of it.

Could add it before landing.

>
https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
> File Source/core/html/HTMLFrameOwnerElement.cpp (right):
> 
>
https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
> Source/core/html/HTMLFrameOwnerElement.cpp:116: Document*
> HTMLFrameOwnerElement::getSVGDocument(ExceptionState& exceptionState) const
> I don't think this change is needed.

Right, fixed.

>
https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
> File Source/core/html/HTMLFrameOwnerElement.h (right):
> 
>
https://codereview.chromium.org/146193003/diff/70001/Source/core/html/HTMLFra...
> Source/core/html/HTMLFrameOwnerElement.h:52: Document*
> getSVGDocument(ExceptionState&) const;
> I don't think this change is needed.

Fixed.

>
https://codereview.chromium.org/146193003/diff/70001/Source/core/svg/SVGDocum...
> File Source/core/svg/SVGDocument.cpp (right):
> 
>
https://codereview.chromium.org/146193003/diff/70001/Source/core/svg/SVGDocum...
> Source/core/svg/SVGDocument.cpp:44: Element* elem =
document->documentElement();
> do we need an isSVGDocument() check or will this do the right thing without?

I think the combination of it being the document element and having tagName
<svg> with ns svg is enough.

[Erik Dahlström (inactive), 2014-01-24 10:30:36]

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3707: PassRefPtr<Event>
Document::createEvent(ExceptionState& exceptionState)
Do we know how much content would break if we removed this? (SVG 1.1 required
the eventType parameter via DOM2 Events, and DOM4 requires it too).

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.idl:64: [RaisesException] Event createEvent(optional
DOMString eventType);
Is this change really necessary? DOM4 doesn't seem to make this optional.

[Erik Dahlström (inactive), 2014-01-24 12:57:00]

https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
File Source/core/svg/SVGDocument.idl (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
Source/core/svg/SVGDocument.idl:23: readonly attribute SVGSVGElement       
rootElement;
Please add a UseCounter for this, this is mostly just a legacy duplicate of
document.documentElement. I'd prefer if we could just remove it.

[Inactive, 2014-01-24 14:01:59]

Looks like there are a few valid failures on the bots.

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3707: PassRefPtr<Event>
Document::createEvent(ExceptionState& exceptionState)
On 2014/01/24 10:30:37, Erik Dahlström wrote:
> Do we know how much content would break if we removed this? (SVG 1.1 required
> the eventType parameter via DOM2 Events, and DOM4 requires it too).

I advised Rob to add a UseCounter for this one to make sure we can safely remove
it. Maybe we can even directly deprecate it.
However, straight removal seems a bit risky but I'm not an API owner.

https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
File Source/core/svg/SVGDocument.idl (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
Source/core/svg/SVGDocument.idl:23: readonly attribute SVGSVGElement       
rootElement;
On 2014/01/24 12:57:01, Erik Dahlström wrote:
> Please add a UseCounter for this, this is mostly just a legacy duplicate of
> document.documentElement. I'd prefer if we could just remove it.

It is still part of the spec though:
https://svgwg.org/svg2-draft/single-page.html#struct-__svg__SVGDocument__root...

[arv (Not doing code reviews), 2014-01-24 14:46:51]

Awesome. A few comments.

https://codereview.chromium.org/146193003/diff/160001/LayoutTests/svg/custom/...
File LayoutTests/svg/custom/dynamic-svg-document-creation-expected.txt (right):

https://codereview.chromium.org/146193003/diff/160001/LayoutTests/svg/custom/...
LayoutTests/svg/custom/dynamic-svg-document-creation-expected.txt:6: RenderBlock
{html} at (0,0) size 800x200
Maybe this test should be dumpAsText?

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3714: return createEvent("undefined",
exceptionState);
Crazy, but OK.

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/dom/Documen...
Source/core/dom/Document.idl:64: [RaisesException] Event createEvent(optional
DOMString eventType);
On 2014/01/24 10:30:37, Erik Dahlström wrote:
> Is this change really necessary? DOM4 doesn't seem to make this optional.

This actually breaks other things of similar outrageous proportion. Now
createEvent.length is 0 when it should be 1 ;-)

You could use [TreatUndefinedAs=NullString] and check for that instead.

https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
File Source/core/svg/SVGDocument.idl (right):

https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
Source/core/svg/SVGDocument.idl:23: readonly attribute SVGSVGElement       
rootElement;
On 2014/01/24 14:02:00, Chris Dumez wrote:
> On 2014/01/24 12:57:01, Erik Dahlström wrote:
> > Please add a UseCounter for this, this is mostly just a legacy duplicate of
> > document.documentElement. I'd prefer if we could just remove it.
> 
> It is still part of the spec though:
>
https://svgwg.org/svg2-draft/single-page.html#struct-__svg__SVGDocument__root...

I would also prefer a UseCounter for this. Spec or no spec. We are all in the
same boat.

[rwlbuis, 2014-01-24 16:13:57]

Hi,

On 2014/01/24 14:46:51, arv wrote:
> Awesome. A few comments.
> 
>
https://codereview.chromium.org/146193003/diff/160001/LayoutTests/svg/custom/...
> File LayoutTests/svg/custom/dynamic-svg-document-creation-expected.txt
(right):
> 
>
https://codereview.chromium.org/146193003/diff/160001/LayoutTests/svg/custom/...
> LayoutTests/svg/custom/dynamic-svg-document-creation-expected.txt:6:
RenderBlock
> {html} at (0,0) size 800x200
> Maybe this test should be dumpAsText?

I realized this when looking again at the patch :) The latest patch set does
that and removes the .png.

>
https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
> File Source/core/svg/SVGDocument.idl (right):
> 
>
https://codereview.chromium.org/146193003/diff/160001/Source/core/svg/SVGDocu...
> Source/core/svg/SVGDocument.idl:23: readonly attribute SVGSVGElement       
> rootElement;
> On 2014/01/24 14:02:00, Chris Dumez wrote:
> > On 2014/01/24 12:57:01, Erik Dahlström wrote:
> > > Please add a UseCounter for this, this is mostly just a legacy duplicate
of
> > > document.documentElement. I'd prefer if we could just remove it.
> > 
> > It is still part of the spec though:
> >
>
https://svgwg.org/svg2-draft/single-page.html#struct-__svg__SVGDocument__root...
> 
> I would also prefer a UseCounter for this. Spec or no spec. We are all in the
> same boat.

Agreed, I added a UseCounter in the latest patch set. Now back to fixing the
merge problem...

[rwlbuis, 2014-01-26 21:15:14]

All green! Ready for review...

[Inactive, 2014-01-27 15:48:54]

+jochen as API Owner.

https://codereview.chromium.org/146193003/diff/330001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/146193003/diff/330001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3712: if (!isSVGDocument()) {
The UseCounter should likely be at the beginning of this function.
Maybe we can even deprecate this one directly? We should ask an API Owner.

https://codereview.chromium.org/146193003/diff/330001/Source/core/dom/Documen...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/146193003/diff/330001/Source/core/dom/Documen...
Source/core/dom/Document.idl:64: [MeasureAs=DocumentCreateEvent,
RaisesException] Event createEvent(optional DOMString eventType);
This will measure usage of all createEvent() calls, even those that provide an
argument. I think we only want to deprecate calls without argument so we will
likely need to add the UseCounter code manually on native side instead.

https://codereview.chromium.org/146193003/diff/330001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/146193003/diff/330001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:307: DocumentCreateEvent,
DocumentCreateEventOptionalArgument? This is the only one we want to deprecate.

[jochen (gone - plz use gerrit), 2014-01-28 13:15:07]

what's the impact on existing web content of these changes?

[Inactive, 2014-01-28 14:13:33]

On 2014/01/28 13:15:07, jochen wrote:
> what's the impact on existing web content of these changes?

This is mostly backward compatible as the returned Document behaves exactly like
the old SVGDocument (the API was moved from SVGDocument to Document).

The 2 web facing changes are:
- window.SVGDocument no longer exists. I don't know how much this is used. If we
worry about this, we could manually add an SVGDocument attribute to the Window
object and have it point to Document (like we used to do for XMLDocument).
- The various methods that used to return an SVGDocument now return a Document
(which behaves exactly like an SVGDocument) but it is unlikely the caller relies
on the exact type of Object being returned.

[Inactive, 2014-01-28 14:17:08]

lgtm but this needs approval from an API Owner.

Also, please make sure arv / ed are happy with this.

[rwlbuis, 2014-01-28 14:41:15]

On 2014/01/28 14:17:08, Chris Dumez wrote:
> lgtm but this needs approval from an API Owner.
> 
> Also, please make sure arv / ed are happy with this.

Excellent! I'll add pdr too.

[pdr., 2014-01-28 21:16:35]

On 2014/01/28 14:41:36, rwlbuis wrote:

LGTM2

[Erik Dahlström (inactive), 2014-01-29 00:00:38]

On 2014/01/28 21:16:35, pdr wrote:
> On 2014/01/28 14:41:36, rwlbuis wrote:
> 
> LGTM2

lgtm too, (minor nit about the UseCounter naming, I think
"SVGDocumentRootElement" would have been a more descriptive name)

[jochen (gone - plz use gerrit), 2014-01-30 08:29:21]

lgtm

[commit-bot: I haz the power, 2014-01-30 12:11:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/146193003/570001

[commit-bot: I haz the power, 2014-01-30 12:11:27]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 304.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
bfce74a241b88e89f318a624ce73c13c51869456..96aa90fe636033c5d62223b89ee9405e48b88ba3
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -304,6 +304,8 @@ public:
         ResetReferrerPolicy,
         CaseInsensitiveAttrSelectorMatch, // Case-insensitivity dropped from
specification.
         CaptureAttributeAsBoolean,
+        SVGRootElement,
+        DocumentCreateEventOptionalArgument,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         NumberOfFeatures, // This enum value must be last.

[commit-bot: I haz the power, 2014-01-30 12:11:51]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 20:13:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/146193003/580001

[commit-bot: I haz the power, 2014-01-30 22:40:59]

Change committed as 166179

[commit-bot: I haz the power, 2014-01-30 22:41:09]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 22:41:46]

CQ bit was unchecked on CL. Ignoring.

[Nils Barth (inactive), 2014-01-31 01:26:50]

Bindings generator side CL here:
Remove SVGDocument from bindings generator and test cases
https://codereview.chromium.org/151043002/

(Removing test and special-casing.)