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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 10 05:38:57 PDT 2009

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

Attachment 28163: proposed patch

------- 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?

More information about the webkit-reviews mailing list