[webkit-reviews] review granted: [Bug 96264] Reuse floating point formatting of TextStream in RenderTreeAsText.cpp : [Attachment 163394] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 96264: Reuse floating point formatting of TextStream in
RenderTreeAsText.cpp
https://bugs.webkit.org/show_bug.cgi?id=96264

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163394&action=review


I like this, it is a better encapsulation.

> Source/WebCore/ChangeLog:10
> +	   RenderTreeAsText uses the same format for streaming out floats as
> +	   TextStream does. Replace formatNumberRespectingIntegers() with
> +	   a new overload to the operator<< to avoid code duplications.

I would like a bit more explanation.
1) What code duplication is saved.
2) Mention it takes advantage of StringBuilder::appendNumber() to avoid a
temporary StringImpl.

> Source/WebCore/platform/text/TextStream.cpp:29
> +#include <math.h>

MathExtras.h for future proofing it?

> Source/WebCore/platform/text/TextStream.cpp:119
> +TextStream& TextStream::operator<<(const FormatNumberRespectingIntegers&
value)

value -> formattedValue or something like that?
The following "value.value" is a little unfortunate for readability.

> Source/WebCore/rendering/RenderTreeAsText.h:-102
> -String formatNumberRespectingIntegers(double);
> -

Can you remove 
#include <wtf/MathExtras.h> ?