[webkit-reviews] review granted: [Bug 21945] Space should be allowed between -- and > in comment end : [Attachment 55560] Proposed fix: Move comment parsing to HTML5 (matches FF) and adds more test cases for broken comments

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 21945: Space should be allowed between -- and > in comment end
https://bugs.webkit.org/show_bug.cgi?id=21945

Attachment 55560: Proposed fix: Move comment parsing to HTML5 (matches FF) and
adds more test cases for broken comments
https://bugs.webkit.org/attachment.cgi?id=55560&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 Implemented the HTML comment parsing algorithm so that we match HTML5
and
+	 FF when parsing comments. Missing from this patch is

fast/parser/comments.html says "Output of this test should match WinIE". Is
this still true?

+	 the parser errors, which will be added in a follow up patch.
+.
+
+	 Added tests cases for broken comments.

Extra line with a dot here.

+    // FIXME: This code should not be called for these states!

Is "this code" the emitCommentToken() function? Are "these states" the ones
just below the comment? But a comment in the else clause tells us that this
code is "needed to properly parse broken comments", which doesn't seem like a
bad thing.

I don't understand what this FIXME is trying to tell me. It seems to be related
to a FIXME in HTMLTokenizer::parseComment(), but with a strange twist about
broken comments in else clause.

+    // HTML5 Comment state (see section 10.2.4.48 to 10.2.4.52).

The section numbers change all the time.

+    // FIXME: We are missing CommentStartState and CommentStartDashState.

Why does this need to be fixed, what is the observable effect? Would it be
better to add these states, and add FIXMEs at the points where we need to enter
and handle these?

r=me