[webkit-reviews] review denied: [Bug 39537] Teach HTML5 parser how to lex comments correctly : [Attachment 56825] 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 39537: Teach HTML5 parser how to lex comments correctly
https://bugs.webkit.org/show_bug.cgi?id=39537

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/html/HTML5Lexer.cpp:880
 +			ASSERT(*source == '-');
We should add an advanceAndASSERT() method to SegmentedString which does this
for you.

WebCore/html/HTML5Lexer.cpp:879
 +		    if (peek == SegmentedString::DidMatch) {
What does DidMatch mean?

WebCore/html/HTML5Lexer.cpp:915
 +		    m_token->appendToComment('-');
Where does the '-' come from?

WebCore/html/HTML5Token.h:167
 +	    return String(StringImpl::adopt(m_data));
This method can't be called more than once.  That seems bad, no?  Should we
ASSERT that m_data is non-empty here?

WebCore/html/HTML5Tokenizer.cpp:69
 +	default:
We should remove this default: and instead list all the missing states.

WebCore/platform/text/SegmentedString.h:120
 +	    ASSERT_NOT_REACHED();
SegmentedString seems like a rather mature API, seems lame to add an imature
method to it.


I think this needs one more round.  SEe above.