-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add "dealing with the event loop from other specifications" #2774
Conversation
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.
Generally looks great; thank you for putting this together. There are a few nits, and one specific issue with settings objects that needs to be addressed.
source
Outdated
<h5 id="event-loop-for-spec-authors">Dealing with the event loop from other specifications</h5> | ||
|
||
<p>Writing specifications that correctly interact with the <span>event loop</span> can be tricky. | ||
This is compounded by how this specification uses a platform-independent processing model, so we |
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.
Well, "threading-model-independent" is the really key part here.
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.
Changing to "concurrency-model-independent"
source
Outdated
<p>Writing specifications that correctly interact with the <span>event loop</span> can be tricky. | ||
This is compounded by how this specification uses a platform-independent processing model, so we | ||
say things like "<span>event loop</span>" and "<span>in parallel</span>" instead of using more | ||
familiar terms like "main thread" or "on a background thread".</p> |
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 makes it sounds like those terms "really" mean "main thread" or "on a background thread", but the whole point is that they don't.... I'm not sure whether that's just me, though.
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.
Changing to "using more familiar model-specific terms like..."
</ul> | ||
|
||
<p>The next complication is that, in algorithm sections that are <span>in parallel</span>, you | ||
must not create or manipulate objects associated to a specific <span>JavaScript realm</span>, |
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.
"JavaScript" or "ECMAScript"? Same elsewhere.
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.
JavaScript, per the note in https://html.spec.whatwg.org/multipage/infrastructure.html#dependencies:
The term "JavaScript" is used to refer to ECMA-262, rather than the official term ECMAScript, since the term JavaScript is more widely known.
(I was thinking about raising an issue about how this might no longer be true these days, but meh.)
algorithm steps are running <em><span>in parallel</span></em> to the JavaScript code.</p> | ||
|
||
<p>You can, however, manipulate specification-level data structures and values from the WHATWG | ||
Infra Standard, as those are realm-agnostic. They are never directly exposed to JavaScript without |
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 should be a link to the Infra Standard, ideally... I know you have a reference at the end of the paragraph.
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.
That's pretty against the style we use everywhere else, so I'll leave it as-is :-/.
source
Outdated
|
||
<li><p><span>Queue a task</span>, on the <span>networking task source</span>, to resolve | ||
<var>p</var> with a JavaScript array created from <var>encryptedURLs</var>, using the | ||
<code>Array</code> constructor from <var>global</var>.</p></li> |
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.
Is it actually possible to create an Array from an Infra List directly? If so, it would be good if the Infra definition of List linked to the definition of how it's done somewhere, because it's not obvious to me what defines that behavior.
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.
It is not, really. Infra lists can be treated as sequences (last sentence of https://heycam.github.io/webidl/#idl-sequence), so let me rejigger this to explicitly link to Web IDL conversions... Hard to strike a balance between ceremony and clarity here, given our underspecified promise primitives :(.
This also points out that IDL's "convert" algorithm needs to take a realm.
source
Outdated
<span>in parallel</span> steps. This is necessary, since parsing depends on the <span>current | ||
settings object</span>, which would no longer be current after going <span>in parallel</span>. | ||
Alternately, it could have saved a reference to the <span>current settings object</span> and | ||
used it during the <span>in parallel</span> steps; that would have been equivalent.</p></li> |
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.
No, that would have been a race. Parsing relative to a settings object gets the "API base URL" of the settings object, which is the "base URL" of the window's associated Document in the Window case, which is mutable. So if you start this algorithm and then mess with <base>
tags, it's really not clear what can/should happen if the URL parsing is happening in parallel.
What you could do is grab the relevant base URL (which is a string, hence immutable for our purposes) before going parallel and then do the URL parsing in the parallel section relative to that base URL.
In general, attempting to use a settings object in a parallel section is full of pitfalls like that, so I think our advice should be that you do not access settings objects in any way from parallel sections and instead grab the state you actually need from the settings object up front.
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.
Great catch, fixing and adding a new bullet. Let me know what you think.
f456ff4
to
861a73a
Compare
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, thank you!
This is immensely useful. Thank you for making it. |
This helps a lot indeed. Thank you! |
Thanks for writing this up @domenic, it's quite useful! |
This is an attempt to give guidance and examples for a common area of difficulty we've seen for spec authors. It is inspired directly by conversations with @equalsJeffH in IRC and in w3c/webauthn#472, although that's only the latest in a string of conversations. Other people I remember who might appreciate this: @jyasskin, @tobie.
@bzbarsky, would you mind reviewing this? Suggestions on things to add or cut welcome. @annevk would also be a great reviewer, but he's still on vacation for some time, so maybe after he gets back.
Preview version