Bug 157882 – Generate bindings code for EventTarget.addEventListener() / removeEventListener()

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Generate bindings code for EventTarget.addEventListener() / removeEventListener(). Custom code is current hardcoded in the bindings generator, we need to clean this up.

[Chris Dumez]

Created attachment 279347 [details]
Patch

[Chris Dumez]

Created attachment 279349 [details]
Patch

[Chris Dumez]

Created attachment 279351 [details]
Patch

[Chris Dumez]

Created attachment 279352 [details]
Patch

[Chris Dumez]

Created attachment 279353 [details]
Patch

[Chris Dumez]

Created attachment 279354 [details]
Patch

[Chris Dumez]

Created attachment 279356 [details]
Patch

[Chris Dumez]

Created attachment 279358 [details]
Patch

[Darin Adler]

Comment on attachment 279358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279358&action=review

> Source/WebCore/dom/EventListenerMap.h:58
> +    bool remove(const AtomicString& eventType, EventListener&, bool useCapture, size_t& indexOfRemovedListener);

There’s no question that this is a step in the right direction. But really I think the remove functions should take a predicate (std::function, probably) telling them what to remove rather than an EventListener&. Expressing that predicate as a == function on an EventListener object is not the best way to do it. We should talk about this in person at some point.

> Source/WebCore/dom/RegisteredEventListener.h:34
> +        RegisteredEventListener(Ref<EventListener> listener, bool useCapture)

Should this be Ref&& instead of just Ref?

> LayoutTests/ChangeLog:11
> +        Rebaseline existing test now that the 'length' property of those functions
> +        have changed. This is because the IDL has been updated to match our
> +        implementation (all parameters are optional). While this does not match
> +        the specification, the new length now matches our actual behavior.

I don’t think this is good. I’d prefer that we still return length 2. Is there some easy way to do that?

[Chris Dumez]

Comment on attachment 279358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279358&action=review

>> LayoutTests/ChangeLog:11
>> +        the specification, the new length now matches our actual behavior.
> 
> I don’t think this is good. I’d prefer that we still return length 2. Is there some easy way to do that?

I don't think people actually rely on this. I do agree this is not great though.
The length computation relies on arguments being marked as optional or not in the IDL. Besides adding a new IDL extended attribute, I am not quite sure how to maintain the length.

[Chris Dumez]

Created attachment 279453 [details]
Patch

[Chris Dumez]

(In reply to comment #10)
> Comment on attachment 279358 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279358&action=review
> 
> >> LayoutTests/ChangeLog:11
> >> +        the specification, the new length now matches our actual behavior.
> > 
> > I don’t think this is good. I’d prefer that we still return length 2. Is there some easy way to do that?
> 
> I don't think people actually rely on this. I do agree this is not great
> though.
> The length computation relies on arguments being marked as optional or not
> in the IDL. Besides adding a new IDL extended attribute, I am not quite sure
> how to maintain the length.

I have't found an easy way to fix this one. Do you think it is worth maintaining the previous length at the cost of adding some kind of IDL attribute, e.g. [Length=2])?

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #10)
> > Comment on attachment 279358 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=279358&action=review
> > 
> > >> LayoutTests/ChangeLog:11
> > >> +        the specification, the new length now matches our actual behavior.
> > > 
> > > I don’t think this is good. I’d prefer that we still return length 2. Is there some easy way to do that?
> > 
> > I don't think people actually rely on this. I do agree this is not great
> > though.
> > The length computation relies on arguments being marked as optional or not
> > in the IDL. Besides adding a new IDL extended attribute, I am not quite sure
> > how to maintain the length.
> 
> I have't found an easy way to fix this one. Do you think it is worth
> maintaining the previous length at the cost of adding some kind of IDL
> attribute, e.g. [Length=2])?

Alternatively, we could align our behavior with the spec (and with Firefox and Chrome) and mark the 2 first parameters as mandatory. This is a bit riskier but this is right thing.

[Darin Adler]

Seems like we can add a temporary IDL extended attribute or temporary hard coded exception in the bindings script. Then we can remove it when we change behavior to not have the arguments be optional. No reason to temporarily return 0 since we have always returned 2 and we think that 2 is the right value long term.

[Chris Dumez]

Created attachment 279605 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279605 [details]
Patch

Clearing flags on attachment: 279605

Committed r201305: <http://trac.webkit.org/changeset/201305>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.