Bug 6010 – SVGLoad event is not supported

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

[Eric Seidel (no email)]

WebKit+SVG does not support proper SVGLoad events

SVGLoad events are a bit tricky (and rather ill defined in the spec).  In order to implement them 
properly, we need to do something like this:

1.  When a close tag is hit on an SVGElementImpl, closeRenderer() (the current awful name) will be 
called.
2.  At this point, we need to check to see if this element has an onLoad listener.
3.  If this element has an onload listener, we need to check all of this element's children to see if any of 
them have externalResourcesRequired set (required resources), and if so, if those resources are not yet 
loaded.
3a.  If all all children have already loaded their required resources, we issue an SVGLoad event at this 
time (with proper capture/bubble symantics).
3b.  If NOT all of the children have loaded their required resources at this time, we must push some 
sort of data structure onto a "pending" stack (or toggle some flag on the element in question).
4.  Once a load is completed for an already-closed element, we again have to check to see if all of it's 
required children have been loaded yet, and if their Load events have been sent.  If all required children 
are loaded, we must then start at the deepest child who has not yet sent a Load event (who actually 
needs to, aka has a listener for one) and send that load event, then it's parents, etc. until we've reached 
the element who just finished loading.  At which time we continue processing.

One problem this may cause is possible DOM instability due to executing arbitrary code during parse 
time.  We'll have to guard against this in some manner.

*many* things in SVG are keyed off of this specific SVGLoad symantec.  One simple example is Xlink as 
noted in 6009.  

Note, that after asking graouts and dino, we have recieved clarification from the w3c that SVGLoad 
events are *not* issued due to programatic insertion, only due to actual parsing.

[Eric Seidel (no email)]

Also consider:
http://lists.w3.org/Archives/Public/www-svg/2003Oct/0055.html
http://www.w3.org/TR/SVG/interact.html#LoadEvent

[Eric Seidel (no email)]

This is a (strange) but basic SVG feature which we could not ship SVG without.  Adding SVGHitList keyword and bumping to P2.

Implementing SVGLoad events is somewhat non trivial.  There are a few things to consider:
1.  SVGElement needs to maintain state:
  a. Does it have any required external resources: externalResourcesRequired() returns true AND it actually has resources.
  b. If it's resources are loaded (assuming they are required)
2. SVGElement::closeRenderer() (or possibly XMLTokenizer) needs to be made smart enough to check if the subtree including the just-closed node had loaded all of its required resource it needs to *add this node* to the list of nodes ready to recieve a SVGLoad event, and then send any pending SVGLoad events.

[Eric Seidel (no email)]

Created attachment 9731 [details]
adds basic SVGLoad support

This supports pretty much all of SVGLoad that we need.  Things it doesn't support include:
1. externalResourcesRequired is not implemented correctly for all elements (only <image> currently)
2. removing/adding elements can change the load state.
3. document "error state" is not yet implemented, and a failed load is supposed to cause us to enter such, but doesn't yet.

[Eric Seidel (no email)]

Created attachment 9732 [details]
Same as above (sans tests and images)

[Darin Adler]

Comment on attachment 9731 [details]
adds basic SVGLoad support

Why is that assert correct in EventTargetNode::dispatchGenericEvent? Can't JavaScript create an event with a type of ""? The assertion seems wrong to me.

Can SVGElement::haveLoadedRequiredResources() be rewritten so that it's iterative instead of recursive?

The patch otherwise looks pretty good.

[Eric Seidel (no email)]

(In reply to comment #5)
> (From update of attachment 9731 [details] [edit])
> Why is that assert correct in EventTargetNode::dispatchGenericEvent? Can't
> JavaScript create an event with a type of ""? The assertion seems wrong to me.
> 
> Can SVGElement::haveLoadedRequiredResources() be rewritten so that it's
> iterative instead of recursive?
> 
> The patch otherwise looks pretty good.
> 

I can remove the assert change. I was simply trying to turn a // ### into an assert (since the later actual catches "bugs" instead of documenting them. ;)  I can change it to .isNull() instead.

Yes, I can make haveLoadedRequiredResources() iterative.  I'll do that now.

[Eric Seidel (no email)]

Yes, it looks like "!isNull()" should be a valid check.  I don't see any way to create an element with null type via javascript:

    case DOMEvent::InitEvent:
      event.initEvent(AtomicString(args[0]->toString(exec)), args[1]->toBoolean(exec), args[2]->toBoolean(exec));
      return jsUndefined();

[Eric Seidel (no email)]

Created attachment 9733 [details]
patch addressing darin's concerns

[Eric Seidel (no email)]

*** Bug 10125 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 9733 [details]
patch addressing darin's concerns

In haveLoadedRequiredResources I would have used a for loop.

r=me

[Darin Adler]

Comment on attachment 9731 [details]
adds basic SVGLoad support

r=me on the test changes, but the code changes from this patch are superseded by the newer patch.

[Eric Seidel (no email)]

Ok, so I goofed.  This patch excessively mallocs, and does not support capture.  It also seems to break on of the SVG layout tests after my last adjustment for darin.  So I'm going to roll it out.  (Actually, I'm going to ask ap to roll it out for me, since my internet connection here in MN totally sucks, and svn merge doesn't seem to like having a slow connection.)

[Darin Adler]

Comment on attachment 9733 [details]
patch addressing darin's concerns

Two problems I missed when reviewing:

    1) lots of layout tests failing
    2) creates events too often, so won't have good performance (Maciej's feedback)

[Eric Seidel (no email)]

My weekend of vaction (and some webkit hacking) is now over.  It will be several weeks until I get back to this.

If someone else would like to chaperone this through (olliej?) I'm fine with that.

After talking more with Maciej, I think that the simplest solution is to ignore the malloc (potential) slowdown for now (until we have an SVG PLT) and just change the event dispatch to use dispatchGenericEvent (I was confused before as to how capture worked, and thus had rolled my own dispatch).

The one layout-test hang was caused by caling "parentNode()" instead of "currentTarget->parentNode()" in my last patch.

That's all.

[Eric Seidel (no email)]

resolving bug 10264 would make things easier.  I don't view it as absolutely necessary to get that all in place before landing a (possibly slow) mallocing-on-every element-close solution though.

[Eric Seidel (no email)]

Created attachment 9995 [details]
final patch (with layout test updates, no malloc-avoiding optimization)

[Eric Seidel (no email)]

Per maciej's request, I made an SVG PLT Suite file which pointed to all of the SVG W3C tests and ran an approx before/after test (I left other apps running, including colloquy during the test):

Before (no SVGLoad)
57.415s, 0.065 mean, 0,050 sqrt mean

After (with SVGLoad)
58.466s, 0.066 mean, 0.052 sqrt mean

I'm still quite comfortable landing this, given that we've not even tried to optimize SVG.  We also have no external performance testing tools, making it a bit hard to expect "normal" developers (those who haven't worked internally) to do any performance testing.

[Eric Seidel (no email)]

The test was with 5 runs, cached content.  I used a debug build running from withing Xcode.  It was not run under gdb.

[Eric Seidel (no email)]

Created attachment 9997 [details]
the plt suite I made for testing

[Darin Adler]

Comment on attachment 9995 [details]
final patch (with layout test updates, no malloc-avoiding optimization)

Looks good. r=me

Code like this:

-        ExceptionCode ec;
-        RefPtr<Event> ev = m_doc->createEvent("HTMLEvents", ec);
-        ev->initEvent(readystatechangeEvent, true, true);
+        RefPtr<Event> ev = new Event(readystatechangeEvent, true, true);
         m_onReadyStateChangeListener->handleEvent(ev.get(), true);

can just be written as:
         m_onReadyStateChangeListener->handleEvent(new Event(readystatechangeEvent, true, true), true);

One of the nice things about PassRefPtr is that it enables that idiom, and it's actually more efficient too. There's no need for the get() in any case, since you can pass a RefPtr to a PassRefPtr without it. If you want to be more optimal you could use release() there instead of get().