-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Imperatively define the global property references #648
Conversation
Commits to be squashed later, but I thought this would be easier to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful stuff. A few minor tweaks to make, mostly around scoping and naming.
index.bs
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly id has gone out of scope and should be pulled out.
index.bs
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interfaceObject is a bad name here; namedConstructor or maybe namedConstructorFactory would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed all comments except for the one below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit, yayyy
index.bs
Outdated
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|: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: above it's "For every id in [LegacyWindowAlias]'s identifiers"; here it's "For ever NamedConstructor identifier id". I like the first one more.
397f31d
to
535951e
Compare
535951e
to
752f29b
Compare
Preview | Diff