1809408 - Do not expose stream-internal algorithms; implement separate exposed functions if needed

Status: RESOLVED FIXED

(Core :: DOM: Streams, enhancement)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

For example, ReadableStreamClose does not correspond to https://streams.spec.whatwg.org/#readablestream-close but it's way too easy to be confused.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Yeah... this is a recurring challenge; the 'un-named' spec operations which overlap with clearly defined 'internal' methods.

For anyone following, ReadableStreamClose instead corresponds to the same named 'internal' method.

Having said that, making sure we obey the spec invariant a bit earlier "The following algorithms must only be used on ReadableStream instances initialized via the above set up or set up with byte reading support algorithms (not, e.g., on web-developer-created instances):" is also important.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

I'm planning to move all the internal things to internal.h and exclude that header from moz.build. (The actual implementations are okay to be in the current files.)

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1809408 - Part 1: Move ReadableStream internal algorithms to internal namespace r=evilpie

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1809408 - Part 2: Move WritableStream internal algorithms to internal namespace r=evilpie

Depends on D167782

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1809408 - Part 3: Move TransformStream internal algorithms to internal namespace r=evilpie

Depends on D167829

Comment 6

[Tom S]

I don't think I like this approach. All functions declarations in one file, unrelated to the implementation, seems like a step backwards to me. Maybe we should consider introducing an internal namespace?

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Sounds like an option. That will still expose things, but at least people will see it's for internal things. Also less destructive in terms of VCS as moving things means harder to track history. I'll try that.

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Patches are up, but TBH I have a mixed feeling with this approach because it now contaminates code with namespace streams_internal here and there, and needlessly still exposing the namespace in the header. I don't think it's any bad to have internal functions in one internal header as IMO what's more important is where the impls are. Do you like the new ones better?

(I don't care too much as long as we can hide internal things, though. The new approach has less diffs, so that's also a thing... 🤔)

Comment 9

[Tom S]

I don't think it's terrible, the only thing that is annoying is the namespaces in the cpp files. Maybe we should actually consider moving some of these internal standalone functions to the corresponding classes. I did that a bit more while doing the WritableStream implementation and feels like better C++ to me.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

I think it's a little better; we could put it in the C++ classes, though it's less clear on the restrictions that streams_internal (what do you think about re-naming that to streams::internal, which feels less like a stylistic outlier. )

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #10)

I think it's a little better; we could put it in the C++ classes, though it's less clear on the restrictions that streams_internal (what do you think about re-naming that to streams::internal, which feels less like a stylistic outlier. )

Not sure I like it as dom::streams then would be a namespace whose sole member would be dom::streams::internal. That said, maybe it's better to rename it to _abstract at least, to match what the spec is saying.

(I'm copying the style of namespace bindings_detail)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bc810a8e68a
Part 1: Move ReadableStream internal algorithms to internal namespace r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/38da95750e3f
Part 2: Move WritableStream internal algorithms to internal namespace r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/7b9ab840dcf1
Part 3: Move TransformStream internal algorithms to internal namespace r=mgaudet

Comment 13

[Cristina Horotan [:chorotan]]

Backed out 3 changesets (bug 1809408) for bustages on ReadableStreamBYOBRequest.cpp

Backout: https://hg.mozilla.org/integration/autoland/rev/ccceaab42360cbe22012510085769ecc5c67d22a

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=7b9ab840dcf1f0f59d4b48adfad28adcaade7367

Failure log: https://treeherder.mozilla.org/logviewer?job_id=404750293&repo=autoland&lineNumber=22012

Comment 14

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/ccceaab42360

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b147225a5685
Part 1: Move ReadableStream internal algorithms to internal namespace r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/7ecf3a7d6acb
Part 2: Move WritableStream internal algorithms to internal namespace r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/3b3901fa30fc
Part 3: Move TransformStream internal algorithms to internal namespace r=mgaudet

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/b147225a5685
https://hg.mozilla.org/mozilla-central/rev/7ecf3a7d6acb
https://hg.mozilla.org/mozilla-central/rev/3b3901fa30fc