[karthik2804]

This commit moves the existing TextDecoder and TextEncoder implementations into a new encoding crate, and adds the corresponding stream classes.

[tschneidereit]

Thank you for this! I left a whole bunch of comments, a few of which definitely need addressing. Please push changes addressing them as new commits instead of force-pushing changes to the existing commit, so I can see what changed.

(Note: I'll probably make sense to start reading the comments in text_encoder_stream.rs first, because I started commenting there first, and some comments in text_decoder_stream.rs might make less sense otherwise.)

[tschneidereit]

no_trace doesn't have a name parameter. This doesn't cause a compile error only because the parameter is ignored.

Suggested change

	#[no_trace(name = "ignoreBOM")]
	#[no_trace]

[tschneidereit]

Function::new_callback takes impl ToJSVal, so no need to convert to a value here. You can just pass self to new_callback

Suggested change

let self_payload = scope.root_value(self.as_value());

[tschneidereit]

Suggested change

	self_payload,
	self,

[tschneidereit]

Suggested change

	self_payload,
	self,

[tschneidereit]

This is unfortunately unsafe: it allows code to intercept the set_property calls by installing setters on Object.prototype, and thus modify internal behavior of the builtin.

Additionally, if Object.prototype has start or cancel functions or a readableType member, those would all be used in setting up the TransformStream, and expose / enable modifying internal behavior.

This can be worked around by creating an object without a prototype. However, in this case we should do something entirely different to begin with: use the native transform and flush algorithms directly.

The way that could work is by mirroring ReadableStream::new_native, with a new function with this signature:

impl TransformStream<'_> {
    /// <https://streams.spec.whatwg.org/#transformstream-set-up>
    pub fn new_native<'r>(
        scope: &'r Scope<'_>,
        transform: AlgorithmArg<'r>,
        flush: AlgorithmArg<'r>,
        cancel: AlgorithmArg<'r>,
    ) -> Result<TransformStream<'r>, ExnThrown>;
}

The body would roughly be:

• js::class::create_instance_with::<TransformStreamImpl>, as create_readable_stream does.
• initialize_transform_stream with a start promise resolved with undefined, writable high-water mark 1, readable high-water mark 0, and no size algorithms.
• TransformStreamDefaultController::new.
• set_up_transform_stream_default_controller, passing AlgorithmArg::Native for the algorithms.

You could then avoid creating JSFunctions for transform and flush entirely.

[tschneidereit]

This is unsafe, because it retains the data() borrow across the calls into JS that enqueue can trigger. Those could then call back into the EncoderStream and take a data_mut, causing a mutable borrow to coexist with an immutable one.

[tschneidereit]

This step doesn't exist in the spec at all. Make sure to double-check that the step comments are fully accurate throughout the implementation, please. (The actual operation also happens to be redundant, since leading_surrogate is initialized to its default None value anyway.)

[tschneidereit]

I think you should be able to return an &'static str here, but that might be wrong. Would be good to try though, to avoid the allocation of a string we'll throw away immediately.

[karthik2804]

This works!

[tschneidereit]

This can also use cast_payload

[tschneidereit]

I think this can reuse most of TextDecoder::decode, if the core of that is split out into a pub(crate) helper function, with the decoder passed in as an argument.

That'd give us the fast paths provided by that implementation, and avoid a few perf hazards in here. In particular, the input is copied into a Vec instead of directly using the input buffer's bytes as a slice, and the decoded result is converted from a String to a JSString, running decoding again. Both are avoided by reusing a shared implementation.