onload called too many times for <svg:image> We seem to be calling onload multiple times for each <svg:image> object.
Created attachment 17911 [details] test case (uses remote URL)
Created attachment 20363 [details] First attempt There were two problems here. First, that load was dispatched both when image was loaded and tag was closed. Second, that image load was dispatched to parents too, while the spec says: A SVGLoad event is dispatched only to the element to which the event applies; it is not dispatched to its ancestors. This patch fixes that. The testcase works, just not sure if external image is allowed, can change it to use a local image if needed. Cheers, Rob.
Bah. Unfortunately we need to better understand what proper SVGLoad event handling is before we can really make any changes to this code. To do so, we need at least 3 (possibly 4) test cases, as described below: 1. normal loading, sign up for all listeners, make sure they are in order (a single capture listener on the top element should work) 2. loading with externalResourcesRequired, make sure it delays all parents, but not siblings 3. using display: none, possibly (#4?) in combination with externalResoucesRequired Your attached patch removes code which was originally written to handle the externalResoucesRequired delay case. However, I'm not sure if <svg:image> sends its Load event right after tag close, or after image load? The spec is somewhat vague: http://www.w3.org/TR/SVG11/interact.html#LoadEvent The event is triggered at the point at which the user agent has fully parsed the element and its descendants and is ready to act appropriately upon that element, such as being ready to render the element to the target device. Referenced external resources that are required must be loaded, parsed and ready to render before the event is triggered. Optional external resources are not required to be ready for the event to be triggered. I think <image> has sorta an implicit "external resources required" on it, and thus delays its load event and all parent load events until its image is loaded, and thus it's ready for display. http://www.w3.org/TR/SVG11/struct.html#ExternalResourcesRequired externalResourcesRequired=true Indicates that resources external to the current document are required. If an external resource is not available, progressive rendering is suspended, the document's SVGLoad event is not fired and the animation timeline does not begin until that resource and all other required resources become available, have been parsed and are ready to be rendered. If a timeout event occurs on a required resource, then the document goes into an error state (see Error processing). The document remains in an error state until all required resources become available.
Comment on attachment 20363 [details] First attempt I think we need more test cases (see comments above) before we can really evaluate this patch.
Created attachment 20490 [details] Better approach This patch keeps the intend of the current code, delaying parent load dispatching until the image is loaded for externalResourcesRequired=true. Also it makes sure the event is dispatched to the parent node, the current code used "this"! With the new checks there are no multiple load dispatches. Comparing with other implementations is a bit hard; Opera does not seem to support externalResourcesRequired, saying: Failed attribute on image element: externalResourcesRequired="true". Both and FireFox and Opera just seem to dispatch the load for the image right after parsing the tag, but I don't think this is what the spec indicates. The malloc avoiding code is just an idea I have, and could be dealt with separately, though while messing with the load code I think it is convenient to attack that problem too. Cheers, Rob.
Comment on attachment 20490 [details] Better approach Nice! I don't think static RefPtr is "safe". the Event object would end up shared between frames, and would be unsafe. Data could survive on the JSEvent wrapper too (since those are generally cached based on pointer). I think that <image> should delay its onload until after its resources are loaded and ready to render anyway. I'm not sure what that means for externalResourcesRequired on <image> then... r- for the Event sharing issue.
Created attachment 20491 [details] Fix the "avoid malloc" part This patch introduces hasListenerType, could also be called hasListenerForType and/or moved into EventTargetNode. Cheers, Rob.
Comment on attachment 20491 [details] Fix the "avoid malloc" part Two comments: 1. I think SVG Load events might support capture, in which case you need to dispatch the event anyway (unless you walk up the parent chain and check to see if any parent has a capturing load event listener (which is what the comment is about). Also, hasListenerType(const AtomicString &eventType) we put the & next to AtomicString, per our style guidelines. Since you've pointed out yet another case that our testing missed, you might want to write a test case to verify that we correctly support capturing load event listeners (since this patch would have broken that). r- for the broken event capture support.
Created attachment 20492 [details] Fix the "avoid malloc" part part II This patch tries to do the ancestor capture listener check and adds a testcase for it, where it verifies that the capturing listener on the document element receives all load events. Cheers, Rob.
Comment on attachment 20492 [details] Fix the "avoid malloc" part part II Looks nice. From a first glance, it seems all issues Eric menioned, are fixed, right? In that case r=me.
Hi Niko, (In reply to comment #10) > (From update of attachment 20492 [details] [edit]) > Looks nice. From a first glance, it seems all issues Eric menioned, are fixed, > right? In that case r=me. Thnx for the r+! Eric had some issues about a method name IIRC, I'll try to contact him about that. Mfg, Rob.
The change looks fine. My comment over IRC was simply that: hasLoadListener doesn't actually do what it says. It's more: hasListenerForLoadEventTarget(element) (although that's kinda klunky name) The real fix of course is to fix the event system to use "EventPromise" stack-elements. I think this fix is fine until Darin or someone follows through with adding an EventPromise. Event promise is a stack-allocated object which knows how to malloc an Event with the right information, but only does so when required.
*** Bug 10264 has been marked as a duplicate of this bug. ***
*** Bug 12282 has been marked as a duplicate of this bug. ***
Landed in r32738.