[webkit-reviews] review denied: [Bug 40199] HTML5 parser should normalize line endings : [Attachment 57947] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 40199: HTML5 parser should normalize line endings
https://bugs.webkit.org/show_bug.cgi?id=40199

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/html/HTML5Lexer.cpp:335
 +  #define PEEK_AND_RECONSUME_IN(StateName)				      
\
Please add comments to these macros about when to use which.  It's not clear
when to use PEEK_AND_ vs. normal RECONSUME

WebCore/html/HTML5Lexer.cpp:338
 +	    if (!m_inputStreamPreprocessor.peek(source, m_lineNumber))	      
\
This might want to be a macro itself, sicne it's used other places. 
EXIT_IF_PEEK_FAIL or something?

WebCore/html/HTML5Lexer.cpp:413
 +	if (m_skipLeadingNewLineForListing) {
We might want to document in the header why both the input stream preprocessor
"skip next newline" and this m_skipLeadingNewLineForListing are needed. 
They're both needed.  Mostly to cover the <pre>\r\n case cleanly.

WebCore/html/HTML5Lexer.cpp:1160
 +		// We ignore the return value because it's checked by the loop.

This comment is not clear.  What's the return value?  How is it checked by
which loop?

WebCore/html/HTML5Lexer.h:138
 +		UChar nextInputCharacter() { return m_nextInputCharacter; }
const?

WebCore/html/HTML5Lexer.h:136
 +		InputStreamPreprocessor() : m_nextInputCharacter('\0'),
m_skipNextNewLine(false) { }
separate lines?

WebCore/html/HTML5Lexer.h:147
 +			    return false;
So I'm not sure what a false vs. true return here mean.  False means that there
is not enough data in the stream to be able to do the \r\n handling?

This just isn't clear.	You talked about adding more ASSERTs in
InputStreamPreprocessor to prevent double-peeking too.