-
Notifications
You must be signed in to change notification settings - Fork 48
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
WiP: editorials #301
WiP: editorials #301
Conversation
Signed-off-by: Zoltan Kis <[email protected]>
…dictionary etc. Signed-off-by: Zoltan Kis <[email protected]>
…mplementation]] internal slot. Signed-off-by: Zoltan Kis <[email protected]>
Signed-off-by: Zoltan Kis <[email protected]>
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.
Thanks @zolkis! A added a few minor suggestions.
index.bs
Outdated
1. Let |promise| be [=a new promise=]. | ||
1. Return |promise| and run the following steps [=in parallel=]. |
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.
I'd suggest indent the "in parallel" steps that follow so the in parallel steps start from index 1.
I'm used to seeing the "Return promise." on its own line after the "in parallel" substeps. Reading the in parallel definition I don't think there's any normative impact either way so we can return early too if that is a more common practice.
If we want to return the promise on the same line with "run the following steps in parallel", we may want to update compute()
similarly.
Maybe return promise on its own line maps more directly with the code that is written to implement this (also for tests) and thus less error prone?
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 problem, I can use the prose used in Web Bluetooth, Compute Pressure etc, or the prose used in WebRTC and others.
the method when invoked, MUST return a new promise promise and run the following steps in parallel:
or,
- If [condition] is true, return a promise rejected with a newly created WhateverError.
- (...) other steps
- Let p be a new promise.
- (all the other steps)
- Return p.
I have mostly used/preferred the former, since it gives a better hunch on what's happening in the event/task/microtask loop. The method really returns and the rest happen on a new timeline that implements "parallel" on the given underlying platform.
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.
I'll let the editors @huningxin @wchao1115 to share which convention they'd prefer to use.
…s. Move the section with permission policy after the Web IDL of the ML interface. Signed-off-by: Zoltan Kis <[email protected]>
<dl class=switch> | ||
<dt>{{MLContextOptions}} | ||
<dt>{{MLContextOptions}} or {{undefined}} |
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.
Good that you brought up the undefined type.
Should we handle undefined type as a separate case in this switch statement? Or maybe better to handle it as part of an Otherwise case to have a fall back to default for all unknown arguments including undefined?
Switch on options:
-> MLContextOptions
-> GPUDevice
-> Otherwise
Switch statement semantics are not specified in Web IDL, but it seems Otherwise is being popularized (out of default, otherwise, or else), notably by HTML LS.
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.
Let's discuss this on the call. I am open - just expressed the JS convention, meaning since we have a default value, and it falls under the same switch option, I handled them together. I am completely open on it, though.
If it's controversial, we should not use a switch, instead use separate, well defined steps.
First case: GPUDevice
Second case: undefined (defaults kick in)
Third case: arg defined. For each property of the argument also defined in MLContextOptions, do (...), otherwise for MLContextOptions not defined in the argument use defaults.
Otherwise: (fail).
Signed-off-by: Zoltan Kis <[email protected]>
@zolkis can we close this now? |
I will close (and likely remove) this one as a cleanup, after #310 is merged. |
The content here has been merged in other PRs. Closing. |
Content
For other interface using [[implementation]] as an internal slot, it's a bad name, kind of circular self-reference. We need to find another name. Implementation refers to the user agent code that implements the given algorithm(s).
Next, will improve / define algorithms where missing.
If there are commits that are trivial to merge, but others are more controversial, I can create new PRs with the accepted commits cherry-picked from here and leave this PR open.
Feel free to edit/commit here.
Preview | Diff