Remove support for the externalResourcesRequired attribute.

Remove support for the externalResourcesRequired attribute.

Intent: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/intent$20to$20remove/blink-dev/cyBqba-FxQU/fL208UQx3sEJ

Changes in behavior:

1. Load events should only be dispatched to elements that have some actual external content to load and to the outermost svg element.
2. Scripts should be killed/not executed if the svg is not xml wellformed.
3. Load events on elements that have external resources to fetch will not be dispatched until that resource is ready for use, similar to how externalResourcesRequired="true" used to work.

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

[Stephen Chennney, 2013-11-06 16:45:41]

Most of this is cleanup, but there are some serious questions in amongst it. I
am particularly concerned with the way that load events are fired - maybe we
need to entirely rethink it.

https://codereview.chromium.org/62083002/diff/1/LayoutTests/svg/custom/path-d...
File LayoutTests/svg/custom/path-domsubtreemodified-crash.html (right):

https://codereview.chromium.org/62083002/diff/1/LayoutTests/svg/custom/path-d...
LayoutTests/svg/custom/path-domsubtreemodified-crash.html:33:
externalResourcesRequired["baseVal"] = undefined;
I think you can remove this test and its result. I doubt it's still testing what
it was testing.

https://codereview.chromium.org/62083002/diff/1/Source/core/dom/DOMImplementa...
File Source/core/dom/DOMImplementation.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/dom/DOMImplementa...
Source/core/dom/DOMImplementation.cpp:149: //        addString(svgFeatures,
"ExternalResourcesRequired");
Remove the line entirely, I think.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGClipPathEl...
File Source/core/svg/SVGClipPathElement.cpp (left):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGClipPathEl...
Source/core/svg/SVGClipPathElement.cpp:59: if (supportedAttributes.isEmpty()) {
Remove braces.

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

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGGElement.c...
Source/core/svg/SVGGElement.cpp:52: bool SVGGElement::isSupportedAttribute(const
QualifiedName& attrName)
This can just return false now. If it's a virtual and the default is false, then
it need not exist at all.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageEleme...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageEleme...
Source/core/svg/SVGImageElement.cpp:196: bool
SVGImageElement::haveLoadedRequiredResources()
Does this method even need to exist any longer? It seems not. It's declared
virtual, but was non-virtual in SVGExternalResourcesRequired. It seems like that
was an error anyway.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageLoade...
File Source/core/svg/SVGImageLoader.cpp (left):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageLoade...
Source/core/svg/SVGImageLoader.cpp:45:
imageElement->sendSVGLoadEventIfPossible(true);
Don't we always need to dispatch a load event now, or at least modify the
sendSVGLoadEventIfPossible method? If not, do we even need
sendSVGLoadEventIfPossible?

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGPolyElement.h
File Source/core/svg/SVGPolyElement.h (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGPolyElemen...
Source/core/svg/SVGPolyElement.h:57:
BEGIN_DECLARE_ANIMATED_PROPERTIES(SVGPolyElement)
I think all this can go - we have no properties to declare.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGScriptElem...
File Source/core/svg/SVGScriptElement.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGScriptElem...
Source/core/svg/SVGScriptElement.cpp:218: void
SVGScriptElement::dispatchLoadEvent()
This effectively does nothing except assert. What should be happening here, if
anything?

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSwitchElem...
File Source/core/svg/SVGSwitchElement.h (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSwitchElem...
Source/core/svg/SVGSwitchElement.h:43:
BEGIN_DECLARE_ANIMATED_PROPERTIES(SVGSwitchElement)
Remove.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSymbolElem...
File Source/core/svg/SVGSymbolElement.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSymbolElem...
Source/core/svg/SVGSymbolElement.cpp:58: if (supportedAttributes.isEmpty()) {
Remove braces.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGUseElement.h
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGUseElement...
Source/core/svg/SVGUseElement.h:75: virtual bool haveLoadedRequiredResources() {
return m_haveFiredLoadEvent; }
Ditto. In particular, if these are virtual, what version are they overriding, or
what is overriding this one?

[Erik Dahlström (inactive), 2013-11-07 09:38:03]

I agree about the need to look over the load event handling in svg in general.
I'd be happy to take a deeper look at that either in this patch, or in a
followup patch.

https://codereview.chromium.org/62083002/diff/1/LayoutTests/svg/custom/path-d...
File LayoutTests/svg/custom/path-domsubtreemodified-crash.html (right):

https://codereview.chromium.org/62083002/diff/1/LayoutTests/svg/custom/path-d...
LayoutTests/svg/custom/path-domsubtreemodified-crash.html:33:
externalResourcesRequired["baseVal"] = undefined;
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> I think you can remove this test and its result. I doubt it's still testing
what
> it was testing.

fine by me, I was unsure what to do with it since I didn't have access rights to
the webkit bug it referred to.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGClipPathEl...
File Source/core/svg/SVGClipPathElement.cpp (left):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGClipPathEl...
Source/core/svg/SVGClipPathElement.cpp:59: if (supportedAttributes.isEmpty()) {
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> Remove braces.

Done.

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

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGGElement.c...
Source/core/svg/SVGGElement.cpp:52: bool SVGGElement::isSupportedAttribute(const
QualifiedName& attrName)
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> This can just return false now. If it's a virtual and the default is false,
then
> it need not exist at all.

Actually it was buggy before, it should have had SVGLangSpace attributes added
here.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageEleme...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageEleme...
Source/core/svg/SVGImageElement.cpp:196: bool
SVGImageElement::haveLoadedRequiredResources()
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> Does this method even need to exist any longer? It seems not. It's declared
> virtual, but was non-virtual in SVGExternalResourcesRequired. It seems like
that
> was an error anyway.

This method gets called from SVGElement::sendSVGLoadEventIfPossible.

IMHO this should really wait for the image to load completely (which I assume is
what hasPendingActivity would do), but I chose not to change how the code worked
in this first iteration.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageLoade...
File Source/core/svg/SVGImageLoader.cpp (left):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGImageLoade...
Source/core/svg/SVGImageLoader.cpp:45:
imageElement->sendSVGLoadEventIfPossible(true);
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> Don't we always need to dispatch a load event now, or at least modify the
> sendSVGLoadEventIfPossible method? If not, do we even need
> sendSVGLoadEventIfPossible?

My preference would be to dispatch the image load event here, possibly also
triggering load on the parent outermost svg element (if all resources have
loaded). It's not clear to me why dispatchLoadEvent should dispatch an error
event here, shouldn't that be done via ImageLoader::dispatchPendingErrorEvent?

Again, I opted for not changing the code if possible in the first iteration,
just assuming that eRR returns false everywhere and seeing what falls out.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGPolyElement.h
File Source/core/svg/SVGPolyElement.h (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGPolyElemen...
Source/core/svg/SVGPolyElement.h:57:
BEGIN_DECLARE_ANIMATED_PROPERTIES(SVGPolyElement)
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> I think all this can go - we have no properties to declare.

There are some BEGIN_REGISTER_ANIMATED_PROPERTIES and related macros in the cpp
file that fail to compile if this isn't here.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGScriptElem...
File Source/core/svg/SVGScriptElement.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGScriptElem...
Source/core/svg/SVGScriptElement.cpp:218: void
SVGScriptElement::dispatchLoadEvent()
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> This effectively does nothing except assert. What should be happening here, if
> anything?

I've copied the code from SVGExternalResourcesRequired::dispatchLoadEvent to
here (it was called in the header file), and removed dead code from assuming
eRR=false. This is the result of that.

What I'd really like for it to do is dispatch load here, possibly triggering
load on the parent outermost <svg> element.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSwitchElem...
File Source/core/svg/SVGSwitchElement.h (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSwitchElem...
Source/core/svg/SVGSwitchElement.h:43:
BEGIN_DECLARE_ANIMATED_PROPERTIES(SVGSwitchElement)
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> Remove.

Won't compile if removed due to BEGIN_REGISTER_ANIMATED_PROPERTIES and such in
the cpp file.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSymbolElem...
File Source/core/svg/SVGSymbolElement.cpp (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGSymbolElem...
Source/core/svg/SVGSymbolElement.cpp:58: if (supportedAttributes.isEmpty()) {
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> Remove braces.

Done.

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGUseElement.h
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/62083002/diff/1/Source/core/svg/SVGUseElement...
Source/core/svg/SVGUseElement.h:75: virtual bool haveLoadedRequiredResources() {
return m_haveFiredLoadEvent; }
On 2013/11/06 16:45:43, Stephen Chenney wrote:
> Ditto. In particular, if these are virtual, what version are they overriding,
or
> what is overriding this one?

SVGElement::haveLoadedRequiredResources is virtual, that's what's being
overridden.

The default implementation in SVGElement is essentially "go through all children
asking if they've loaded", if no element says "no" then return true. So it makes
sense to override the method on elements that load external resources.

[Stephen Chennney, 2013-12-18 20:56:23]

I dropped the ball on this in a big way. I think we should move ahead again.

How about a rebase and try runs and then we can commit.

I'm happy as is, but I don't want to looks good ... until try jobs pass.

[Erik Dahlström (inactive), 2014-01-08 08:14:44]

On 2013/12/18 20:56:23, Stephen Chenney wrote:
> I dropped the ball on this in a big way. I think we should move ahead again.
> 
> How about a rebase and try runs and then we can commit.
> 
> I'm happy as is, but I don't want to looks good ... until try jobs pass.

Ok, rebased, the win, mac and linux blink release builds are now green. Please
have another look.

[Stephen Chennney, 2014-01-08 15:52:55]

lgtm

[commit-bot: I haz the power, 2014-01-08 15:53:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/62083002/190001

[commit-bot: I haz the power, 2014-01-08 18:08:57]

Change committed as 164692