Replace SVGDocument by XMLDocument

Replace SVGDocument by XMLDocument since it is removed from SVG2. In order to achieve this move its functionality to SVGDocumentExtensions and adjust the call sites. XMLDocument now knows how to create a SVG Document and getSVGDocument returns a Document. Finally mark XMLDocument as final.

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

[rwlbuis, 2014-05-03 20:40:05]

PTAL.

[pdr., 2014-05-03 21:35:30]

I like where this is heading.

https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:593:
m_frame->document()->accessSVGExtensions().startPan(m_frame->view()->windowToContents(event.event().position()));
This isn't necessary. Instead of checking for svgExtensions and then using
accessSVGExtensions, we should just use accessSVGExtensions.

https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:1399:
m_frame->document()->svgExtensions()->updatePan(m_frame->view()->windowToContents(m_lastKnownMousePosition));
accessSVGExtensions (here, and elsewhere)

https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
File Source/core/svg/SVGDocumentExtensions.cpp (right):

https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
Source/core/svg/SVGDocumentExtensions.cpp:464: if (m_document->renderer())
This repaint call can be removed.

https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
Source/core/svg/SVGDocumentExtensions.cpp:469: SVGSVGElement*
SVGDocumentExtensions::rootElement(const Document& document)
Is the document argument necessary here? It is always equal to m_document.

[rwlbuis, 2014-05-03 21:42:19]

On 2014/05/03 21:35:30, pdr wrote:
> I like where this is heading.
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:593:
>
m_frame->document()->accessSVGExtensions().startPan(m_frame->view()->windowToContents(event.event().position()));
> This isn't necessary. Instead of checking for svgExtensions and then using
> accessSVGExtensions, we should just use accessSVGExtensions.
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:1399:
>
m_frame->document()->svgExtensions()->updatePan(m_frame->view()->windowToContents(m_lastKnownMousePosition));
> accessSVGExtensions (here, and elsewhere)
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
> File Source/core/svg/SVGDocumentExtensions.cpp (right):
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
> Source/core/svg/SVGDocumentExtensions.cpp:464: if (m_document->renderer())
> This repaint call can be removed.
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
> Source/core/svg/SVGDocumentExtensions.cpp:469: SVGSVGElement*
> SVGDocumentExtensions::rootElement(const Document& document)
> Is the document argument necessary here? It is always equal to m_document.

Good points, will have a look.

[rwlbuis, 2014-05-03 21:52:16]

On 2014/05/03 21:35:30, pdr wrote:
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
> Source/core/svg/SVGDocumentExtensions.cpp:469: SVGSVGElement*
> SVGDocumentExtensions::rootElement(const Document& document)
> Is the document argument necessary here? It is always equal to m_document.

I just realized that the bindings need it.

[rwlbuis, 2014-05-03 21:57:11]

On 2014/05/03 21:35:30, pdr wrote:
> I like where this is heading.
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:593:
>
m_frame->document()->accessSVGExtensions().startPan(m_frame->view()->windowToContents(event.event().position()));
> This isn't necessary. Instead of checking for svgExtensions and then using
> accessSVGExtensions, we should just use accessSVGExtensions.

Not sure if I agree, we only want to do this for SVG Documents which AFAIK is
not guarenteed here. What I can do it
testing isSVGDocument first and then using accessSVGExtensions to ask
zoomAndPanEnabled.

>
https://codereview.chromium.org/268803005/diff/20001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:1399:
>
m_frame->document()->svgExtensions()->updatePan(m_frame->view()->windowToContents(m_lastKnownMousePosition));
> accessSVGExtensions (here, and elsewhere)

Well if we get here we know it is an SVG Document and SVGDocumentExtensions has
been accessed, so I figured svgExtensions() is
the quickest way to get access.

[pdr., 2014-05-05 17:37:58]

On 2014/05/03 21:52:16, rwlbuis wrote:
> On 2014/05/03 21:35:30, pdr wrote:
> >
>
https://codereview.chromium.org/268803005/diff/20001/Source/core/svg/SVGDocum...
> > Source/core/svg/SVGDocumentExtensions.cpp:469: SVGSVGElement*
> > SVGDocumentExtensions::rootElement(const Document& document)
> > Is the document argument necessary here? It is always equal to m_document.
> 
> I just realized that the bindings need it.

Lets still convert the callers to use
document->accessSVGExtensions().rootElement() and add the function
"SVGSVGElement* rootElement() const;" for the cases where we know which document
to use.

https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:593:
m_frame->document()->accessSVGExtensions().startPan(m_frame->view()->windowToContents(event.event().position()));
Nit: inside the if statement we can just use svgExtensions().

https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:1399:
m_frame->document()->svgExtensions()->updatePan(m_frame->view()->windowToContents(m_lastKnownMousePosition));
Nit: It isn't obvious to me that this always hits an accessSVGExtensions() first
and it won't be obvious to future refactorers either. I'd prefer we switch these
lone ones to accessSVGExtensions()
(here, and in handleMouseReleaseEvent).

[rwlbuis, 2014-05-05 19:06:59]

On 2014/05/05 17:37:58, pdr wrote:
> On 2014/05/03 21:52:16, rwlbuis wrote:
> > I just realized that the bindings need it.
> 
> Lets still convert the callers to use
> document->accessSVGExtensions().rootElement() and add the function
> "SVGSVGElement* rootElement() const;" for the cases where we know which
document
> to use.

I did that. However some call sites need the non-const version of rootElement.

>
https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:593:
>
m_frame->document()->accessSVGExtensions().startPan(m_frame->view()->windowToContents(event.event().position()));
> Nit: inside the if statement we can just use svgExtensions().

This needs the non-const one unfortunately.

>
https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:1399:
>
m_frame->document()->svgExtensions()->updatePan(m_frame->view()->windowToContents(m_lastKnownMousePosition));
> Nit: It isn't obvious to me that this always hits an accessSVGExtensions()
first
> and it won't be obvious to future refactorers either. I'd prefer we switch
these
> lone ones to accessSVGExtensions()
> (here, and in handleMouseReleaseEvent).

Done.

[pdr., 2014-05-05 19:49:17]

On 2014/05/05 19:06:59, rwlbuis wrote:
> On 2014/05/05 17:37:58, pdr wrote:
> > On 2014/05/03 21:52:16, rwlbuis wrote:
> > > I just realized that the bindings need it.
> > 
> > Lets still convert the callers to use
> > document->accessSVGExtensions().rootElement() and add the function
> > "SVGSVGElement* rootElement() const;" for the cases where we know which
> document
> > to use.
> 
> I did that. However some call sites need the non-const version of rootElement.
> 
> >
>
https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
> > File Source/core/page/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
> > Source/core/page/EventHandler.cpp:593:
> >
>
m_frame->document()->accessSVGExtensions().startPan(m_frame->view()->windowToContents(event.event().position()));
> > Nit: inside the if statement we can just use svgExtensions().
> 
> This needs the non-const one unfortunately.
> 
> >
>
https://codereview.chromium.org/268803005/diff/40001/Source/core/page/EventHa...
> > Source/core/page/EventHandler.cpp:1399:
> >
>
m_frame->document()->svgExtensions()->updatePan(m_frame->view()->windowToContents(m_lastKnownMousePosition));
> > Nit: It isn't obvious to me that this always hits an accessSVGExtensions()
> first
> > and it won't be obvious to future refactorers either. I'd prefer we switch
> these
> > lone ones to accessSVGExtensions()
> > (here, and in handleMouseReleaseEvent).
> 
> Done.

LGTM

[rwlbuis, 2014-05-05 19:55:17]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-05-05 19:55:33]

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

[commit-bot: I haz the power, 2014-05-05 21:07:11]

Change committed as 173338