[webkit-reviews] review denied: [Bug 21945] <!-- comment> -- > should be recognized as valid comment. : [Attachment 53436] Improved proposed fix: address Darin's comments

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 21945: <!-- comment> -- > should be recognized as valid comment.
https://bugs.webkit.org/show_bug.cgi?id=21945

Attachment 53436: Improved proposed fix: address Darin's comments
https://bugs.webkit.org/attachment.cgi?id=53436&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> What you may not see in the change is that we are inside a comment, thus the
> "-- >" would end the comment and we would badly fail the test. I could have
> added more spaces but I thought that entities would be better.

Indeed, I didn't see that. 

Entities are fine with me, but here are a few more ideas:
1. "- ->" instead of "-- >".
2. [#comment].
3. Change the whole block to a <script>, and use "//" comments for text.

> HTML definition of space character[1] matches XML white space. I had a look
at
> XML when making this change because this is an SGML property and thus sided
> with XML. 

HTML comments are not really SGML, see
<http://ln.hixie.ch/?start=1137799947&count=1> for some amusing history.

> Would isHTMLSpaceCharacter be OK with you?

This name sounds good to me. But HTML5 includes U+000C FORM FEED (FF) in the
set of space characters, which you don't have here:

+    return c == '\r' || c == '\n' || c == '\t' || c == ' ';

Please add a test for the form feed character.

+			&& m_scriptCode[m_scriptCodeSize - 3] == '-' &&
isXMLWhiteSpace(m_scriptCode[m_scriptCodeSize - 2])) {
+		 // HTML4 states that: White space (...) is permitted between
the comment close delimiter ("--")
+		 // and the markup declaration close delimiter (">"). See
https://bugs.webkit.org/show_bug.cgi?id=21945

HTML4 should not be a reason for us to make changes. Other browsers' behavior
and HTML5 draft (as long as it correctly captures browser behavior) would be
better references for this comment.

I'm very surprised that we will be only allowing a single space character
between "--" and ">". This doesn't seem to match any specification or any
browser. Am I missing something?

+		 // FIXME: HTML5 indicates that we need to emit a parsing error
in this case.

I believe this just means logging an error to developer console. Would that be
hard to do?

Marking r-, as this needs some changes.