Summary: | Reuse floating point formatting of TextStream in RenderTreeAsText.cpp | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick R. Gansterer <paroga> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Patrick R. Gansterer <paroga> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, dglazkov, eric, gustavo, peter+ews, philn, webkit.review.bot, xan.lopez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 96330 | ||||||||||||||||
Attachments: |
|
Description
Patrick R. Gansterer
2012-09-10 05:26:09 PDT
Created attachment 163100 [details]
Patch
Comment on attachment 163100 [details] Patch Attachment 163100 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13796870 Comment on attachment 163100 [details] Patch Attachment 163100 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13796869 Comment on attachment 163100 [details] Patch Attachment 163100 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13800803 Comment on attachment 163100 [details] Patch Attachment 163100 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13798785 Created attachment 163107 [details]
Patch
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)) { ... } Created attachment 163387 [details]
Patch
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.
Created attachment 163390 [details]
Patch
Comment on attachment 163390 [details] Patch Attachment 163390 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13818432 Comment on attachment 163390 [details] Patch Attachment 163390 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13806825 Comment on attachment 163390 [details] Patch Attachment 163390 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13806828 Comment on attachment 163390 [details] Patch Attachment 163390 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13826303 Created attachment 163394 [details]
Patch
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> ? Created attachment 163832 [details]
Patch
Committed r128564: <http://trac.webkit.org/changeset/128564> |