Add mixins #433

tobie · 2017-09-01T15:51:34Z

~~This is an early peek. Still a number of things to figure out, but though some of you would be interested to check it out already.~~

As I mentioned on IRC, I feel like the mental model I'm using for mixins is that they're named interface partials that can extend more than one interface with a limited set of members.

~~Not asking for a review right now, but high-level issues/comments welcomed.~~

Discussion happening at #363.

Follow-up issues at #450.

Still to do:

specify order in which mixin members appear on interfaces (see Enumeration order of interface members #432).
check terminology for describing ES objects implementing members specified in mixins ("implements" is potentially confusing in that context).
fix dfn of mixin -> interface mixin (or perhaps interface/mixin).
decide whether mixin member (and friends) should move to interface mixin/member, etc.
check whether there are implements statements lying around.
rebase & squash commits.
splits changes to JS grammar script into its own commit.

Preview | Diff

TimothyGu · 2017-09-28T14:49:00Z

index.bs

+[=partial mixin=]
+definitions.
+
+Similarly, [=legacy callers=] must not be overloaded across


@tobie You might want rebase. Legacy callers are gone, and no one is missing them.

Oh. Good point.

domenic

I know you said not quite ready for review, but I couldn't resist :). This is looking so good!! The model is so much simpler and easier to understand this way.

Other things I noticed:

[Unforgeable] contains a "legacy mixin" example still
Some places that say "actually a mixin" (Ctrl+F for that) should be updated

domenic · 2017-09-29T20:32:13Z

index.bs

 used to define that objects implementing an interface will always
-also implement another interface.
+also implement a [=mixin=]


Maybe we should avoid the verb "implement" for mixins, in favor of e.g. "include the members defined by that mixin" or similar?

Absolutely. I need to check I didn't forget any. Thanks for catching this one.

Oops, didn't check context before answering this. This "implement" is related to JS objects implementing an interface, not interfaces implementing mixins as I initially thought. This is worth pondering a bit to find the right terminology here.

Modified the sentence slightly. But still kept the "implements" terminology, which really describes platform objects implementing interfaces and their members, not interfaces implementing other [NIO] interfaces.

domenic · 2017-09-29T20:33:32Z

index.bs

+
+Note: Mixins, much like [=partial interfaces=], are intended for use as a specification editorial aide,
+allowing a coherent set of functionalities to be grouped together,
+and included in multiple interfaces, possibly across documents.


We should forward-reference your note below about the difference between them.

domenic · 2017-09-29T20:33:53Z

index.bs

+A [=mixin=] is a specification of a set of <dfn export lt="mixin member">mixin members</dfn>
+(matching <emu-nt><a href="#prod-MixinMembers">MixinMembers</a></emu-nt>),
+which are the [=constants=], [=regular operations=], [=regular attributes=],
+and an eventual [=stringifier=],


What's "eventual" mean here?

Lol, I have no idea what I was thinking about when writing this, though I remember being deliberate about it. :-/ Will dig back into it.

domenic · 2017-09-29T20:35:15Z

index.bs

+Of the extended attributes defined in this specification,
+only the [{{Exposed}}] and [{{SecureContext}}] extended attributes are applicable to mixins.
+
+An <dfn>includes statement</dfn> is a definition


I don't think it's a definition. Maybe a statement is what is meant here? Although then you're just saying "statement" three times in the same sentence :)

I just parroted the text for "implements statements", here. Note the spec specifically defines "implements statements" as definitions in the definition of definition.

domenic · 2017-09-29T20:36:47Z

index.bs

+Similarly, for [=attributes=], each copy of the accessor property has
+distinct [=built-in function objects=] for its getters and setters.
+
+The order of appearance of an [=includes statement=] does not matter.


Not sure exactly what this means, but at least one interpretation is false, since A includes B; A includes C; is different from A includes C; A includes B; in terms of property enumeration order. (Kinda. It's all a bit fuzzy especially cross-specification.)

Well, as you note, mixin order is undefined if you have multiple documents. So how do we handle this?

We certainly shouldn't say it doesn't matter. Linking to an open issue for cross-document makes sense, while stating that it does matter within documents, perhaps.

What do we do with the following pre-existing sentence though (which I must have been heavily inspired by):

The order of appearance of an interface definition and any of its partial interface definitions does not matter.
—https://heycam.github.io/webidl/#dfn-partial-interface

Yeah, it's known wrong. #432

domenic · 2017-09-29T20:50:41Z

index.bs

+
+If [{{Exposed}}] appears both on a [=partial mixin=] and its original [=mixin=],
+then the [=partial mixin=]'s [=own exposure set=]
+must be a subset of the [=mixin=]'s [=own exposure set=].


Hmm, why "own" for mixin partials but not-own for interface and namespace partials? I imagine you've thought about this longer so I'm sure you've got it right, I'm just hoping you can explain it to me :)

For two reasons:

Mixins can't have an exposure set as they're just a container for mixin members that melts away when included in an interface, just like a document fragment does when added to a document. They's need to have multiple exposure sets (corresponding to all of the interfaces they get included in), or they'd need to be copied whenever they're included somewhere. So far it seemed like a better mental model to only create copies of mixin members when a mixin is included, and not also of the mixin itself (nor its partial). That prevents us from giving mixins an exposure set, which I think is actually a good thing; their editorial aides, not constructs designed to have JS bindings and show up in the JS environment.

For namespaces and interfaces, however, we can't rely solely on their "own exposure set"; we need to consider the case where they have none and need to default to « primary global ». This will no longer be an issue once we merge Make [Exposed] mandatory, remove [PrimaryGlobal] #423.

domenic · 2017-09-29T20:52:21Z

index.bs

+or the [=mixin=] it is a member of have an [=own exposure set=],
+then the [=mixin member=]'s [=exposure set=] is the [=set/intersection=]
+of the first of these [=own exposure set=] with the the [=host interface=]'s [=exposure set=].
+Otherwise, it is the [=host interface=]'s [=exposure set=].


Some grammar issues with this paragraph, and maybe some long sentences could be cut up. Also I think it's a statement of fact about the above algorithm, and not normative; if so, then maybe making that clearer would be good.

domenic · 2017-09-29T20:54:02Z

index.bs

+        [[ORIENTATION-EVENT]]
+        [[MEDIACAPTURE-STREAMS]]
+        (various [[WEBGL]] extension specifications)
+    </small>


It may be worth including a note about its usage to define what-used-to-be-mixins during a transitional period, so people don't say "those aren't the only ones, DOM uses it too!".

In fact maybe it's worth including a small paragraph in the mixins section about the pattern this is replacing, for the transitional period? Not sure, hmm. Thoughts from others appreciated.

Added a note.

domenic · 2017-09-29T20:56:11Z

index.bs

-    which are [=inherited interface|inherited=] or [=consequential interfaces=] of <code class="idl">A</code>,
+    The following [=IDL fragment=] defines a number of [=interfaces=] and [=mixins=]
+    which are respectively [=inherited interfaces=] of <code class="idl">A</code>
+    and [=mixins=] [=included=] by <code class="idl">A</code>,


The mixins aren't all included by A.

domenic · 2017-09-29T20:56:44Z

index.bs

-*   |A| and |A|’s
-    [=consequential interfaces=]
-    do not declare an [=interface member=]
+*   |A| does not declare an [=interface member=]


Should this be "member" too?

Good catch.

tobie · 2017-10-03T23:03:21Z

OK, this is now mature enough that a review would help.

In particular, the following would be super useful:

whether I forgot to handle whole sections of the spec,
whether the grammar is sound and still LL(1).

Note the todos left in #433 (comment).

domenic

Looking good.

I think the intro to the section on extended attributes needs some revision; currently it lists "interface members, namespace members, dictionary members".

Otherwise I couldn't find anything missed.

Having someone else check on the grammar would be ideal as I'm not very confident in my abilities there.

domenic · 2017-10-06T22:56:42Z

index.bs

+which means that it also implements all of |A|’s [=inherited interfaces=].
+In addition, an [=includes statement=] can be used
+to define that objects implementing an [=interface=] |A| will always
+also implement the [=mixin member|members=] of the [=mixins=] |A| [=includes=].


What does it mean to "implement" a member? Probably just "include"?

domenic · 2017-10-06T22:58:04Z

index.bs

+        PartialInterface
+        MixinRest
+</pre>
+
 <pre class="grammar" id="prod-PartialInterface">


Maybe this should become PartialInterfaceRest?

domenic · 2017-10-06T23:04:06Z

index.bs

+describe the behaviors that can be implemented by an object,
+as if they were specified on the [=interface=] that [=includes=] them.
+
+[=Static attributes=], [=static operations=], [=special operations=], except for [=stringifiers=], and


I think there should not be a comma before "except"

domenic · 2017-10-06T23:07:42Z

index.bs

+An <dfn>includes statement</dfn> is a definition
+(matching <emu-nt><a href="#prod-IncludesStatement">IncludesStatement</a></emu-nt>)
+used to declare that all objects implementing an [=interface=] |I| (identified by the first [=identifier=])
+must additionally implement [=mixin=] |M| (identified by the second identifier).


s/additionally implement/additionally include the members of/, or similar?

domenic · 2017-10-06T23:08:43Z

index.bs

+
+Each [=interface=] |I| that [=includes=] a [=mixin=] |M| must receive
+a copy of each of the [=mixin members|members=] of |M|.
+Each [=mixin member=] cooy is treated as if it had been declared on |I|.


s/cooy/copy/

domenic · 2017-10-06T23:11:40Z

index.bs

+<div class="example" id="example-mixin">
+
+    The following [=IDL fragment=] defines an [=interface=] and a [=mixin=] stating
+    that the [=mixin=] is always implemented on objects implementing the [=interface=].


s/stating that the/. The

(or similar. The sentence is weird right now.)

domenic · 2017-10-06T23:13:59Z

index.bs

+            <td>●</td>
+            <td>●</td>
+            <td>●</td>
+            <td>Only [=read only=] attributes.</td>


Nit: for this and "Only stringifier" I wouldn't include a full stop, myself.

domenic · 2017-10-06T23:14:24Z

index.bs

+            <th>[=Special Operations=]</th>
+            <td>●</td>
+            <td></td>
+            <td>Only [=stringifier=].</td>


Stringifiers, plural, I think?

domenic · 2017-10-06T23:15:57Z

index.bs

+Of the extended attributes defined in this specification,
+only the [{{Exposed}}] and [{{SecureContext}}] extended attributes are applicable to [=mixins=].
+
+An <dfn>includes statement</dfn> is a definition


Maybe we should have two subsections, "Includes statements" and "Using mixins and partials"? (For the latter, it would become a non-normative section, instead of a green-background note.)

Turned the second note into an informative section. Had to move it to the end of the section however, if not the grammar that's tagged on the end feels out of place. Will be tackling this further in #450.

domenic · 2017-10-06T23:17:04Z

index.bs


            if (!(/\bno-index\b/).test(pre.className)) {
-              output += html + "\n";
+              output += html.replace(/<emu-nt>/, "<emu-nt id=\"" + pre.id.replace("prod-", "index-prod-") + "\">")
+                .replace(/#prod-([^a-z])/g, "#index-prod-$1") + "\n";


Maybe this should be a separate commit?

Good point. Noted.

tobie · 2017-10-09T15:53:55Z

@bzbarsky would appreciate if you could have a look at the grammar and make sure it's still LL(1).

bzbarsky · 2017-10-09T16:01:59Z

I'm going to semi-punt to @heycam: I'm not very good at telling whether the grammar is behaving, short of implementing it and having my parser generator barf on me...

Cameron, if you have time to look over this, that would be great. If not, please let me know, and I'll see if I can get the tooling to give me answers. ;)

tobie · 2017-10-10T20:57:20Z

Adding an on demand @heycam as part of #458.

tobie · 2017-10-11T08:18:55Z

OK, this is ready to be "(green button) rebased and merged." Would appreciate someone giving it a last read, tbh. I'll do so myself again before merging it.

tobie · 2017-10-11T08:21:29Z

Note the WebIDL grammar is now checked as part of the travis build, so these changes are LL(1)-compliant.

annevk

Didn't check for correctness, but have read most of the words. Spotted one thing that looks like a typo.

annevk · 2017-10-11T08:23:48Z

index.bs

+which means that it also implements all of |A|’s [=inherited interfaces=].
+In addition, an [=includes statement=] can be used
+to define that objects implementing an [=interface=] |A|
+will always also includes the [=interface mixin member|members=]


s/includes/include/

travisleithead · 2017-10-11T18:33:38Z

+1 looks good

domenic

Nits around section structure, but LGTM, let's merge this! Very excited.

domenic · 2017-10-11T20:43:38Z

index.bs

+        identifier "includes" identifier ";"
+</pre>
+
+<h4 id="using-mixins-and-partials">Using Mixins and Partials</h4>


Section titles should be sentence case, not title case.

Again good catch.

domenic · 2017-10-11T20:45:14Z

index.bs

+only the [{{Exposed}}] and [{{SecureContext}}] extended attributes
+are applicable to [=interface mixins=].
+
+An <dfn>includes statement</dfn> is a definition


I think includes statements could be a subsection, similar to how implements statements are before this change.

Yes. Tracking this in #450. I'd like to find the right amount of consistency when splitting up these massive initial sections. Easier to do all of them at once.

domenic · 2017-10-11T20:47:40Z

index.bs

@@ -1128,6 +1164,325 @@ The following extended attributes are applicable to interfaces:
 </div>


+<h3 id="idl-mixins">Mixins</h3>


Should the ID/title here be "Interface mixins"? Or maybe there should immediately be a h4 for "Interface mixins"? Or we can not worry about it until you do dictionary mixins?

* Obsolete use of [NoInterfaceObject] extended attribute as mixins. * Add new interface mixin and partial interface mixin constructs. * Replace implements statement by includes statement which only accepts mixins on its rhs. * Remove supplemental interface and related concepts altogether. * Add generic members dfn. * Add table to clarify which members each construct accepts. * Refactor default toJSON operation, and [Exposed] and [SecureContext] algorithms accordingly. Closes whatwg#118. * Prevent operation overloading across mixins and interfaces. Closes whatwg#261. Closes whatwg#363. Closes whatwg#164. Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452. Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495.

Handling of these concepts was more complicated than (now) necessary. Inheritance: There's no reason to record inheritance separately in `IdlArray`'s `this.["inheritance"]`, since it can be determined just as easily from `this.members` via the `member.base` attributes. The concept of "consequential interfaces" in Web IDL went away with `implements` statements in whatwg/webidl#433 and here in #28619. `traverse_inherited_and_consequential_interfaces()` can be replaced with just `get_inheritance_stack()`. Mixins: For valid `A includes B` statements, `A` is always an interface and `B` is always an interface mixin, so there are no include chains or the possibility of cycles. `recursively_get_includes()` assumed this. Instead just save the `includes` statements found in `this.includes` and apply them in `merge_includes`, similar to partials. The handling of partials isn't changed, but `collapse_partials` is renamed to `merge_partials` to match the above. No observable differences in test results whatsoever are intended.

This example was adapted from a previous one in whatwg#433, but things are much simpler now and the example makes it seem more complicated than it really is. Interface mixins aren't special, so make the same point without them. Fixes whatwg#979.

This example was adapted from a previous one in whatwg#433, but things are much simpler now and the example makes it seem more complicated than it really is. Make the points inheritance and mixins separately. This also fixes the order of attributes on the returned JSON object, which was wrong the original example. The attributes of the base interface should be included first. Reverse the inheritance order in the example to make this clearer, matching `interface B : A` in two other examples. Fixes whatwg#979.

Handling of these concepts was more complicated than (now) necessary. No changes in the actual test results are intended. Inheritance: There's no reason to record inheritance separately in `IdlArray`'s `this.["inheritance"]`, since it can be determined just as easily from `this.members` via the `member.base` attributes. The concept of "consequential interfaces" in Web IDL went away with `implements` statements in whatwg/webidl#433 and here in #28619. `traverse_inherited_and_consequential_interfaces()` can be replaced with just `get_inheritance_stack()`. While this changes the behavior of `default_to_json_operation()`, there is no `toJSON` operation declared in a mixin in interfaces/, only in resources/ tests, so this change should not affect real tests. default_to_json_operation.html was updated along the lines of simplified examples in the spec: whatwg/webidl#980 Mixins: For valid `A includes B` statements, `A` is always an interface and `B` is always an interface mixin, so there are no include chains or the possibility of cycles. `recursively_get_includes()` assumed this. Instead just save the `includes` statements found in `this.includes` and apply them in `merge_mixins`, similar to partials. The handling of partials isn't changed, but `collapse_partials` is renamed to `merge_partials` to match the above. Not today: Further unification of partials and mixins is possible, and the handling of [Exposed] is missing for mixins, but this is left for later.

This example was adapted from a previous one in #433, but things are much simpler now and the example makes it seem more complicated than it really is. Make the points inheritance and mixins separately. This also fixes the order of attributes on the returned JSON object, which was wrong the original example. The attributes of the base interface should be included first. Reverse the inheritance order in the example to make this clearer, matching `interface B : A` in two other examples. Fixes #979.

…eritance and mixins, a=testonly Automatic update from web-platform-tests [idlharness.js] Simplify handling of inheritance and mixins (#28650) Handling of these concepts was more complicated than (now) necessary. No changes in the actual test results are intended. Inheritance: There's no reason to record inheritance separately in `IdlArray`'s `this.["inheritance"]`, since it can be determined just as easily from `this.members` via the `member.base` attributes. The concept of "consequential interfaces" in Web IDL went away with `implements` statements in whatwg/webidl#433 and here in web-platform-tests/wpt#28619. `traverse_inherited_and_consequential_interfaces()` can be replaced with just `get_inheritance_stack()`. While this changes the behavior of `default_to_json_operation()`, there is no `toJSON` operation declared in a mixin in interfaces/, only in resources/ tests, so this change should not affect real tests. default_to_json_operation.html was updated along the lines of simplified examples in the spec: whatwg/webidl#980 Mixins: For valid `A includes B` statements, `A` is always an interface and `B` is always an interface mixin, so there are no include chains or the possibility of cycles. `recursively_get_includes()` assumed this. Instead just save the `includes` statements found in `this.includes` and apply them in `merge_mixins`, similar to partials. The handling of partials isn't changed, but `collapse_partials` is renamed to `merge_partials` to match the above. Not today: Further unification of partials and mixins is possible, and the handling of [Exposed] is missing for mixins, but this is left for later. -- wpt-commits: b1f27e8ebbac23edecd374e6ea23f3c395498822 wpt-pr: 28650

tobie force-pushed the mixins branch from 3ac1f8e to 46db187 Compare September 1, 2017 15:56

domenic mentioned this pull request Sep 2, 2017

Designing mixins #363

Closed

tobie force-pushed the mixins branch from 21e40dd to 46db187 Compare September 27, 2017 15:54

TimothyGu reviewed Sep 28, 2017

View reviewed changes

tobie force-pushed the mixins branch from 87df662 to 8925431 Compare September 28, 2017 15:25

tobie mentioned this pull request Sep 28, 2017

What is the expected interaction of [SecureContext] with mixins? #118

Closed

domenic reviewed Sep 29, 2017

View reviewed changes

tobie force-pushed the mixins branch 6 times, most recently from c50578e to 8629d01 Compare October 1, 2017 21:51

bzbarsky mentioned this pull request Oct 3, 2017

domxpath/interfaces.html has no spec to back it up web-platform-tests/wpt#7562

Closed

tobie changed the title ~~[WIP] Add mixins~~ Add mixins Oct 3, 2017

domenic approved these changes Oct 6, 2017

View reviewed changes

tobie force-pushed the mixins branch 2 times, most recently from a33daf7 to 3425f3d Compare October 11, 2017 07:53

annevk reviewed Oct 11, 2017

View reviewed changes

tobie force-pushed the mixins branch from 5f7e368 to c9f2f0b Compare October 11, 2017 11:58

domenic approved these changes Oct 11, 2017

View reviewed changes

tobie force-pushed the mixins branch from 07f60bc to 8bd4d34 Compare October 11, 2017 20:59

saschanaz mentioned this pull request Nov 17, 2017

Python 3 compatibility for WebIDL.py emscripten-core/emscripten#5799

Merged

birtles mentioned this pull request Dec 5, 2017

[web-animations-1] Update WebIDL definition(s) to use new mixin syntax w3c/csswg-drafts#2076

Closed

1 task

saschanaz mentioned this pull request Apr 19, 2018

Conform to the latest Web IDL syntax WICG/speech-api#11

Merged

This was referenced Jul 17, 2018

Use Web IDL mixins browserext/browserext#57

Closed

Use Web IDL mixins browserext/native-messaging#12

Open

saschanaz mentioned this pull request Nov 19, 2018

Add [Exposed] and migrate to new mixin syntax WebBluetoothCG/web-bluetooth#415

Merged

CYBAI mentioned this pull request Dec 23, 2018

Update WebIDL codegen to support new mixin definitions servo/servo#22539

Closed

This was referenced Feb 11, 2019

[css-font-loading-3] Convert FontFaceSource to a mixin w3c/csswg-drafts#3642

Merged

[css-regions-1] Convert Region to a mixin w3c/csswg-drafts#3643

Merged

foolip mentioned this pull request Apr 23, 2021

[idlharness.js] Simplify handling of inheritance and mixins web-platform-tests/wpt#28650

Merged

foolip mentioned this pull request Apr 23, 2021

Editorial: simplify the example on default toJSON() #980

Merged

		@@ -1128,6 +1164,325 @@ The following extended attributes are applicable to interfaces:
		</div>


		<h3 id="idl-mixins">Mixins</h3>

Add mixins #433

Add mixins #433

Conversation

tobie commented Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Oct 3, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

tobie commented Oct 9, 2017

bzbarsky commented Oct 9, 2017

tobie commented Oct 10, 2017

tobie commented Oct 11, 2017 • edited Loading

tobie commented Oct 11, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisleithead commented Oct 11, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Sep 1, 2017 •

edited

Loading

tobie Sep 30, 2017 •

edited

Loading

tobie Oct 3, 2017 •

edited

Loading

tobie Oct 9, 2017 •

edited

Loading

tobie commented Oct 11, 2017 •

edited

Loading