Conversation
whimboo
left a comment
whimboo
left a comment
There was a problem hiding this comment.
We don't seem to have wpt tests for all the cases. Maybe you could check especially for the invalid ones (including the maximum allows values)?
index.html
Outdated
| <li><p>Let <var>timer</var> be a <a>timer</a>. | ||
|
|
||
| <li><p><dfn>Start a timer</dfn> with <var>timer</var> | ||
| and <var>timeout</var>. Continue running this overall algorithm, |
There was a problem hiding this comment.
Question: Shouldn't this be phrased like for other cases with Run the following substeps [in parallel](https://html.spec.whatwg.org/multipage/infrastructure.html#in-parallel) and including the timer's timeout fired flag?
If yes, this would apply to other cases as well.
There was a problem hiding this comment.
I wonder if https://infra.spec.whatwg.org/#algorithm-conditional-abort applicable here
1: Run this steps but abort when timer's [=timer/timeout fired flag=] is set
...
2. If aborted, return an error with error
code timeout.
There was a problem hiding this comment.
Good call, thanks!
I've updated the most magical cases to use that, but kept the ones that were straightforwardly expressed as "wait for a or b". We could update it to use the "abort when" form everywhere, but I think this is OK?
There was a problem hiding this comment.
As reviewed 3 days ago it looks fine to me, as well the more recently pushed changes. So I'm happy with the PR to get merged.
This refactors the way that timeout configuration is used in the spec, and fixes several bugs, but does not otherwise have intentional normative changes. The main change is the creation of a "timeouts configuration" struct that is owned by the session. A new struct is created when the timeouts are changed (including during capabilities processing) and this fixes a number of confusing cases where the "JSON deserialize" algorithm was linked in place of the more specific algorithm for reading the timeout data. In addition a new "timer" class is created with a flag that's set when the required time has elapsed. This is now used in all places where we wait up to a timeout, and we assume we're able to wait for the flag to change, or check the current value of the flag, as required. Fixing this required fixing many cases where it was assumed that timeouts couldn't be null.
Co-authored-by: Alex Rudenko <[email protected]> Co-authored-by: Henrik Skupin <[email protected]>
|
I assume this CC is not meant to block on @nechaev-chromium to review? |
SHA: 6c86972 Reason: push, by jgraham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 6c86972 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 6c86972 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
4 participants
This refactors the way that timeout configuration is used in the spec, and fixes several bugs, but does not otherwise have intentional normative changes.
The main change is the creation of a "timeouts configuration" struct that is owned by the session. A new struct is created when the timeouts are changed (including during capabilities processing) and this fixes a number of confusing cases where the "JSON deserialize" algorithm was linked in place of the more specific algorithm for reading the timeout data.
In addition a new "timer" class is created with a flag that's set when the required time has elapsed. This is now used in all places where we wait up to a timeout, and we assume we're able to wait for the flag to change, or check the current value of the flag, as required. Fixing this required fixing many cases where it was assumed that timeouts couldn't be null.
Preview | Diff