[webkit-reviews] review denied: [Bug 59946] We need a utility class that read lines out of SharedBuffers : [Attachment 91938] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jay Civelli <jcivelli at chromium.org>'s
request for review:
Bug 59946: We need a utility class that read lines out of SharedBuffers
https://bugs.webkit.org/show_bug.cgi?id=59946

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91938&action=review

Below are a ton of nit-picks.  I'm not super excited about the recursive
approach to this problem.  Seems like we could get in trouble with a long line
broken into many segments.  It seems like what you really want here is a state
machine that gets fed a character at a time and keeps track of the CR/LF state
but is oblivious to the segmentation of the shared buffer.  Driving that, you
want a loop that's scanning through a buffer and driving the state machine. 
Driving that, you want a loop that's scanning through the buffer segments. 
That approach removes the recursion and most of the special cases for CR/LFs
splitting over segment boundaries.

We need to do something similar for the InputStreamPreprocessor in the HTML
parser:

http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLTokenizer.h
#L175

but I suspect it might be difficult to share code with the HTML parser because
they're at different layers and the HTML parser as some specialized needs.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-22764
> -			developmentRegion = English;

I think this means you should to upgrade your version of Xcode (although just
within the 3.x.x series).

> Source/WebCore/platform/SharedBufferLineReader.cpp:41
> +    , m_bufferPos(0)

m_bufferPos => m_bufferPosition

> Source/WebCore/platform/SharedBufferLineReader.cpp:57
> +void SharedBufferLineReader::readNextLine(StringBuilder* line)

WebKit uses reference parameters as out-params, not pointers.

> Source/WebCore/platform/SharedBufferLineReader.cpp:78
> +    int eolIndex = getEOLIndex();
> +    if (eolIndex != -1) {

Generally WebKit founds upon in-band signaling.

> Source/WebCore/platform/SharedBufferLineReader.cpp:87
> +    unsigned lenToCopy = m_segmentLength - m_segmentIndex -
(m_previousSegmentEndedWithCR ? 1 : 0);

lenToCopy => lengthToCopy.

> Source/WebCore/platform/SharedBufferLineReader.h:34
> +#include "PlatformString.h"

#include <wtf/text/WTFString.h>

> Source/WebCore/platform/SharedBufferLineReader.h:45
> +// Utility class that allows to read CR-LF terminated lines from a
SharedBuffer.
> +class SharedBufferLineReader {

I'd remove the comment and put the CR-LF notion in the name of the class
somehow.

> Source/WebCore/platform/SharedBufferLineReader.h:49
> +    bool isEOF() const { return m_eof; }

I'm not sure you need this function.  You can tell if you've reached EOF
because you'll get a null string (as distinguished from an empty string).

> Source/WebCore/platform/SharedBufferLineReader.h:52
> +    // Returns the next line read from the buffer.
> +    // Returns an empty string once the EOF has been reached

I'd remove this comment.  Also, you should return a null string at EOF rather
than an empty string.

> Source/WebCore/platform/SharedBufferLineReader.h:56
> +    void readNextLine(WTF::StringBuilder* line);

WTF:: isn't needed here.

> Source/WebCore/platform/SharedBufferLineReader.h:58
> +    // Reads a new segment. Returns false if the end of the buffer was
reached.

Probably should remove this comment.

> Source/WebCore/platform/SharedBufferLineReader.h:62
> +    // Returns the index of the next CR-LF, or -1 if the end of the segment
was reached without a CR-LF.
> +    int getEOLIndex();

Please don't start function names with "get".  As mentioned earlier, we prefer
not to have in-band signaling, but we do use it for find-type operations. 
Consider:

size_t findEndOfLine()

where you return notFound when there is not end of line.  Notice that we should
use size_t rather than int when talking about offsets in memory.

> Source/WebCore/platform/SharedBufferLineReader.h:65
> +    unsigned m_bufferPos;

Probably size_t

> Source/WebCore/platform/SharedBufferLineReader.h:69
> +    unsigned m_segmentLength;
> +    unsigned m_segmentIndex;

Also probably size_t

> Source/WebCore/platform/SharedBufferLineReader.h:70
> +    bool m_eof;

m_reachedEndOfFile ?

> Source/WebCore/platform/SharedBufferLineReader.h:76
> +#endif // SharedBufferLineReader_h

No need for this comment.