Bug 6010

Summary: SVGLoad event is not supported
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ian
Priority: P2 Keywords: SVGHitList
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.w3.org/TR/SVG/interact.html#LoadEvent
Bug Depends on: 10265    
Bug Blocks: 6009    
Attachments:
Description Flags
adds basic SVGLoad support
none
Same as above (sans tests and images)
none
patch addressing darin's concerns
darin: review-
final patch (with layout test updates, no malloc-avoiding optimization)
darin: review+
the plt suite I made for testing none

Description Eric Seidel (no email) 2005-12-08 04:26:30 PST
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.
Comment 2 Eric Seidel (no email) 2006-01-26 15:37:37 PST
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.
Comment 3 Eric Seidel (no email) 2006-07-27 20:22:53 PDT
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.
Comment 4 Eric Seidel (no email) 2006-07-27 20:27:42 PDT
Created attachment 9732 [details]
Same as above (sans tests and images)
Comment 5 Darin Adler 2006-07-27 20:30:06 PDT
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.
Comment 6 Eric Seidel (no email) 2006-07-27 20:40:00 PDT
(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.
Comment 7 Eric Seidel (no email) 2006-07-27 20:46:50 PDT
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();
Comment 8 Eric Seidel (no email) 2006-07-27 21:02:05 PDT
Created attachment 9733 [details]
patch addressing darin's concerns
Comment 9 Eric Seidel (no email) 2006-07-27 21:21:35 PDT
*** Bug 10125 has been marked as a duplicate of this bug. ***
Comment 10 Darin Adler 2006-07-28 08:34:55 PDT
Comment on attachment 9733 [details]
patch addressing darin's concerns

In haveLoadedRequiredResources I would have used a for loop.

r=me
Comment 11 Darin Adler 2006-07-28 08:35:36 PDT
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.
Comment 12 Eric Seidel (no email) 2006-07-28 12:12:39 PDT
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.)
Comment 13 Darin Adler 2006-07-29 07:48:25 PDT
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)
Comment 14 Eric Seidel (no email) 2006-07-31 02:54:01 PDT
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.
Comment 15 Eric Seidel (no email) 2006-08-05 01:26:15 PDT
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.
Comment 16 Eric Seidel (no email) 2006-08-11 22:57:35 PDT
Created attachment 9995 [details]
final patch (with layout test updates, no malloc-avoiding optimization)
Comment 17 Eric Seidel (no email) 2006-08-11 23:30:04 PDT
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.
Comment 18 Eric Seidel (no email) 2006-08-11 23:31:37 PDT
The test was with 5 runs, cached content.  I used a debug build running from withing Xcode.  It was not run under gdb.
Comment 19 Eric Seidel (no email) 2006-08-11 23:32:42 PDT
Created attachment 9997 [details]
the plt suite I made for testing
Comment 20 Darin Adler 2006-08-12 18:16:56 PDT
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().