[webkit-reviews] review granted: [Bug 36419] A backslash in EUC-JP becomes to a yen sign when it is copied : [Attachment 51234] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 36419: A backslash in EUC-JP becomes to a yen sign when it is copied
https://bugs.webkit.org/show_bug.cgi?id=36419

Attachment 51234: Patch v1
https://bugs.webkit.org/attachment.cgi?id=51234&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
This looks good enough for me to say r+. But it would be even better if Dan
and/or Darin could take a look, too. Some comments below.

> +(Note that this test makes sense only when you are using DumpRenderTree and
dumpAsText. You will see yen signs when you are using Safari because of
transcoding)

It seems that this test can be made to work in Safari with String.charCodeAt().


> +    for (TextIterator it(r, false, false, !isDisplayString); !it.atEnd();
it.advance()) {

This is really hard to read. It's a common problem in WebCore, but we are
trying to improve argument readability (usually by adding new overloaded
methods, or using named enums for argument values).

> +    bool m_isForCopying;

As you said, this name can be  confusing. I don't have a better suggestion, but
there should be at least a comment.

> +void RenderText::updateNeedsTranscode()
> +{
> +    const TextEncoding* encoding = document()->decoder() ?
&document()->decoder()->encoding() : 0;
> +    m_needsTranscode = (encoding && encoding->backslashAsCurrencySymbol() !=
'\\');
> +}

TextResourceDecoder always has an encoding, so you could save a null check by
writing this in a different way.

Thanks for the good test coverage!