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

Rewrite specification with algorithms and expanded examples #105

Merged
merged 20 commits into from
Dec 4, 2020

Conversation

reillyeon
Copy link
Collaborator

This change is a large rewrite of the Serial API with some notable
changes:

  • The specification is renamed to the Web Serial API.
  • All interface attributes and methods have algorithmic steps.
  • Examples from the explainer have been integrated.
  • The Security and Privacy considerations sections have been expanded.

This change is a large rewrite of the Serial API with some notable
changes:

* The specification is renamed to the Web Serial API.
* All interface attributes and methods have algorithmic steps.
* Examples from the explainer have been integrated.
* The Security and Privacy considerations sections have been expanded.
@reillyeon reillyeon requested a review from domenic November 13, 2020 01:54
@reillyeon reillyeon mentioned this pull request Nov 13, 2020
5 tasks
Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This is looking great!

Keep in mind that for several patterns I only commented on the first instance of them, e.g. task queuing tweaks.

To summarize the potentially-substantive issues:

  • There's an implicit storage mechanism for "available serial ports on the system which the user has allowed the site to access", which is underspecified and raises a good number of questions.

  • You seem to be defining a lot of new exception types. We generally avoid that, and doing so usually requires coordinating with Web IDL. Maybe it shouldn't, though?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
respecConfig.js Outdated Show resolved Hide resolved
respecConfig.js Outdated Show resolved Hide resolved
@reillyeon
Copy link
Collaborator Author

I've pushed changes to this branch which I believe resolve most of your comments. Notably what is left to do is specify a storage mechanism for permissions and submit a PR to WebIDL to define the new DOMException types I use. I will be taking time off for next week so take your time reviewing these changes.

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Looking good. Only a few additional comments. Perhaps you'd want to tweak those, then merge this, with tracking issues for the DOMException and storage mechanisms?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@domenic
Copy link
Collaborator

domenic commented Dec 3, 2020

Also, it's worth going through the issue tracker after merging this and closing the issues which it fixes, e.g. #87.

index.html Outdated Show resolved Hide resolved
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.

3 participants