Created attachment 58206 [details] Patch

Comment on attachment 58206 [details] Patch WebCore/html/HTML5Lexer.cpp:101 + if (value == 0 || value > 0x10FFFF) Did I just dream up the condition on the right? I thought I got it out of the spec....

Attachment 58206 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/html/HTML5Lexer.cpp:101: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #2) > (From update of attachment 58206 [details]) > WebCore/html/HTML5Lexer.cpp:101 > + if (value == 0 || value > 0x10FFFF) > Did I just dream up the condition on the right? I thought I got it out of the spec.... That condition is here for sure: http://www.whatwg.org/specs/web-apps/current-work/multipage/parsing.html#preprocessing-the-input-stream And it is enforced in InputStreamPreprocessor::peek(). However, I can't find the part of the spec that talks about entity processing and enforcing it here certainly prevents surrogate pair entities from working.

Fixing the test is good. I think our suragate pair handling is still confused. The spec: All U+0000 NULL characters and code points in the range U+D800 to U+DFFF in the input must be replaced by U+FFFD REPLACEMENT CHARACTERs. Any occurrences of such characters and code points are parse errors. That seems wrong. I guess I need to read the HTML5 spec more closely. U+D800–U+DBFF is the range used by the leading surrogate.

I think the spec is written in terms of characters, so I'm confused by the use of "code points" in "All U+0000 NULL characters and code points in the range U+D800 to U+DFFF". But I think the "code points" bit is just a mistake. I think that this pre-processing should already have been done at a layer below the InputStreamPreprocessor, so we can just hack out that code.

(In reply to comment #6) > I think the spec is written in terms of characters, so I'm confused by the use of "code points" in "All U+0000 NULL characters and code points in the range U+D800 to U+DFFF". But I think the "code points" bit is just a mistake. > > I think that this pre-processing should already have been done at a layer below the InputStreamPreprocessor, so we can just hack out that code. Yes. Code below InputStreamPreprocessor is handling surrogate pairs. That's why they work when the filter in legalEntityFor() is removed. Adam, let me know if this is off, but my assessment was that your checks in legalEntityFor were based on the spec in #preprocessing-the-input-stream. The bug was that those checks should only be enforced in peek() rather than prior to adding the entities to the stream.

Comment on attachment 58206 [details] Patch Ok. I'm still not sure I understand the issue 100%, but I'm starting to be convinced.

(In reply to comment #8) > (From update of attachment 58206 [details]) > Ok. I'm still not sure I understand the issue 100%, but I'm starting to be convinced. I've flipped the cq? flag. I'm assuming: 1. This is an appropriate style violation. 2. There isn't another part of the spec that I'm missing which legalEntityFor() was based upon (ie. it was based on #preprocessing-the-input-stream).

http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#tokenizing-character-references [[ Otherwise, if the number is in the range 0xD800 to 0xDFFF or is greater than 0x10FFFF, then this is a parse error. Return a U+FFFD REPLACEMENT CHARACTER. ]]

(In reply to comment #10) > http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#tokenizing-character-references > > [[ > Otherwise, if the number is in the range 0xD800 to 0xDFFF or is greater than 0x10FFFF, then this is a parse error. Return a U+FFFD REPLACEMENT CHARACTER. > ]] OK. So that means surrogate pair entities are expressly forbidden in HTML5. Since they were supported before, that leaves a compatibility question. How do we handle these LayoutTests which test for legacy behavior that is expressly forbidden in HTML5?

We note them in the spreadsheet. Can you test for how other browser (especially IE, Firefox, and Firefox nightly) behave?

(In reply to comment #12) > We note them in the spreadsheet. Can you test for how other browser (especially IE, Firefox, and Firefox nightly) behave? We are following the spec, so I'm marking this issue as WONTFIX, and I've noted it in the spreadsheet. In answer to your question, here's how various browsers perform on entity-surrogate-pairs.html: IE8: PASS Firefox 3.6: FAIL Minefield: FAIL WK legacy: PASS WK HTML5: FAIL It is interesting that comment in the test says "Firefox allows these". I suppose that means FF did at some point but that has now been removed. It is a better scary for compat that IE8 allows surrogate pair entities.

We should record all this information in a doc and send it to the HTML5 WG once we've got everything sorted out.

(In reply to comment #14) > We should record all this information in a doc and send it to the HTML5 WG once we've got everything sorted out. Presumably, we are going to end up with a spreadsheet of only lines that have an "N" in needs code change. Those will be the list of issues we'll have to resolve with the spec. I've added this bug number there so we can reference it when we prepare the list of issues.

See bug 6446 for why this was done (no known Web compatibility issues, just compatibility with other browsers). But thinking about this now, I like our forgiving behavior. There are processes on the Web that blindly encode all non-ASCII "characters" in input stream as numeric entities. I don't have a specific example. but depending on their definition of "character", they can end up with exactly this. And I'm not aware of any downside in supporting such split entities.

I've added your comment to our new document that's tracking these issues: https://docs.google.com/document/edit?id=1as5xYjyMSCph4960iz0-Kb7hZKf_L6f2vts57NMcVBI&hl=en Our current plan is to have the new parser 100% match the spec. If we want to do something differently, we probably want to change the spec and then move the implementation to match the updated spec.

For future reference: <rdar://problem/12747226>and <rdar://problem/10447418> track two regressions from switching to the new behavior.