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

Remove "SendStream" Web IDL interface #307

Merged
merged 7 commits into from
Jul 20, 2021
Merged

Conversation

yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Jul 13, 2021

Have it a spec-only concept for streams created by the
"create a SendStream" procedure.

This is for #306.


Preview | Diff

Have it a spec-only concept for streams created by the
"create a SendStream" procedure.

This is for #306.
@yutakahirano yutakahirano requested a review from ricea July 13, 2021 06:10
@yutakahirano
Copy link
Contributor Author

@ricea, can you take a look?

I'm leaning to pretending the internal slots to be attached to SendStream (or WritableStream). We could define another holder object and pass it to write/close/abort algorithms, but that would be too confusing, given we already have stream and [[InternalStream]] in them.

@ricea
Copy link
Contributor

ricea commented Jul 13, 2021

I'm concerned that implementers may feel they actually have to create a way to attach extra data to a WritableStream. But maybe we could make it clearer in the note that this is just an abstraction. Maybe we could call it a "send stream" instead of a SendStream to reduce the expectation that it would literally appear in the implementation.

@yutakahirano
Copy link
Contributor Author

Added a note.

index.bs Outdated
@@ -1117,6 +1114,11 @@ A {{SendStream}} has the following internal slots.
<tbody>
</table>

Note: These internal slots need to be attached to {{SendStream}} (which is a kind of
{{WritableStream}}s) conceptually, not phisically. For example, implementers MAY have a weak hash
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the "s" in {{WritableStream}}s isn't needed.

Spelling: "physically".

I don't think you should use MAY within a note section.

In general, I would prefer to recommend an implementation strategy where the slots are owned by an object which is owned by the algorithms.

Something like

For example, an implementation could place them on an object which is
referenced from writeAlgorithm, closeAlgorithm and abortAlgorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed nits: thank you!

In general, I would prefer to recommend an implementation strategy where the slots are owned by an object which is owned by the algorithms.

I can do that: Just to confirm, are you fine with the recommendation with https://w3c.github.io/webtransport/#send-stream-STOP_SENDING?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. That algorithm references a "remoteCanceled" promise which isn't defined anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry I forgot to remove the step. I'll do that separately.

What I'm talking about here is the reference to the [[Transport]] internal slot at the first step.

  1. Let transport be stream’s [[Transport]].

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's okay. It's not ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm.

@jan-ivar jan-ivar merged commit 723a113 into main Jul 20, 2021
github-actions bot added a commit that referenced this pull request Jul 20, 2021
SHA: 723a113
Reason: push, by @jan-ivar

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yutakahirano yutakahirano deleted the yhirano/remove-send-stream branch August 26, 2021 11:56
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