[Webkit-unassigned] [Bug 49714] Yensign hack should work with Shift_JIS encoding

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49714


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74205|                            |commit-queue-
               Flag|                            |




--- Comment #3 from Alexey Proskuryakov <ap at webkit.org>  2010-11-18 10:53:02 PST ---
(From update of attachment 74205)
View in context: https://bugs.webkit.org/attachment.cgi?id=74205&action=review

Note that there are other places where we only check for the longer name - e.g. TextEncoding::backslashAsCurrencySymbol().

> Out of curiosity, does TextCodecMac have both Shift_JIS and Shift_JIS_X0213-2000?

Yes. The latter doesn't come from ICU, but from mac-encodings.txt.

I'd really expect a reviewer to know or at least research the answer to such a question before giving r+.

> WebCore/platform/text/TextEncoding.cpp:207
> +    // Shift_JIS_X0213-2000 is not the same encoding as Shift_JIS on Mac. We
> +    // need to check both of them.

This is a short line, it shouldn't be split into two.

> WebCore/platform/text/TextEncoding.cpp:210
> +    static const char* const shiftJis = atomicCanonicalTextEncodingName("Shift_JIS");
> +    static const char* const shiftJis2000 = atomicCanonicalTextEncodingName("Shift_JIS_X0213-2000");
> +    static const char* const eucJp = atomicCanonicalTextEncodingName("EUC-JP");

Coding style mistake - it's "Shift_JIS", not "Shift_Jis", so variables should be named accordingly. Ditto for EUC-JP.

> WebCore/platform/text/TextEncoding.cpp:211
> +    return ((shiftJis && m_name == shiftJis) || (shiftJis2000 && m_name == shiftJis2000) || (eucJp && m_name == eucJp)) ? 0x00A5 : '\\';

I'm not really sure how performance critical this function is, but it's certainly invoked a lot, so I wouldn't be adding unnecessary null checks.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.