[webkit-reviews] review granted: [Bug 103866] Replace JSValue::description() with JSValue::dump(PrintStream&) : [Attachment 177205] the patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 103866: Replace JSValue::description() with JSValue::dump(PrintStream&)
https://bugs.webkit.org/show_bug.cgi?id=103866

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

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


Looks like your only build problem is an .exp file on Windows.

> Source/JavaScriptCore/ChangeLog:32
> +	   (JSValue):

Please remove bogus lines like these from your change log.

> Source/WTF/ChangeLog:12
> +	   (WTF):

Please remove bogus lines like this.

> Source/WTF/ChangeLog:15
> +	   (WTF):

Please remove bogus lines like this.

> Source/WTF/wtf/StringPrintStream.cpp:98
> +    return String::fromUTF8(m_buffer, m_next);

This can fail if there are invalid UTF-8 sequences in the buffer. And then
we’ll get a null string back from the fromUTF8 function. Do we want/need to
handle that?

> Source/WTF/wtf/StringPrintStream.h:63
> +// Convert to a String using UTF8 conversion.

I don’t think the UTF8 emphasis in this comment is helpful. If we wanted to
emphasize UTF-8, then we should have done that in the name
StringPrintStream::toString. Since we don’t want to emphasize it there, there
no reason to emphasize it here.