[webkit-reviews] review requested: [Bug 47397] TextResourceDecoder::checkForHeadCharset can look way past the limit. : [Attachment 75929] updated patch

[bugzilla-daemon at webkit.org]

Jenn Braithwaite <jennb at chromium.org> has asked  for review:
Bug 47397: TextResourceDecoder::checkForHeadCharset can look way past the
limit.
https://bugs.webkit.org/show_bug.cgi?id=47397

Attachment 75929: updated patch
https://bugs.webkit.org/attachment.cgi?id=75929&action=review

------- Additional Comments from Jenn Braithwaite <jennb at chromium.org>
> --- Comment #30 from Adam Barth <abarth at webkit.org>  2010-12-07 19:18:27 PST
---
> (From update of attachment 75396)
> View in context:
https://bugs.webkit.org/attachment.cgi?id=75396&action=review
>
> Very nice patch.  I particularly like the ratio of testing to code change and
the large amount of code removed.  A few nits below.
>
>> WebCore/html/parser/HTMLMetaCharsetParser.cpp:43
>> +	: m_tokenizer(HTMLTokenizer::create(false))
>
> I wish I remembered what false does here.

Added a comment.

>> WebCore/html/parser/HTMLMetaCharsetParser.cpp:64
>> +	    pos += 7;
>
> Ideally we'd have a constant for this value to connect it with the string:
>
> const char* charsetString = "charset";
> const size_t charsetLength = sizeof("charset") - 1;
>
> Something like that.

Done.

>> WebCore/html/parser/HTMLMetaCharsetParser.cpp:80
>> +		quoteMark = value[pos++];
>
> static_cast<char> + ASSERT that it's a 7 bit value would be nice here.

Done.

>> WebCore/html/parser/HTMLMetaCharsetParser.cpp:108
>> +	    String attributeValue = String(iter->m_value.data(),
iter->m_value.size());
>
> You can just call the constructor instead of both the constructor and the
copy constructor (i.e, remove the = ).

Done.

>> WebCore/html/parser/HTMLMetaCharsetParser.cpp:113
>> +	    } else if (!charset.length()) {
>
> charset.isEmpty()

Done.

>> WebCore/html/parser/HTMLMetaCharsetParser.cpp:166
>> +		AtomicString tag(m_token.name().data(), m_token.name().size());

>
> I'd rename tag => tagName, but it's not that important.

Done.

Also, fixed a few minor typos in the tests and modified parser-tests.html to
keep the hixie's original indenting/formatting.