[webkit-reviews] review granted: [Bug 54444] Correct handling of end of buffer partial sequence in UTF-8 and UTF-16 decoders when flushing with zero length : [Attachment 146994] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 54444: Correct handling of end of buffer partial sequence in UTF-8 and
UTF-16 decoders when flushing with zero length
https://bugs.webkit.org/show_bug.cgi?id=54444

Attachment 146994: Patch
https://bugs.webkit.org/attachment.cgi?id=146994&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146994&action=review


Please fix build issues found by EWS.

> Source/WebCore/platform/text/TextCodecUTF16.cpp:77
> +	   if (m_haveBufferedByte && flush) {
> +	       sawError = true;
> +	       if (!stopOnError)
> +		   return String(&replacementCharacter, 1);
> +	   }
> +	   return String();	   

I only had a very brief look at other decoders, but it seems like we may need
to reset to clean state when flush == true.

> Source/WebCore/platform/text/TextCodecUTF16.cpp:128
> +	   if (flush) {
> +	       sawError = true;
> +	       if (!stopOnError) {
> +		   size_t numCharsInBuffer = q - buffer.characters();
> +		   if (numCharsInBuffer == buffer.length()) {
> +		       buffer.resize(numCharsInBuffer + 1);
> +		       q = buffer.characters() + numCharsInBuffer;
> +		   }
> +		   *q++ = replacementCharacter;
> +	       }

Ditto.

> Source/WebCore/testing/Internals.h:51
> -class Internals : public RefCounted<Internals>,
> -		     public FrameDestructionObserver {
> +class Internals : public RefCounted<Internals>, private
FrameDestructionObserver {

Someone else is working on removing inheritance from FrameDestructionObserver
now.

> LayoutTests/fast/encoding/incremental-decoding.html:220
> +		   debug("Test requires WebKit internals object");

This is helpful for other browser vendors trying to open the test, but
necessarily for people opening it in a WebKit based browser.

Also, I'm surprised that the main branch is an else.