-
Notifications
You must be signed in to change notification settings - Fork 45
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
Use modern WebIDL and Infra standard conventions #210
Comments
I labeled this issue with "cr" implying we should address this issue before our Candidate Recommendation publication to satisfy "The document is considered complete and fit for purpose" requirement. |
Updated patterns for defining getters, setters, methods and constructors: https://webidl.spec.whatwg.org/#getter-steps |
@zolkis we discussed this issue on our call with a recommendation on tasks where to start: This issue is tagged as "cr" so we'll allocate time to discuss this on our bi-weeklies. Please bring any open issues or questions either to this issue or to the call. There is likely a lot of details that need to be clarified as you translate the current text into an algorithmic form. Thanks for your contributions! |
(API review questions that'll also inform this work on method steps discussed in #298) |
This issue is good aggregate tracker. Issues might be spanned on need, if the discussion is expected to be contained, or too big. |
I have been developing the features from the TODO in separate private branches in my fork. Some of them were published in PRs (some merged, some open). Most of them are still in private branches in my local repo. I will push them to my fork (without making PRs) so they become visible. I was asked to present all upcoming changes in one fork/branch, so that all changes could be reviewed with the full context. @huningxin : the improved steps that we worked out in the clamp PR (explicit steps to create platform objects for operands and activations) has been included. Should we next update the rest of the algorithms based on that? @wchao1115 you can take a look on the current state, and if you have any feedback, it would be good to fix and include those as early as possible (so far you had feedback on types used in internal slots, not using note style for argument descriptions and remove styling from titles). I have fixed the latter two. On how to make feedback: either in one or more issues, or I could make PRs to an upstream integration branch to be made for this purpose (e.g. zolkis-algorithms-integration or any other preferred name in the upstream repo) from which we could rebase/PR to main when the time comes. Could someone with access rights create that target integration branch? |
Thanks @zolkis. Can you please remove your current set of related PRs in the mainline PR queue, now that you have the entire change hosted in your fork? With the expectation that the change in this fork will be scoped to
the reviewers will review the entire change in the fork and once approved, merge it all at once when it's all ready to go. Any additional change after the merge is expected to be minimal and scoped to adjusting to the set of changes previously merged. Your private fork should therefore no longer be needed once the merge happens. This request was made with the intention to provide the reviewers with enough context behind the whole change, which will unavoidably affect the entire spec as a whole and not incremental in nature. The reviewers expect this entire change to be atomic, meaning that it will leave no follow-up action items after it is merged to the main content. It is important to remember that the change in this private fork is strictly scoped to the two types of changes noted above in order to avoid having a separate parallel content that is in the unmergeable state to the mainline content. |
@wchao1115: affected PRs will be closed when the alternative will have been set up. Right now it's not yet finished setting up.
How exactly these changes are proposed to be reviewed? IMO the most practical way to review and integrate these changes, without affecting the main branch, is using normal github practices, as follows:
About the content of the changes: bear in mind that these are actually adding algorithms, i.e. normative part of the spec. It might result in effects on the current text (so far not much, only arguments descriptions have been moved). Stylistic changes come as needed, like adding new (sub)sections and titles, collapsible algorithm blocks, visual framing of certain blocks (internal slots, validation, algorithms, etc.) IIRC no other changes were needed, and the existing changes fit pretty well in your requested policies. Any further, deeper changes can be done later via issues and PRs in the main branch (regular spec work). |
The These changes align the entire specification with modern specification conventions and add stylistic improvements on top that make navigating this specification more delightful experience. The following resources are made available to the group to assist in this review task:
Thanks @zolkis for this significant effort! |
@anssiko and @zolkis, do we know the ETA to merge |
@wchao1115 we agreed in the WG meeting to spend the time until the next WG meeting for proofreading, sanity check etc. Our implementation team helps in doing that, and I am making extra effort to handle all comments possible, and make issues for the larger or more complex changes to be handled later. The target is that by the next WG meeting, it will be in good enough shape to be merged, and then handle the rest soon after. |
@wchao1115, we expect to merge latest after our next meeting 24 Aug. We have a good momentum with reviews with an extended team pulled in to help with PR #446 review and @zolkis is focused on addressing the review comments directly in the integration branch (see commits). We open new issues for proposed changes more appropriate to be addressed after this big PR has been merged. Thanks for your efforts in driving this to completion, everyone! |
The big PR #446 has been merged, thank you all! Your contributions have been acknowledged in the specification. The modernized specification is now published at its official location (https://www.w3.org/TR/webnn/) as a Candidate Recommendation Draft to signal there have been significant changes to the previous published document 🚀 I'll leave this tracker issue open for a while to help the WG take to completion the few "review feedback" issues (noted in my comment above) we wanted to address on top of this big change. |
Per our discussion on today's call, I'd propose to retire this tracker issue and move to a GH label-based tracking of standards convention updates. I've asked @inexorabletash to help me with this task. Let me know when you've absorbed the useful information from this big meta issue so we can close this. |
Thanks @anssiko So far as I can tell, all of the topics mentioned here are either already fixed or are tracked by open issues. Here's how I'd categorize what's still open:
Further labels e.g. "PR needed" "has PR", "needs WPT", "has WPT" might be useful but that can be discussed separately. |
@inexorabletash much thanks for the label proposals and triage. I think we could document the proposed labels and their semantics in a .md file alongside code conventions and contrib guidelines. An idea up for grabs, PRs welcome :) |
Thanks everyone for your work on this WebIDL and Infra standard conventions alignment effort that has resulted in a significantly improved and more precise specification. Special thanks to @zolkis and @inexorabletash for this effort! I opened #549 and triaged the remaining open issues referenced here. We're now ready to close this tracker 👋 From now on standards conventions related issues can be found via this label: conventions |
The WebIDL spec recommends algorithmic method steps for defining operations: https://heycam.github.io/webidl/#method-steps
This spec currently uses a WebGPU spec inspired convention that differs from this best practice. We should consider adopting the "method steps" convention.
(Here's one example of method steps in context of the WebNN API spec:
https://webmachinelearning.github.io/webnn/#dom-ml-createcontext)
The text was updated successfully, but these errors were encountered: