Bug 20865 – Kill JSSVGLazyEventListener and cleanup event handling

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Kill JSSVGLazyEventListener, merge with JSLazyEventListener.

Cleanup all event handling related functions, rename & generalize the functions (dispatchHTMLEvent, setHTMLWindowEventListener etc..)

[Nikolas Zimmermann]

Created attachment 23450 [details]
Initial patch

[Eric Seidel (no email)]

Comment on attachment 23450 [details]
Initial patch

I'm not really sure ForType and ForTypeAndAtrribute are really worth the extra characters.  I think they're probably just as clear w/o the extra typing.

I think it's strange that a bool flips between "attached to a node" and "attached to the window".  That could also be a 2-value enum (and might be clearer that way).

Seems fine.  Certainly better than what we have today.  We'll probably go through another round of cleanup yet.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 23450 [details] [edit])
> I'm not really sure ForType and ForTypeAndAtrribute are really worth the extra
> characters.  I think they're probably just as clear w/o the extra typing.
I agree the name is weak, we should eventually find a nicer one, before landing. Though some extra characters should be preserved to differentiate between the DOM dispatchEvent/addEV/removeEV methods and our version.
> 
> I think it's strange that a bool flips between "attached to a node" and
> "attached to the window".  That could also be a 2-value enum (and might be
> clearer that way).
Yeah, I should talk to Sam, about that.

> Seems fine.  Certainly better than what we have today.  We'll probably go
> through another round of cleanup yet.
Definately. Thanks for r+ :-)

[Nikolas Zimmermann]

Landed in r36466.

[Darin Adler]

(In reply to comment #3)
> (In reply to comment #2)
> > I'm not really sure ForType and ForTypeAndAtrribute are really worth the extra
> > characters.  I think they're probably just as clear w/o the extra typing.
>
> I agree the name is weak, we should eventually find a nicer one, before
> landing. Though some extra characters should be preserved to differentiate
> between the DOM dispatchEvent/addEV/removeEV methods and our version.

Don't the extra arguments differentiate the function enough? Do we really need the different name? When function names get that long it starts to really hurt readability.

> > I think it's strange that a bool flips between "attached to a node" and
> > "attached to the window". That could also be a 2-value enum (and might be
> > clearer that way).
>
> Yeah, I should talk to Sam, about that.

I think an enum would be better than a bool. But how about two separate functions?

[Nikolas Zimmermann]

> Don't the extra arguments differentiate the function enough? Do we really need
> the different name? When function names get that long it starts to really hurt
> readability.

The arguments are indeed sufficient to differentiate, I could prepare a new patch, if you like.
Proposal:
setEventListenerForType -> setEventListener
setEventListenerForTypeAndAttribute -> setEventListenerByAttribute
removeEventListenerForType -> removeEventListener
eventListenerForType -> getEventListener

I don't really like getEventListener, maybe retrieve/cached/whatever as prefix?
I'm open for any suggestions.
 
> > > I think it's strange that a bool flips between "attached to a node" and
> > > "attached to the window". That could also be a 2-value enum (and might be
> > > clearer that way).
> >
> > Yeah, I should talk to Sam, about that.
> 
> I think an enum would be better than a bool. But how about two separate
> functions?
That's definately possible, but we'd need to store the enum anyways to differentiate between "attached to node" / "attached to window".