TextDecoder should properly handle streams

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Allow TextCodec::decode to properly handle streams

[Alex Christensen]

Created attachment 408046 [details]
Patch

[Darin Adler]

Comment on attachment 408046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408046&action=review

> Source/WebCore/platform/text/DecodeFailure.h:35
> +struct DecodeFailure {
> +    String stringBeforeError;
> +    size_t bytesConsumed { 0 };
> +};

Looking at the call sites, Expected isn’t really doing its job to make the code clean. Most of the call sites seem to want the string whether there was an error or not, but it seems they only need to know how many bytes were consumed if it was an error. Maybe the return value should just be a simple structure:

    struct DecodeResult {
        String string;
        Optional<size_t> bytesConsumedBeforeError;
    };

Or if you want to be even more straightforward:

    struct DecodeResult {
        String string;
        size_t bytesConsumed { 0 };
        bool success { false };
    };

I think those might be better than the Expected for how this is actually used.

[Alexey Proskuryakov]

Comment on attachment 408046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408046&action=review

> Source/WebCore/ChangeLog:8
> +        In order to properly handle cases like a stream breaking in the middle of a surrogate pair

Doesn't text decoding already properly handle such cases when decoding content as it comes from the network?

[Alex Christensen]

Comment on attachment 408046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408046&action=review

>> Source/WebCore/ChangeLog:8
>> +        In order to properly handle cases like a stream breaking in the middle of a surrogate pair
> 
> Doesn't text decoding already properly handle such cases when decoding content as it comes from the network?

That uses TextResourceDecoder, which stores a std::unique_ptr<TextCodec> which keeps state instead of keeping a buffer like TextDecoder currently does.  While this approach passes all existing web platform tests, it is incorrect.  I need to keep a std::unique_ptr<TextCodec> instead of a buffer, and I should probably add a test that fails with this implementation and passes with a correct implementation.  the only decoding failures in the existing tests are at the end of a stream block.

[Alex Christensen]

Created attachment 408067 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Darin Adler]

Comment on attachment 408067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408067&action=review

Excellent. This is exactly how the TextCodec class was designed to be used.

> Source/WebCore/dom/TextDecoder.h:54
> +    ~TextDecoder();

We typically put this before other member functions. Arbitrary, but it’s atypical to put it at the end of the public section.

> Source/WebCore/dom/TextDecoder.h:64
> +    std::unique_ptr<TextCodec> m_textCodec;

I would have named this just m_codec.

[Alex Christensen]

http://trac.webkit.org/r266668

[Radar WebKit Bug Importer]

<rdar://problem/68402719>

[Darin Adler]

Comment on attachment 408067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408067&action=review

> Source/WebCore/dom/TextDecoder.cpp:153
> +    auto oldBuffer = std::exchange(m_buffer, { });

Can we also come back here and delete this code?

> Source/WebCore/dom/TextDecoder.h:66
>      Vector<uint8_t> m_buffer;

And delete this?

[Alex Christensen]

Those suggested improvements were done in https://trac.webkit.org/changeset/266681