[webkit-reviews] review granted: [Bug 24285] Text resource loading checks for BOM twice : [Attachment 28163] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 24285: Text resource loading checks for BOM twice
https://bugs.webkit.org/show_bug.cgi?id=24285

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * loader/CachedScript.cpp:
> +	   (WebCore::CachedScript::CachedScript):
> +	   (WebCore::CachedScript::setEncoding):
> +	   (WebCore::CachedScript::encoding):
> +	   (WebCore::CachedScript::script):
> +	   * loader/CachedScript.h:
> +	   * loader/appcache/ManifestParser.cpp:
> +	   (WebCore::parseManifest):
> +	   Use TextResourceDecoder, as TextEncoding::decode() no longer checks
for BOM.

Isn't this a bit more expensive? Are there really no differences in behavior
other than the BOM check by using the TextResourceDecoder?

>      if (m_source != UserChosenEncoding && m_source != AutoDetectedEncoding
&& encoding().isJapanese())
>	   detectJapaneseEncoding(data, len);
>  
> -    ASSERT(encoding().isValid());
> +    ASSERT(m_encoding.isValid());

There's a mix here of m_encoding and encoding(). We should consistently use
m_encoding, I think.

I'll say r=me for now. I'm thinking that maybe we should remove the decode
functions from TextEncoding, since that's just a convenience for newTextCodec.
What callers really need that and are they better served by having that helper
rather than calling newTextCodec directly?