1241783 - Make the binding generator automatically convert a mixin (as an arg type) into a union type.

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Boris Chiou [:boris]]

According to Bug 1174575 Comment #4, we want to use a mixin as an argument in a constructor [1], but our binding generator can't automatically convert this sort of IDL into a union type, so we have to use a workaround (defining a union manually) to achieve this purpose.

Therefore, I think we have to make the binding generator automatically convert this kind of IDL (mixin) into a union type.

[1]
Element implements Animatable;
CSSPseudoElement implements Animatable;

[Constructor (Animatable? target,
              object? frames,
              optional (unrestricted double or KeyframeEffectOptions) options)]
interface KeyframeEffectReadOnly : AnimationEffectReadOnly {
  readonly attribute Animatable?    target;
  ...
}

Spec: https://w3c.github.io/web-animations/#the-keyframeeffect-interfaces

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This is IDL that will likely be disallowed once we split interfaces into classes and mixins. Can the spec just use a union?

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to :Ms2ger from comment #1)
> This is IDL that will likely be disallowed once we split interfaces into
> classes and mixins. Can the spec just use a union?

Originally it did but feedback from Google (specifically Tab, I believe) was to add an interface for this instead.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Could you point me at the discussion so I can disagree there?

Comment 4

[Boris Zbarsky [:bzbarsky]]

So doing this in a systematic way by clearly separating out mixins from non-mixins such that the mixins never have a descriptor is a bit of a pain because we have some things that are both mixins and interfaces (at least for purposes of instanceof): ChromeWindow and XPathEvaluator.

Not to mention the SVG things from bug 1241898 and bug 1241899.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and the point is that this means we can't just rely on catching instances of this via missing descriptors throwing or something.  We have to go and change the guts of the object model generator in some way.  And we need to do this after the finish() calls, since that's when interfaces discover what they're consequential interfaces of...

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to :Ms2ger from comment #3)
> Could you point me at the discussion so I can disagree there?

I'm afraid this feedback was relayed in person. Feel free to post to public-fx (topic starting [web-animations]) though.

I'm quite ok with changing this to a union (we used to use a typedef for this union since it appears in a couple of places) and am happy to relay that feedback to Google when I meet with them next week--assuming we agree that's what should happen.

Comment 7

[Philip Jägenstedt [:foolip]]

This was linked from https://github.com/w3c/web-animations/issues/186, y'all might be interested in that.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Philip Jägenstedt from comment #7)
> This was linked from https://github.com/w3c/web-animations/issues/186, y'all
> might be interested in that.

Based on discussion in that bug, it seems like this IDL should not be allowed. I'll follow up here to drop the comments from the webidl in m-c then mark this as resolved.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Actually, I think it's probably clearer to mark this as invalid and file a separate bug for removing the comments.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Actually, I think it's probably clearer to mark this as invalid and file a
> separate bug for removing the comments.

That bug is bug 1357631.