Re: [heycam/webidl] Add mixins (#433)

domenic approved this pull request.

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.

> @@ -794,11 +815,10 @@ interface |B| that inherits from |A|, and so on.
 Note that general multiple inheritance of interfaces is not supported, and
 objects also cannot implement arbitrary sets of interfaces.
 Objects can be defined to implement a single given interface |A|,
-which means that it also implements all of |A|’s
-[=inherited interfaces=].  In addition,
-an [=implements statement=] can be
-used to define that objects implementing an interface will always
-also implement another interface.
+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"?

>          PartialDictionary
         Namespace
 </pre>
 
+<pre class="grammar" id="prod-PartialInterfaceOrPartialMixin">
+    PartialInterfaceOrPartialMixin :
+        PartialInterface
+        MixinRest
+</pre>
+
 <pre class="grammar" id="prod-PartialInterface">

Maybe this should become PartialInterfaceRest?

> +allowing a coherent set of functionalities to be grouped together,
+and included in multiple interfaces, possibly across documents.
+They are not meant to be exposed through language bindings.
+Guidance on when to choose [=partial interface|partials=], [=mixins=], or [=partial mixins=]
+can be found in the <a href="#when-to-use-partials-vs-mixins">following note</a>.
+
+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 [=stringifiers=]
+that appear between the braces in the [=mixin=] [=declaration=].
+
+These [=constants=], [=regular operations=], [=regular attributes=], and [=stringifiers=]
+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"

> +      /* mixin_members... */
+    };
+</pre>
+
+The order that members appear in has significance for property enumeration
+in the <a href="#es-namespaces">ECMAScript binding</a>.
+
+Note that unlike [=interfaces=] or [=dictionaries=], [=mixins=] do not create types.
+
+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
+(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?

> +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).
+[=Interface=] |I| is said to <dfn>include</dfn> [=mixin=] |M|.
+
+<pre class="webidl" class="syntax">
+    interface_identifier includes mixin_indentifier;
+</pre>
+
+The first [=identifier=] must reference a [=callback interface|non-callback=] [=interface=] |I|.
+The second identifier must reference a [=mixin=] |M|.
+
+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/

> +(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).
+[=Interface=] |I| is said to <dfn>include</dfn> [=mixin=] |M|.
+
+<pre class="webidl" class="syntax">
+    interface_identifier includes mixin_indentifier;
+</pre>
+
+The first [=identifier=] must reference a [=callback interface|non-callback=] [=interface=] |I|.
+The second identifier must reference a [=mixin=] |M|.
+
+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|.
+Its <dfn>host interface</dfn> is |I|.

This paragraph is a bit weird as a "must" statement, but I get what it's trying to do. Maybe it could be made less weird by inserting something like "Per the appropriate language binding" or something, since as an implementer, the language-binding specific normative requirements are going to be the actually interesting ones?

> +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 [=includes statements=] affects the order in which [=mixin=]
+are [=included=] by their [=host interface=].
+
+Issue: [=mixin member|Member=] order isn't clearly specified,
+in particular when [=mixins=] are defined in separate documents.
+It is discussed in <a href="https://github.com/heycam/webidl/issues/432">issue #432</a>.
+
+No [=extended attributes=] defined in this specification are applicable to [=includes statements=].
+
+<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.)

> +        </tr>
+    </thead>
+    <tbody>
+        <tr>
+            <th>[=Constants=]</th>
+            <td>●</td>
+            <td>●</td>
+            <td>●</td>
+            <td></td>
+        </tr>
+        <tr>
+            <th>[=Regular attributes=]</th>
+            <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.

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

Stringifiers, plural, I think?

> +    };
+
+    partial interface mixin <mark>SomeMixin</mark> {
+      /* mixin_members... */
+    };
+</pre>
+
+The order that members appear in has significance for property enumeration
+in the <a href="#es-namespaces">ECMAScript binding</a>.
+
+Note that unlike [=interfaces=] or [=dictionaries=], [=mixins=] do not create types.
+
+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.)

>  
             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?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/433#pullrequestreview-67810838