-
Notifications
You must be signed in to change notification settings - Fork 349
Editorial: align with new IDL patterns #1054
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
Conversation
Use "this" and "x steps are". Also use the relevant settings object in constructors, rather than current. (They are identical there, but it's better to encourage relevant.)
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.
Looks good; this is a nice upgrade. (I wonder if we've killed off WHATWG uses of "context object" at this point?)
I found a number of potential additional improvements while I was reading the output, but they're pretty much all preexisting issues.
</ol> | ||
|
||
<p>The static <dfn method for=Response><code>error()</code></dfn> method, when invoked, must run | ||
these steps: | ||
<p>The static <dfn method for=Response><code>error()</code></dfn> method steps are: | ||
|
||
<ol> | ||
<li><p>Let <var>r</var> be a new {{Response}} object, whose <a for=Response>response</a> is a new |
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.
This could link to https://heycam.github.io/webidl/#new and state the realm (current realm).
There are a total of 6 nearby object creations that could do the same.
@@ -6841,14 +6799,14 @@ partial interface mixin WindowOrWorkerGlobalScope { | |||
|
|||
<p>The | |||
<dfn id=dom-global-fetch method for=WindowOrWorkerGlobalScope><code>fetch(<var>input</var>, <var>init</var>)</code></dfn> | |||
method, must run these steps: | |||
method steps are: | |||
|
|||
<ol> | |||
<li><p>Let <var>p</var> be a new promise. |
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.
This could be [=a new promise=]
, although then you should specify the realm; I'd be curious whether it's relevant or current in implementations...
@@ -6841,14 +6799,14 @@ partial interface mixin WindowOrWorkerGlobalScope { | |||
|
|||
<p>The | |||
<dfn id=dom-global-fetch method for=WindowOrWorkerGlobalScope><code>fetch(<var>input</var>, <var>init</var>)</code></dfn> | |||
method, must run these steps: | |||
method steps are: |
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.
The object construction in here is pretty sketchy, but I imagine you're aware already, and any solutions would probably be larger diffs than makes sense for this PR.
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.
Yeah, there's a number of places where we don't specify realms and I basically need to write tests. But I also want that to be separate as it's no longer editorial.
Anything else to be done here?
|
LGTM |
Use "this" and "x steps are". Also use the relevant settings object in constructors, rather than current. (They are identical there, but it's better to encourage relevant.)
Preview | Diff