Skip to content
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

spec: update remaining "in parallel" steps to use tasks #37

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

wacky6
Copy link
Member

@wacky6 wacky6 commented Nov 8, 2024

@wacky6 wacky6 requested a review from tidoust November 8, 2024 03:56
@wacky6
Copy link
Member Author

wacky6 commented Nov 8, 2024

tidoust: i've updated query/create algorithm in-parallel steps, ptal :)

Copy link
Contributor

@tidoust tidoust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor editorial comments. I think there's a third occurrence of a missing "s" in "run the following step" that I missed in the previous PR.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
1. [=/Resolve=] |p| with |result|.
1. Run the following step [=in parallel=]:
1. <a lt="convert">Convert |constraint| into a suitable form</a> for creating a platform-dependent [=handwriting recognizer=].
1. If the user agent can't create or prepare a [=handwriting recognizer=] to perform recognitions, [=queue a Handwriting Recognition API task=] to [=/reject=] |p| with a new {{DOMException}} according to the failure cause, then abort the remaining steps:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both steps "queue a Handwriting Recognition API task". Maybe worth re-writing as:

  1. [=Queue a Handwriting Recognition API task=] to:
    1. If the user agent can't create or prepare a [=handwriting recognizer=] to perform recognitions, [=/reject=] |p| with a new {{DOMException}} according to the failure cause:
      • If |constraint|'s ...
    2. Otherwise:
      1. Let |result| be...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this cause the sequence of event to be interpreted differently?

I think the "if checks" (e.g. check if user agent can instantiate a suitable recognizer) should happen outside of the enqueued tasks, because those checks has nothing to do with JavaScript / DOM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the change introduces any substantive difference, but I may have missed something ;)

The Infra Standard explains that "Conformance requirements phrased as algorithms or specific steps may be implemented in any manner, so long as the end result is equivalent. (In particular, the algorithms are intended to be easy to follow, and not intended to be performant.)"

Where and how the user agent checks the if condition is not observable by the application as far as I can tell. In particular, it does not trigger any event that the application could see. Regardless of how the algorithm is written, it seems totally fine for an implementation to check that condition within the enqueued task or outside of the enqueued task.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Thanks for the explanation. I've updated to use

Queue a task to:
- If, ...
- Otherwise, ...

wacky6 and others added 2 commits November 11, 2024 13:34
Co-authored-by: François Daoust <fd@tidoust.net>
@wacky6 wacky6 merged commit 89bd5f0 into main Nov 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants