[webkit-reviews] review granted: [Bug 115848] Make TextCodecICU not depend on TextEncoding : [Attachment 201135] 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 115848: Make TextCodecICU not depend on TextEncoding
https://bugs.webkit.org/show_bug.cgi?id=115848

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201135&action=review


> Source/WebCore/platform/text/TextCodecICU.cpp:32
> +#include "TextEncodingRegistry.h"

Why is this include needed?

>>> Source/WebCore/platform/text/TextCodecICU.cpp:62
>>> +	 // TextEncoding name is always atomic, so the string is never deleted.

>> 
>> I am a little confused by "atomic" here. What do you mean?
> 
> TextEncodingRegistry has a map for encodings, see
atomicCanonicalTextEncodingName().

Maybe word more like this?

    // TextEncoding name is always one from atomicCanonicalTextEncodingName,
guaranteed to never be deleted.

> Source/WebCore/platform/text/TextCodecICU.cpp:230
> -    const char* name = m_encoding.name();
> -    m_needsGBKFallbacks = name[0] == 'G' && name[1] == 'B' && name[2] == 'K'
&& !name[3];
> +    m_needsGBKFallbacks = m_encodingName[0] == 'G' && m_encodingName[1] ==
'B' && m_encodingName[2] == 'K' && !m_encodingName[3];

Seems to be a slight decrease in readability. The original author (me) thought
that using a local made it more readable. It wasn’t performance optimization,
but expression readability optimization.

> Source/WebCore/platform/text/TextCodecICU.cpp:437
> +    if (shouldShowBackslashAsCurrencySymbolIn(m_encodingName)) {

Why not optimize further by searching for '\\' and not making a copy if there
is no '\\' character?

> Source/WebCore/platform/text/TextCodecICU.cpp:439
> +	   copy.append(characters, length);
> +	   copy.replace('\\', 0xA5);

This copies the characters twice. Instead, we could just use Vector<UChar> and
modify the copy in place.

> Source/WebCore/platform/text/TextCodecICU.h:45
> +	   TextCodecICU(const char* encoding);

Maybe mark this explicit?

> Source/WebCore/platform/text/TextCodecICU.h:59
> +	   const char* m_encodingName;

Maybe even const char* const?