Editorial: simplify EventListenerOptions processing #492
Conversation
| <code>{{EventListenerOptions/capture}}</code>. | ||
|
|
||
| <li><p>Return <var>capture</var>. | ||
| <li><p>Return <var>options</var>'s <code>{{EventListenerOptions/capture}}</code>. |
There was a problem hiding this comment.
This all looks nice (not sure why I thought the extra text was useful - sorry).
dom.bs
Outdated
| interface EventTarget { | ||
| void addEventListener(DOMString type, EventListener? callback, optional (AddEventListenerOptions or boolean) options); | ||
| void removeEventListener(DOMString type, EventListener? callback, optional (EventListenerOptions or boolean) options); | ||
| void addEventListener(DOMString type, EventListener? callback, optional (AddEventListenerOptions or boolean) options = false); |
There was a problem hiding this comment.
Why make this change?
IIRC we discussed this on my original PR. I could dig up the history but perhaps the argument against it was that it was observably identical to not specifying any default and instead relying on the dictionary defaults? If behavior is indeed identical with or without this change, then I'd prefer we leave it out - the meaning when named options seems more explicit. Also since we're now doing spec/impl IDL diffing, we'd want to get all the implementations to update which seems silly if there's no behavior difference whatsoever.
There was a problem hiding this comment.
This way we don't have to set the default value in prose. For some reason we didn't spot this solution in the original PR and we had all the defaults in prose and therefore I argued to not have them in IDL: #82 (diff).
There was a problem hiding this comment.
Actually, maybe this isn't needed because omitting the argument means an implicit dictionary gets passed. I'm not a 100% sure that's also true if the dictionary is part of a union, but it'd make sense.
There was a problem hiding this comment.
Ah yes, that's what happens: https://heycam.github.io/webidl/#es-union. Okay, I'll revert the IDL changes.
There was a problem hiding this comment.
Yeah, exactly. Our implementation also works that way too - getting the dictionary defaults automatically from the bindings generator, not explicitly in code (ignoring, for now, our special treatment of passive).
|
Thanks! |
@RByers what do you think?
Preview | Diff