Re: [heycam/webidl] Add legacy platform objects. (#230)

domenic approved this pull request.

Looks good. Some editorial nits and some things worth doing as a follow-up.

> @@ -2127,8 +2127,18 @@ If multiple legacy callers are specified on an interface, overload resolution
 is used to determine which legacy caller is invoked when the object is called
 as if it were a function.
 
-Legacy callers must not be defined to return a
-[=promise type=].
+[=Legacy callers=] can only be defined on interfaces that also
+[=support indexed properties|supports indexed=] or
+[=support named properties|named properties=].
+
+Note: This artificial restriction allows bundling all the interfaces
+which support these various legacy [=extended attributes=]

How about "bundling all interfaces with exotic object behavior"? What "various legacy extended attributes" were you thinking of?

> @@ -5073,6 +5083,13 @@ object that are considered to be platform objects:
 *   objects that implement a non-[=callback interface|callback=] [=interface=];
 *   objects representing IDL {{DOMException|DOMExceptions}}.
 
+<dfn id="dfn-legacy-platform-object" export>Legacy platform objects</dfn> are
+[=platform objects=] that implement an [=interface=] which
+does not have a [{{Global}}] or [{{PrimaryGlobal}}] [=extended attribute=],
+[=support indexed properties|supports indexed=] or

s/supports/support to match "objects"

>  
 *   The interface must not define a [=named property setter=].
+*   The interface must not define an [=indexed property getter=].

Probably add "or indexed property setter"?

>  
-If a [=platform object=] implements an [=interface=] that
-[=support indexed properties|supports indexed=] or
-[=support named properties|named properties=],
-the object will appear to have additional properties that correspond to the
-object’s indexed and named properties.  These properties are not “real” own
+[=Legacy platform objects=] will appear to have additional properties that correspond to the

s/the/their ?

>  properties on the object, but are made to look like they are by being exposed
 by the \[[GetOwnProperty]] internal method.
 
-However, when the [{{Global}}] or
-[{{PrimaryGlobal}}]
-[=extended attribute=] has been used,
-named properties are not exposed on the object but on another object
-in the prototype chain, the [=named properties object=].
-
 It is permissible for an object to implement multiple interfaces that support indexed properties.

Maybe as a follow-up we should disallow this. @bzbarsky, do you know of any existing interfaces that implement one or more interfaces that support indexed properties? (Or for that matter named properties.)

>  
-If a [=platform object=] implements an [=interface=] that
-[=support indexed properties|supports indexed=] or
-[=support named properties|named properties=],
-the object will appear to have additional properties that correspond to the
-object’s indexed and named properties.  These properties are not “real” own
+[=Legacy platform objects=] will appear to have additional properties that correspond to the
+[=indexed properties|indexed=] and [=named properties=].  These properties are not “real” own
 properties on the object, but are made to look like they are by being exposed
 by the \[[GetOwnProperty]] internal method.

Link this to the [[GOP]] method's definition?

>  
-    This should ensure that for objects with named properties, property resolution is done in the following order:
+    <div class="note">

IMO this note should not be part of the algorithm, i.e., as it was before is better.

>  </div>
 
-Support for [=getters=] is
-handled by the <a href="#getownproperty">platform object \[[GetOwnProperty]] method</a>
-defined in section [[#getownproperty]], and
-for [=setters=]
-by the <a href="#defineownproperty">platform object \[[DefineOwnProperty]] method</a>
-defined in section [[#defineownproperty]] and the <a href="#platformobjectset">platform object \[[Set]] method</a>
-defined in section [[#platformobjectset]].
+Support for [=getters=] is handled by the <a href="#legacy-platform-object-getownproperty">legacy platform object \[[GetOwnProperty]] method</a>,
+and for [=setters=] by the <a href="#legacy-platform-object-defineownproperty">legacy platform object \[[DefineOwnProperty]] method</a>,
+and the <a href="#legacy-platform-object-set">legacy platform object \[[Set]] method</a>.

No comma before the last "and"

>  
 
-<h4 id="getownproperty-guts" dfn>The PlatformObjectGetOwnProperty abstract operation</h4>
+<h4 id="getownproperty-guts" lt="LegacyPlatformObjectGetOwnProperty" dfn>The LegacyPlatformObjectGetOwnProperty abstract operation</h4>

Needs `abstract-op` attribute in the `h4`, I think instead of `dfn`. Then all references to it must be `<a abstract-op>...</a>` instead of `[=...=]`. This ensures casing is preserved in the "terms defined in this specification" section.

> @@ -12032,22 +12034,18 @@ defined in section [[#platformobjectset]].
 </div>
 
 
-<h4 id="getownproperty">Platform object \[[GetOwnProperty]] method</h4>
+<h4 id="legacy-platform-object-getownproperty" oldids="getownproperty">Legacy platform object \[[GetOwnProperty]] method</h4>

IMO these section headers should be simpler for easier scanning. Either just `\[[GetOwnProperty]]`, or maybe ES-style `\[[GetOwnProperty]] ( |P| )` like we've done in HTML (see https://html.spec.whatwg.org/multipage/browsers.html#windowproxy-getownproperty).

>  
 <div algorithm="to invoke the internal [[GetOwnProperty]] method of platform objects">
 
-    The internal \[[GetOwnProperty]] method of every [=platform object=] |O|
-    that implements an [=interface=] which
-    [=support indexed properties|supports indexed=] or
-    [=support named properties|named properties=]
+    The internal \[[GetOwnProperty]] method of every [=legacy platform object=] |O|

If you move the variable declarations into the header, these intro paragraphs can go away, as they have in HTML.

>  </div>
 
 
-<h4 id="invoking-indexed-setter">Invoking a platform object indexed property setter</h4>
+<h4 id="invoking-indexed-setter">Invoking a legacy platform object indexed property setter</h4>

As a follow-up: IMO this should be an abstract op, like LegacyPlatformObjectGetOwnProperty. And all the abstract ops should move after the method definitions, to the bottom of this section.

>      the following steps are taken:
 
-    1.  If |O| [=support indexed properties|supports indexed properties=] or
-        |O| [=support named properties|supports named properties=],
-        return <emu-val>false</emu-val>.
-    1.  Return [=OrdinaryPreventExtensions=](|O|).
+    1.  return <emu-val>false</emu-val>.

Capitalize `Return`

-- 
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/230#pullrequestreview-7898653