Bug 39537 – Teach HTML5 parser how to lex comments correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Teach HTML5 parser how to lex comments correctly

[Adam Barth]

Created attachment 56789 [details]
Work in progress

[Adam Barth]

Created attachment 56825 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 56825 [details]
Patch

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.

[Adam Barth]

Created attachment 56831 [details]
Patch

[WebKit Review Bot]

Attachment 56831 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/HTML5Lexer.cpp:879:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 56831 [details]
Patch

WebCore/html/HTML5Token.h:159
 +          return cachedData();
cachedDataString()?

WebCore/html/HTML5Tokenizer.cpp:46
 +      case HTML5Token::EndOfFile:
Sooooo much better!

WebCore/platform/text/SegmentedString.h:135
 +      void advanceAndASSERT(UChar expectedCharacter)
This will fail in RElease builds.  We need UNUSED_PARAM I think.

Otherwise looks OK.

[Adam Barth]

Created attachment 56832 [details]
Patch for landing

[Eric Seidel (no email)]

Attachment 56831 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2346102

[WebKit Commit Bot]

Comment on attachment 56832 [details]
Patch for landing

Clearing flags on attachment: 56832

Committed r60051: <http://trac.webkit.org/changeset/60051>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.