Bug 96749 – WTFString::show doesn't dump non-ASCII characters in a readable manner

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Glenn Adams]

To repeat: in debugger, invoke WTFString::show on a String that contains a non-ASCII character.  That non-ASCII character is dumped as '?' which isn't useful for debugging. It should dump a reasonable ASCII representation instead, e.g., using the \uXXXX syntax.

[Glenn Adams]

Created attachment 164093 [details]
Patch

[Glenn Adams]

Comment on attachment 164093 [details]
Patch

suggest benjaminp review

[Alexey Proskuryakov]

Comment on attachment 164093 [details]
Patch

We have existing functionality for escaping strings in WebCore/platform/text/TextCodec.h and in window.escape function implementation. I suggest either reusing existing code, or at least keeping implementations close together.

[Benjamin Poulain]

Comment on attachment 164093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164093&action=review

> Source/WTF/wtf/text/WTFString.cpp:1141
> -    buffer.resize(length + 1);
> -    for (unsigned i = 0; i < length; ++i) {
> +    unsigned bufferNeeds = 1;
> +    for (unsigned i = 0; i < length; i++) {
>          UChar ch = characters[i];
> -        buffer[i] = ch && (ch < 0x20 || ch > 0x7f) ? '?' : ch;
> +        bufferNeeds += !ch || ch < 0x20 || ch > 0x7f ? 6 : 1;
> +    }
> +
> +    buffer.resize(bufferNeeds);
> +    unsigned bufferUsed = 0;

The name bufferNeeds is not good enough, it poorly maps to the purpose of the variable.

This is debug code, it should be simple to read. Drop the first loop, use Vector::append() instead of handling the size manually. 

Why would you change ++i to i++?

> Source/WTF/wtf/text/WTFString.cpp:1143
> +        UChar ch = characters[i];

I would drop impl->characters() before and use (*impl)[i] here.

> Source/WTF/wtf/text/WTFString.cpp:1144
> +        if (!ch || ch < 0x20 || ch > 0x7f) {

We have isASCII() for that.

> Source/WTF/wtf/text/WTFString.cpp:1150
> +            for (unsigned k = 0; k < 4; k++, ch <<= 4) {
> +                static const char hexDigits[] = "0123456789ABCDEF";
> +                buffer[bufferUsed++] = hexDigits[(ch >> 12) & 0xF];
> +            }

We have functions for that in HexNumbers.h

[Glenn Adams]

(In reply to comment #3)
> (From update of attachment 164093 [details])
> We have existing functionality for escaping strings in WebCore/platform/text/TextCodec.h and in window.escape function implementation. I suggest either reusing existing code, or at least keeping implementations close together.

understood, but perhaps WTF should not be dependent on WebCore? yes?

[Benjamin Poulain]

> > We have existing functionality for escaping strings in WebCore/platform/text/TextCodec.h and in window.escape function implementation. I suggest either reusing existing code, or at least keeping implementations close together.
> 
> understood, but perhaps WTF should not be dependent on WebCore? yes?

That is not what Alexey suggested :)

WTF should not depend on WebCore, but it is common for features to move from JavaScriptCore and WebCore to WTF.
In this case, I am not entirely convinced it is necessary, but I haven't looked at TextCodec to see what can be done.

[Glenn Adams]

Created attachment 164263 [details]
address comments in comment #4

[Glenn Adams]

(In reply to comment #6)
> > > We have existing functionality for escaping strings in WebCore/platform/text/TextCodec.h and in window.escape function implementation. I suggest either reusing existing code, or at least keeping implementations close together.
> > 
> > understood, but perhaps WTF should not be dependent on WebCore? yes?
> 
> That is not what Alexey suggested :)
> 
> WTF should not depend on WebCore, but it is common for features to move from JavaScriptCore and WebCore to WTF.
> In this case, I am not entirely convinced it is necessary, but I haven't looked at TextCodec to see what can be done.

in this present case, my goal is simply to have WTFString::show do something more sensible with !isASCIIPrintable UChars; might I suggest another BZ be filed if there is a desire to do more (such as move other related WebCore functions into WTFString et al.)

[Benjamin Poulain]

> in this present case, my goal is simply to have WTFString::show do something more sensible with !isASCIIPrintable UChars; might I suggest another BZ be filed if there is a desire to do more (such as move other related WebCore functions into WTFString et al.)

WebKit has a shared responsibility codebase. As such, if there is a better patch to be done, it should be done.
Letting someone else clean after you is not a great option...

I'll have a look at the patch.

[Alexey Proskuryakov]

> In this case, I am not entirely convinced it is necessary, but I haven't looked at TextCodec to see what can be done.

Ditto. Having code like this in three places surely feels wrong though.

[Benjamin Poulain]

Comment on attachment 164263 [details]
address comments in comment #4

Looks good.

I don't think there is enough advantages to share the code with TextCodec.

[WebKit Review Bot]

Comment on attachment 164263 [details]
address comments in comment #4

Clearing flags on attachment: 164263

Committed r128682: <http://trac.webkit.org/changeset/128682>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Glenn Adams]

(In reply to comment #9)
> > in this present case, my goal is simply to have WTFString::show do something more sensible with !isASCIIPrintable UChars; might I suggest another BZ be filed if there is a desire to do more (such as move other related WebCore functions into WTFString et al.)
> 
> WebKit has a shared responsibility codebase. As such, if there is a better patch to be done, it should be done.
> Letting someone else clean after you is not a great option...
> 
> I'll have a look at the patch.

Understood. I'd be happy to work on "a better patch" or an additional patch if you or Alexey believe it is worth pursuing, in which case please give me a few hints of what you would like to see done.

I agree one shouldn't leave around detritus to be cleaned up, and hope I'm not doing that here.

Thanks for landing this one!

[Mark Rowe (bdash)]

This broke all of the Mac release builds, so I rolled it out in r128684.

[Mark Rowe (bdash)]

Example build log file is at <http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/3900/steps/compile-webkit/logs/stdio>.

[Glenn Adams]

Created attachment 164277 [details]
fix release build

[Glenn Adams]

Comment on attachment 164277 [details]
fix release build

mea culpa; looks like HexNumber.h can't be included without a template expansion that references hexDigitsForMode(); this patch update fixes the Mac builds problem by conditionalizing the inclusion

[Benjamin Poulain]

Comment on attachment 164277 [details]
fix release build

I would prefer HexNumber.h modified so that does not happen again.

[Darin Adler]

It’s not so great to dump in an ambiguous format. Probably need to change "\" into "\\" if we’re also going to use "\" as a metacharacter.

[Glenn Adams]

(In reply to comment #20)
> It’s not so great to dump in an ambiguous format. Probably need to change "\" into "\\" if we’re also going to use "\" as a metacharacter.

Good point (even though this is primarily intended for human reading, where it is unlikely to be mistaken). I'll make this fix and r? after someone lands the depends-on patch at [1].

[1] https://bugs.webkit.org/attachment.cgi?id=164304&action=review

[Glenn Adams]

Created attachment 164502 [details]
address comment #20

[Glenn Adams]

Comment on attachment 164502 [details]
address comment #20

all comments addressed, please review and land

[WebKit Review Bot]

Comment on attachment 164502 [details]
address comment #20

Clearing flags on attachment: 164502

Committed r128908: <http://trac.webkit.org/changeset/128908>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.