Re: [heycam/webidl] Imperatively define the interface objects (#648)

domenic requested changes on this pull request.

Beautiful stuff. A few minor tweaks to make, mostly around scoping and naming.

> @@ -12434,6 +12432,12 @@ object is associated with.
             1.  Let |interfaceObject| be the result of [=create an interface object|creating
                 an interface object=] for |interface| with |id| in |realm|.
             1.  Perform [=!=] [$CreateMethodProperty$](|target|, |id|, |interfaceObject|).
+        1.  If the |interface| is declared with a [{{LegacyWindowAlias}}] [=extended attribute=],
+            and |target| implements the {{Window}} [=interface=], then:
+            1.  Assert: |interfaceObject| was created in the previous steps.

We tend to treat ifs as "block scope", so the interfaceObject variable went out of scope and shouldn't bused here. So, nesting under the previous bullet would be better. (But then we run into the issues you noted in #645.) You could alternately say "Let interfaceObject be null" in an outer scope.

> @@ -12438,6 +12436,12 @@ object is associated with.
             1.  [=list/iterate|For every=] [=LegacyWindowAlias identifier|identifier=] |id| in
                 [{{LegacyWindowAlias}}]'s [=LegacyWindowAlias identifier|identifiers=]:
                 1.  Perform [=!=] [$CreateMethodProperty$](|target|, |id|, |interfaceObject|).
+        1.  If the |interface| is declared with a [{{NamedConstructor}}] [=extended attribute=],
+            then:
+            1.  [=list/iterate|For every=] [=NamedConstructor identifier=] |id| on |interface|:

I'm prety sure there can only be on NamedConstructor identifier, so this loop is not necessary.

> @@ -12438,6 +12436,12 @@ object is associated with.
             1.  [=list/iterate|For every=] [=LegacyWindowAlias identifier|identifier=] |id| in
                 [{{LegacyWindowAlias}}]'s [=LegacyWindowAlias identifier|identifiers=]:
                 1.  Perform [=!=] [$CreateMethodProperty$](|target|, |id|, |interfaceObject|).
+        1.  If the |interface| is declared with a [{{NamedConstructor}}] [=extended attribute=],
+            then:
+            1.  [=list/iterate|For every=] [=NamedConstructor identifier=] |id| on |interface|:
+                1.  Let |interfaceObject| be the result of [=create a named constructor|creating

interfaceObject is a bad name here; namedConstructor or maybe namedConstructorFactory would be better.

> @@ -12442,6 +12441,12 @@ object is associated with.
                 1.  Let |interfaceObject| be the result of [=create a named constructor|creating
                     a named constructor=] with |id| for |interface| in |realm|.
                 1.  Perform [=!=] [$CreateMethodProperty$](|target|, |id|, |interfaceObject|).
+    1.  [=list/iterate|For every=] [=callback interface=] |interface| that is [=exposed=] in

"Define the interface objects" is no longer a good name for this algorithm, as it creates interface objects, legacy callback interface objects, and (in the next commit) namespace objects. Maybe "Define the top-level constructs" or "Define the global property references" or something.

> @@ -12434,6 +12432,12 @@ object is associated with.
             1.  Let |interfaceObject| be the result of [=create an interface object|creating
                 an interface object=] for |interface| with |id| in |realm|.
             1.  Perform [=!=] [$CreateMethodProperty$](|target|, |id|, |interfaceObject|).
+        1.  If the |interface| is declared with a [{{LegacyWindowAlias}}] [=extended attribute=],
+            and |target| implements the {{Window}} [=interface=], then:
+            1.  Assert: |interfaceObject| was created in the previous steps.

Similarly _id_ has gone out of scope and should be pulled out.

> @@ -12447,6 +12447,11 @@ object is associated with.
         1.  Let |interfaceObject| be the result of [=create a legacy callback interface
             object|creating a legacy callback interface object=] for |interface| with |id| in |realm|.
         1.  Perform [=!=] [$CreateMethodProperty$](|target|, |id|, |interfaceObject|).
+    1.  [=list/iterate|For every=] [=namespace=] |namespace| that is [=exposed=] in
+        |realm|:
+        1.  Let |namespaceObject| be the result of [=create a namespace object|creating a namespace object=] for |namespace| in |realm|.
+        1.  Let |id| be |namespace|'s [=identifier=].

Nit: in other cases you grab id first.

-- 
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/648#pullrequestreview-207017073