Bug 103866 – Replace JSValue::description() with JSValue::dump(PrintStream&)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

JSValue::description() does the horrible static char buf[thingy] thingy.

[Filip Pizlo]

Created attachment 177205 [details]
the patch

Let's see how many bots this breaks.

[Build Bot]

Comment on attachment 177205 [details]
the patch

Attachment 177205 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15119276

[Darin Adler]

Comment on attachment 177205 [details]
the patch

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.

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 177205 [details])
> 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.

Fixed.

> 
> > Source/WTF/ChangeLog:12
> > +        (WTF):
> 
> Please remove bogus lines like this.

Fixed.

> 
> > Source/WTF/ChangeLog:15
> > +        (WTF):
> 
> Please remove bogus lines like this.

Fixed.

> 
> > 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?

I don't think so.  This is used for debugging exclusively at this point, and I believe that in all cases the input is currently some combination of:

- String constants from our own code.

- Things that used to WTF::String but were converted to a CString via String::utf8().

If it ever becomes a problem I guess we could use fromUTF8WithLatin1Fallback.  Also, do we happen to have a fromUTF8() variant that inserts a '?' or other dummy marker character for cases where it encountered garbage?  This could be useful for example in the new profiler (https://bugs.webkit.org/show_bug.cgi?id=102999) if at some point something internally in JSC created an invalid UTF8 sequence - we'd still want to get some output rather than a null string, and falling all the way back to Latin1 might not be appropriate.

> 
> > 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.

I'll remove it.

[Filip Pizlo]

Created attachment 177344 [details]
patch for landing

I think that Windows will still have problems.  I will wait for Win EWS before landing.

[Build Bot]

Comment on attachment 177344 [details]
patch for landing

Attachment 177344 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15132173

[Filip Pizlo]

Created attachment 177416 [details]
patch for landing

Trying this again.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/136539