[webkit-reviews] review granted: [Bug 233481] SharedBufferChunkReader should be refactored : [Attachment 445164] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has granted Jean-Yves Avenard
[:jya] <jean-yves.avenard at apple.com>'s request for review:
Bug 233481: SharedBufferChunkReader should be refactored
https://bugs.webkit.org/show_bug.cgi?id=233481

Attachment 445164: Patch

https://bugs.webkit.org/attachment.cgi?id=445164&action=review




--- Comment #4 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445164
  --> https://bugs.webkit.org/attachment.cgi?id=445164
Patch

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

> Source/WebCore/ChangeLog:10
> +	   SharedBufferChunkReader required the SharedBuffer to be flatten
which
> +	   would mutate the underlying SharedBuffer.

"to be flattened"

But I'd rewrite this to describe the change being made, e.g.:

Avoid flattening a SharedBuffer when reading reading it through
SharedBufferChunkReader.

And maybe this can be the top line of the ChangeLog in place of "Rewrite
SharedBufferChunkReader", which is a bit non-specific.

> Source/WebCore/ChangeLog:11
> +	   the peek method was also incorrect if the data read overlapped
multiple

Similarly change this to describe the change being made:

Fix a bug in the peek method where too many characters would be read if the
requested size overlaps multiple segments.

> Source/WebCore/ChangeLog:17
> +	   We add it to makefiles and export the header so that API tests can
be written.

(Probably doesn't need to be mentioned.)

> Source/WebCore/platform/SharedBuffer.cpp:307
> +	   size_t segmentLength = std::min(currentSegment->segment->size(),
remaining);
> +	   data.append(currentSegment->segment->data(), segmentLength);

"segmentLength" sounds like the length of the segment, but I don't immediately
have a better suggestion.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:78
>	       auto currentCharacter = m_segment[m_segmentIndex++];
>	       if (currentCharacter != m_separator[m_separatorIndex]) {
>		   if (m_separatorIndex > 0) {

The existing code to check for separators doesn't work correctly with arbitrary
separator strings. Not sure if it matters. But for example if the buffer
contains "aaab" and the separator string is "aab", then it won't detect it as a
separator, since at m_segmentIndex = 2, it'll detect a mismatch, and reset
m_segmentIndex to 0.

Leave a FIXME in here?

> Source/WebCore/platform/SharedBufferChunkReader.cpp:98
> +	   if (++m_iteratorCurrent == m_iteratorEnd) {
>	       m_reachedEndOfFile = true;

Maybe set m_segment to nullptr just to be clean in here.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:137
>	   if (segmentLength > requestedSize)
>	       segmentLength = requestedSize;

Replace this with a std::min, since that's what we do elsewhere?

> Source/WebCore/platform/SharedBufferChunkReader.h:63
> +    const size_t m_bufferSize { 0 };

Can drop the initializer since both constructors initialize this. (And since
it's const, the compiler will complain if a constructor stops initializing it.)

> Source/WebCore/platform/SharedBufferChunkReader.h:66
> +    bool m_reachedEndOfFile { false };

I think we can get rid of m_reachedEndOfFile, and check `m_iteratorCurrent !=
m_iteratorEnd` instead. (In a private helper function, if you like.)

> Tools/TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:237
> +    const char* const simpleText = "This is a stupid test.";

s/stupid/simple/ if you want to be more positive. :-)