Bug 96264 – Reuse floating point formatting of TextStream in RenderTreeAsText.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Patrick R. Gansterer]

Reuse floating point formatting of TextStream in RenderTreeAsText.cpp

[Patrick R. Gansterer]

Created attachment 163100 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163100 [details]
Patch

Attachment 163100 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13796870

[Peter Beverloo (cr-android ews)]

Comment on attachment 163100 [details]
Patch

Attachment 163100 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13796869

[Build Bot]

Comment on attachment 163100 [details]
Patch

Attachment 163100 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13800803

[Early Warning System Bot]

Comment on attachment 163100 [details]
Patch

Attachment 163100 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13798785

[Patrick R. Gansterer]

Created attachment 163107 [details]
Patch

[Benjamin Poulain]

Comment on attachment 163107 [details]
Patch

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

> Source/WebCore/rendering/RenderTreeAsText.cpp:90
> -String formatNumberRespectingIntegers(double value)
> +void writeNumberRespectingIntegers(TextStream& ts, float value)

The change double->float must be justified.

> Source/WebCore/rendering/RenderTreeAsText.cpp:121
> -    ts << "(" << formatNumberRespectingIntegers(p.x());
> -    ts << "," << formatNumberRespectingIntegers(p.y());
> +    ts << "(";
> +    writeNumberRespectingIntegers(ts, p.x());
> +    ts << ",";
> +    writeNumberRespectingIntegers(ts, p.y());
>      ts << ")";
>      return ts;

I haven't looked at this code in detail yet so that is just an early comment.

To make the code more readable, could't you add an operator<< to TextStream taking an wrapper returned by formatNumberRespectingIntegers? Something like this:

struct TextStreamFormatNumberRespectingIntegers { double value; }

TextStream::operator<<(TextStreamFormatNumberRespectingIntegers value) {
    if (hasFractions(value)) {
        ...
}

[Patrick R. Gansterer]

Created attachment 163387 [details]
Patch

[WebKit Review Bot]

Attachment 163387 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/text/TextStream.h:38:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 163390 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163390 [details]
Patch

Attachment 163390 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13818432

[WebKit Review Bot]

Comment on attachment 163390 [details]
Patch

Attachment 163390 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13806825

[Early Warning System Bot]

Comment on attachment 163390 [details]
Patch

Attachment 163390 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13806828

[Early Warning System Bot]

Comment on attachment 163390 [details]
Patch

Attachment 163390 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13826303

[Patrick R. Gansterer]

Created attachment 163394 [details]
Patch

[Benjamin Poulain]

Comment on attachment 163394 [details]
Patch

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

[Patrick R. Gansterer]

Created attachment 163832 [details]
Patch

[Patrick R. Gansterer]

Committed r128564: <http://trac.webkit.org/changeset/128564>