Bug 74825 – We don't pass all of the html5lib unsafe-text.dat tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

We don't pass all of the html5lib unsafe-text.dat tests

[Adam Barth]

Created attachment 119783 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 119783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119783&action=review

> Source/WebCore/ChangeLog:14
> +        (WebCore::::shouldSkipNullCharacters):

::::?

> Source/WebCore/ChangeLog:24
> +            - We're supposed to replace NUL characters if the next character

if the next?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2821
> +        if (m_framesetOk && !isAllWhitespaceOrReplacementCharacters(characters))

Seems this subtly may need a comment.

[Adam Barth]

Comment on attachment 119783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119783&action=review

>> Source/WebCore/ChangeLog:24
>> +            - We're supposed to replace NUL characters if the next character
> 
> if the next?

Yeah, this setting on the tokenizer affects future tokens.

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2821
>> +        if (m_framesetOk && !isAllWhitespaceOrReplacementCharacters(characters))
> 
> Seems this subtly may need a comment.

I'm happy to add a comment.  I'm not sure what it would say other than to look at a specific line in the spec.  There's a test, which seems like the important thing.

[Eric Seidel (no email)]

Comment on attachment 119783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119783&action=review

>>> Source/WebCore/ChangeLog:24
>>> +            - We're supposed to replace NUL characters if the next character
>> 
>> if the next?
> 
> Yeah, this setting on the tokenizer affects future tokens.

Sure, but the sentence makes no sense.

>>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2821
>>> +        if (m_framesetOk && !isAllWhitespaceOrReplacementCharacters(characters))
>> 
>> Seems this subtly may need a comment.
> 
> I'm happy to add a comment.  I'm not sure what it would say other than to look at a specific line in the spec.  There's a test, which seems like the important thing.

If the spec mentions replacemnt characters here then that's fine. :)

[Adam Barth]

Created attachment 119870 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119870 [details]
Patch for landing

Clearing flags on attachment: 119870

Committed r103247: <http://trac.webkit.org/changeset/103247>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.