Bug 66851 - Fix CSSPrimitiveValue::cssText() to use StringBuilder
Summary: Fix CSSPrimitiveValue::cssText() to use StringBuilder
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-24 05:09 PDT by Oliver Varga
Modified: 2012-02-25 13:00 PST (History)
9 users (show)

See Also:


Attachments
Fix CSSPrimitiveValue::cssText() to use StringBuilder (14.34 KB, patch)
2011-08-24 05:15 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder (12.99 KB, patch)
2011-08-25 00:35 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder (12.88 KB, patch)
2011-08-25 01:35 PDT, Oliver Varga
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder (12.93 KB, patch)
2011-08-26 02:42 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder (18.00 KB, patch)
2011-09-14 10:08 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder (17.90 KB, patch)
2011-09-14 10:22 PDT, Oliver Varga
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder (22.54 KB, patch)
2011-10-14 09:28 PDT, Oliver Varga
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (21.94 KB, patch)
2011-10-17 06:37 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (21.31 KB, patch)
2011-10-18 07:11 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (21.35 KB, patch)
2011-10-19 00:40 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (21.35 KB, patch)
2011-10-19 00:42 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (21.35 KB, patch)
2011-10-21 05:32 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (21.26 KB, patch)
2011-10-21 08:29 PDT, Oliver Varga
kling: review-
Details | Formatted Diff | Diff
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload (20.87 KB, patch)
2011-10-23 14:19 PDT, Oliver Varga
tkent: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Correcting (22.59 KB, patch)
2011-11-03 06:20 PDT, Oliver Varga
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Correcting v2 (29.06 KB, patch)
2012-01-09 07:00 PST, Szilard Ledan
tonikitoo: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Varga 2011-08-24 05:09:34 PDT
Replace the string concatenation (+, +=, and Vector<Uchar> append()) to the much faster StringBuilder append().
http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg16022.html
Comment 1 Oliver Varga 2011-08-24 05:15:14 PDT
Created attachment 104982 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 2 Andreas Kling 2011-08-24 06:03:23 PDT
Comment on attachment 104982 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

> Source/WebCore/ChangeLog:14
> +        According to the tests youtube, ebay,
> +        baidu and wordpress loading is faster about 5-8%.

5-8%? Seriously? That sounds way too good to be true, how are you testing this exactly?
Comment 3 Darin Adler 2011-08-24 09:50:27 PDT
Comment on attachment 104982 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

Thanks for tackling this. I see no reason to overload the += operator, though. Lets use append.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:57
> +    inline void operator+=(const String& string)

Why is it critical to use += instead of an append function? Just to keep the editing down?

> Source/WebCore/css/CSSPrimitiveValue.cpp:646
> +            text += formatNumber(m_value.num);
> +            text += "em";

I think these would read better as append function calls.
Comment 4 Zoltan Herczeg 2011-08-24 23:33:31 PDT
I suggested Oliver to keep the += since for me it is easier to read. Sorry Oliver for the extra work.
Comment 5 Oliver Varga 2011-08-24 23:35:06 PDT
(In reply to comment #2)
> (From update of attachment 104982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104982&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        According to the tests youtube, ebay,
> > +        baidu and wordpress loading is faster about 5-8%.
> 
> 5-8%? Seriously? That sounds way too good to be true, how are you testing this exactly?

I tested our internal testing system, called "methanol benchmarking".
It is loading webpages many times and it is measured.
Comment 6 Oliver Varga 2011-08-25 00:35:26 PDT
Created attachment 105141 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 7 Oliver Varga 2011-08-25 01:23:25 PDT
(In reply to comment #2)
> (From update of attachment 104982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104982&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        According to the tests youtube, ebay,
> > +        baidu and wordpress loading is faster about 5-8%.
> 
> 5-8%? Seriously? That sounds way too good to be true, how are you testing this exactly?

You are right, the 5-8% is too good to be true.
I performed the tests again, and now generated a minimal performance tweak.
Comment 8 Oliver Varga 2011-08-25 01:35:12 PDT
Created attachment 105144 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 9 Darin Adler 2011-08-25 07:40:02 PDT
Comment on attachment 105144 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

My comments include many ideas that are ways to do better in the future. The one thing you should definitely fix before landing is the append(c) thing. Please consider the others too, though. We don’t want any changes that actually make things slower.

> Source/WebCore/css/CSSPrimitiveValue.cpp:638
> +            text.append(formatNumber(m_value.num));

Suggestion for further improvement:

There’s a significant performance gain available if we can come up with a way to do these without creating a string and then appending it. We need an equivalent of formatNumber that works directly with the StringBuilder, or some way that does not involve memory allocation other than what StringBuilder does.

> Source/WebCore/css/CSSPrimitiveValue.cpp:716
> -            text = quoteCSSStringIfNeeded(m_value.string);
> +            text.append(quoteCSSStringIfNeeded(m_value.string));

Similarly, for the quoted string case it's much more efficient to do the quoting without allocating an entire new string.

> Source/WebCore/css/CSSPrimitiveValue.cpp:732
>              DEFINE_STATIC_LOCAL(const String, attrParen, ("attr("));

There's no value in having this pre-allocated in a String. It can just be an append of a const char*.

> Source/WebCore/css/CSSPrimitiveValue.cpp:755
>              DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));

There's no value in having this pre-allocated in a String. It can just be an append of a const char*.

> Source/WebCore/css/CSSPrimitiveValue.cpp:-737
> -            Vector<UChar> result;
> -            result.reserveInitialCapacity(32);

Are you sure the StringBuilder is more efficient than the Vector<UChar> was?

> Source/WebCore/css/CSSPrimitiveValue.cpp:762
> +            result.append(rectVal->top()->cssText());

This is another opportunity to be much more efficient. If we had a function that appended the CSS text to a StringBuilder without ever creating a string, we could probably save many extra string allocations here.

> Source/WebCore/css/CSSPrimitiveValue.cpp:794
> -            appendNumber(result, static_cast<unsigned char>(color.red()));
> -            append(result, commaSpace);
> +            result.append(String::number(color.red()));

Again, making a string out of the number just to put it in the string builder is quite inefficient. It’s a step backwards to go from the appendNumber function, which can and has been optimized, to an implementation that explicitly allocates a string each time.

> Source/WebCore/css/CSSPrimitiveValue.cpp:853
>              char c = static_cast<char>(m_value.ident);
> -            text = String(&c, 1U);
> +            text.append(String(&c, 1U));

This is wrong. It can just be append(c) or even append(static_cast<char>(m_value.ident)). Explicitly creating a string makes us dow double allocation.

> Source/WebCore/css/CSSPrimitiveValue.cpp:867
> -    cssTextCache().set(this, text);
> +    cssTextCache().set(this, text.toString());
>      m_hasCachedCSSText = true;
> -    return text;
> +    return text.toString();

Calling toString() twice seems a bit inelegant. I would put the string into a local variable instead.
Comment 10 Oliver Varga 2011-08-26 02:41:48 PDT
(In reply to comment #9)
> (From update of attachment 105144 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105144&action=review
> 
> My comments include many ideas that are ways to do better in the future. The one thing you should definitely fix before landing is the append(c) thing. Please consider the others too, though. We don’t want any changes that actually make things slower.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:638
> > +            text.append(formatNumber(m_value.num));
> 
> Suggestion for further improvement:
> 
> There’s a significant performance gain available if we can come up with a way to do these without creating a string and then appending it. We need an equivalent of formatNumber that works directly with the StringBuilder, or some way that does not involve memory allocation other than what StringBuilder does.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:716
> > -            text = quoteCSSStringIfNeeded(m_value.string);
> > +            text.append(quoteCSSStringIfNeeded(m_value.string));
> 
> Similarly, for the quoted string case it's much more efficient to do the quoting without allocating an entire new string.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:732
> >              DEFINE_STATIC_LOCAL(const String, attrParen, ("attr("));
> 
> There's no value in having this pre-allocated in a String. It can just be an append of a const char*.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:755
> >              DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));
> 
> There's no value in having this pre-allocated in a String. It can just be an append of a const char*.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:-737
> > -            Vector<UChar> result;
> > -            result.reserveInitialCapacity(32);
> 
> Are you sure the StringBuilder is more efficient than the Vector<UChar> was?
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:762
> > +            result.append(rectVal->top()->cssText());
> 
> This is another opportunity to be much more efficient. If we had a function that appended the CSS text to a StringBuilder without ever creating a string, we could probably save many extra string allocations here.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:794
> > -            appendNumber(result, static_cast<unsigned char>(color.red()));
> > -            append(result, commaSpace);
> > +            result.append(String::number(color.red()));
> 
> Again, making a string out of the number just to put it in the string builder is quite inefficient. It’s a step backwards to go from the appendNumber function, which can and has been optimized, to an implementation that explicitly allocates a string each time.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:853
> >              char c = static_cast<char>(m_value.ident);
> > -            text = String(&c, 1U);
> > +            text.append(String(&c, 1U));
> 
> This is wrong. It can just be append(c) or even append(static_cast<char>(m_value.ident)). Explicitly creating a string makes us dow double allocation.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:867
> > -    cssTextCache().set(this, text);
> > +    cssTextCache().set(this, text.toString());
> >      m_hasCachedCSSText = true;
> > -    return text;
> > +    return text.toString();
> 
> Calling toString() twice seems a bit inelegant. I would put the string into a local variable instead.

Sorry for the inattention, now I'm correcting.
And thank you for the guidance. I will considered your suggestions.
Comment 11 Oliver Varga 2011-08-26 02:42:30 PDT
Created attachment 105339 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 12 Oliver Varga 2011-09-14 10:08:02 PDT
Created attachment 107345 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 13 WebKit Review Bot 2011-09-14 10:10:15 PDT
Attachment 107345 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/WebCore/css/CSSPrimitiveValue.cpp:894:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:108:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/text/StringBuilder.h:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/text/StringBuilder.h:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/text/StringBuilder.h:166:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 7 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Oliver Varga 2011-09-14 10:22:13 PDT
Created attachment 107347 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 15 WebKit Review Bot 2011-09-14 20:04:06 PDT
Comment on attachment 107347 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

New failing tests:
editing/execCommand/query-command-value-background-color.html
svg/animations/animate-color-rgba-calcMode-discrete.html
Comment 16 Darin Adler 2011-09-15 15:01:12 PDT
Comment on attachment 107347 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:105
> +        char* number = new char[absoluteExp + sizeOfsignificand + 3];

This is an inefficient way to allocate memory, and the wrong way to go for this function. The point of such functions is to be fast and doing a malloc makes them slow. An efficient way to allocate memory is:

    Vector<char, 32> number(absoluteExp + sizeOfsignificand + 3);

This will use the stack for the buffer unless the buffer has to be larger than 32 bytes.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:158
> +        char* number = new char[digitsOfNumber + 1];

Ditto.
Comment 17 Oliver Varga 2011-10-14 09:28:01 PDT
Created attachment 111022 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder
Comment 18 WebKit Review Bot 2011-10-14 09:50:09 PDT
Comment on attachment 111022 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

Attachment 111022 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10054573
Comment 19 Darin Adler 2011-10-14 10:31:23 PDT
Comment on attachment 111022 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:135
> +    void append(double d, int precision = 6)

This giant function should not be inlined in a header.
Comment 20 Oliver Varga 2011-10-17 06:37:10 PDT
Created attachment 111257 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload
Comment 21 WebKit Review Bot 2011-10-17 06:40:16 PDT
Attachment 111257 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   293b636..9b6f4d5  master     -> origin/master
	M	Tools/BuildSlaveSupport/build.webkit.org-config/config.json
	M	Tools/ChangeLog
r97614 = 9b6f4d5fe02791ab160cc89f3585e1ae198694bd (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒1﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒2﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒3﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Oliver Varga 2011-10-18 07:11:58 PDT
Created attachment 111441 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload
Comment 23 Oliver Varga 2011-10-19 00:40:34 PDT
Created attachment 111570 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload
Comment 24 WebKit Review Bot 2011-10-19 00:42:12 PDT
Attachment 111570 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/text/StringBuilder.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Oliver Varga 2011-10-19 00:42:38 PDT
Created attachment 111571 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload
Comment 26 Oliver Varga 2011-10-21 05:32:05 PDT
Created attachment 111955 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

I correct the previous mistake I found here: http://queues.webkit.org/results/10121269
After that I didn't found any information in connection with the Build Failed on win EWS.
I try to upload again the previous patch.
Comment 27 Darin Adler 2011-10-21 07:47:52 PDT
Comment on attachment 111955 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

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

If you want a patch to be reviewed, you need to set review+ on it.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:82
> +    static inline unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint)

The inline keyword here is not needed.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:108
> +    {
> +            // #shift
> +            int p = 1;
> +            while (num[p+1] != '\0' && p <= shift && num[1] != '\0') {
> +                    num[ p ] = num[p+1];
> +                    num[p+1] = '.';
> +                    p++;
> +            }

This function is indented 8 rather than 4 space tab stops.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:136
> +    void append(double d)

This large function should not be inlined in a header.
Comment 28 Oliver Varga 2011-10-21 08:29:29 PDT
Created attachment 111969 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

I corrected the indentation, and deleted the inline modifier.
Comment 29 Andreas Kling 2011-10-21 08:47:20 PDT
Comment on attachment 111969 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

You need to move the huge functions in StringBuilder.h to a cpp file to prevent them from being inlined at least once per compilation unit that calls them.
Comment 30 Oliver Varga 2011-10-23 14:19:55 PDT
Created attachment 112126 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

The huge functions moved to the StringBuilder.cpp
Comment 31 Kent Tamura 2011-10-23 21:41:36 PDT
Comment on attachment 112126 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

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

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:180
> +       length += -m_exponent - 1;
> +       length += m_precision;
> +       return length;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:184
> +       return 0;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:198
> +    while (num[p+1] != '\0' && p <= shift && num[1] != '\0') {
> +         num[ p ] = num[p+1];
> +         num[p+1] = '.';
> +         p++;

5 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:206
> +    while (p <= shift) {
> +         num[ p ] = '0';
> +         num[p+1] = '.';
> +         num[p+2] = 0;
> +         p++;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:212
> +    for (int i = length - 1; i >= p && (num[i] == '0' || num[i] == '.'); --i)
> +         num[i]=0;

5 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:216
> +    if (num[0] == '0' && num[1] == '0')
> +         return 2;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:219
> +    if (num[0] == '0' && num[1] != '.')
> +         return 1;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:222
> +    if (num[0] != '0')
> +       return 0;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:238
> +    if (d < 0) {
> +       isNegative = true;
> +       d = -d;
> +       StringBuilder::append("-", 1);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:276
> +    if (!precision) {
> +       if (m_exponent < 0) {
> +         m_significand[2] = m_significand[1];

3 space indentation,
and 2 space indentation.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:284
> +       for (; i < digitsBeforeDecimalPoint && i < sizeOfsignificand; ++i) {
> +         *pointer = m_significand[i];

2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:289
> +       if (m_significand[i] > '4') {
> +         toShif = true;

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:300
> +         if (m_significand[0] == '0') {
> +            maxLimit--;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:305
> +    } else
> +       jump = true;

3 space indentation.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:310
> +    if (jump) {
> +       for (int i = 0; i < numberOfZeros; i++) {
> +          if (i == 1 && !dotAlreadyWritten) {
> +             *pointer = dot;

3 space indentations

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:319
> +          if (maxLimit == precision) {
> +             if (numberOfZeros > precision + 1 || (numberOfZeros == (precision + 1) && m_significand[0] < '5')) {
> +               pointer = &number[1];

3 space indentation and 2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:324
> +               if (toShif)
> +                  from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:330
> +             if (numberOfZeros == precision + 1 && m_significand[0] > '4') {
> +               pointer[-1] = '1';

2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:334
> +               if (toShif)
> +                  from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:343
> +       for (int i = 0; i < sizeOfsignificand; i++) {
> +          if (i == placeOfDot + 1 && sizeOfsignificand != i && !dotAlreadyWritten) {
> +             *pointer = dot;

3 space indentations

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:351
> +          if (maxLimit == precision) {
> +             if (i + 1 < sizeOfMSignificand && m_significand[i + 1] < '5') {
> +               while (*pointer == '0' && dotAlreadyWritten)

3 space indentation,
and 2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:354
> +                if (*pointer == dot)
> +                   *pointer = '\0';

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:358
> +                if (toShif)
> +                   from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:365
> +              if (m_significand[i] == '9') {
> +                while (*pointer == '9' && *pointer != dot)
> +                  pointer--;

2 space indentations

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:368
> +                if (*pointer == dot)
> +                     pointer--;

5 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:370
> +                 pointer[1] = '\0';

Unnecessary 1 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:373
> +                if (toShif)
> +                   from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:383
> +              if (toShif)
> +                from = shifting(&number[0], valueOfShift);

2 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:393
> +    for (int i = 0; i < (m_exponent - sizeOfsignificand) + 1; i++) {
> +       *pointer = zero;
> +       pointer++;

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:399
> +    if (toShif)
> +       from = shifting(&number[0], valueOfShift);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:407
> +    if (n < 0) {
> +       StringBuilder::append("-", 1);

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:412
> +    if (!(n / 10)) {
> +       char c = static_cast<char>(n + '0');

ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:424
> +    do {
> +       *pointer = (n % 10) + '0';

3 space indentation

> Source/JavaScriptCore/wtf/text/StringBuilder.h:82
> +    static unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint);

Do not prepend 'm_' for argument names.
Comment 32 Darin Adler 2011-10-23 22:12:52 PDT
Comment on attachment 112126 [details]
Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:31
> +#include <math.h>
> +#include <wtf/MathExtras.h>
> +#include <wtf/dtoa.h>

No need to add these includes to the header. Please don’t.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:84
> +    static unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint);
> +
> +    static int shifting(char* num, int shift);

I don’t think these need to be public functions. They could be private to the .cpp file and use “static” to get external linkage. If we decide to make them public to share them with other code, we probably would not put them in the StringBuilder class. In fact, longer term functions that convert numbers to strings don’t necessarily belong in StringBuilder.

The names here are not normal WebKit function names. We do not use “get” for name of something that returns a value (see WebKit coding style guide), and the name seems too terse. It says “precision”, but precision of what? Needs a name that actually says what it does. And “shifting” is also a curious name for a function. Normally functions that return values are named what they return. If this returns a “shifting” then we have an unconventional use of English grammar here. Also, the argument’s name should not be “num”.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:88
> +    void append(double);
> +
> +    void append(int);

I’m not sure that overloading based on type is quite right here, since the format that is used is based on the type and the dependency on type might be subtle. We may want to give more specific names to these functions as we do to the conversion functions in String.h.

We should refactor this so it shares code with functions in String.h and StringImpl.h.
Comment 33 Oliver Varga 2011-11-03 06:20:48 PDT
Created attachment 113473 [details]
Correcting

I made the previous requests:
 * I corrected the indentation, and rename some variables and functions.
The next step, I will do shortly:
 * I am going to create a new cpp and header file what name is NumericFormatting.h/cpp.
 * It will includes low level numeric formatting functions, that allows higher level classes like String and StringBuilder to use them
   efficiently, so the StringBuilder::append(double) would call the NumericFormatting::doubleToAsciiUsedByDtoa.
Comment 34 Darin Adler 2011-11-03 09:53:36 PDT
(In reply to comment #33)
> NumericFormatting::doubleToAsciiUsedByDtoa.

I like the idea behind this patch but not this name.

For one thing, we spell it ASCII, not Ascii. But also "double to ASCII used by dtoa" is a confusing phrase and I think we can come up with a more straightforward name.
Comment 35 Darin Adler 2011-11-03 10:00:25 PDT
Comment on attachment 113473 [details]
Correcting

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

I appreciate your continuing to work hard on this patch; it’s worth doing. I think there is still some work needed.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:197
> +int StringBuilder::firstDigitOfOriginalNumber(char* numberToShifting, int measurementOfShift)

I don’t understand the meaning of the name “number to shifting".

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:202
> +        numberToShifting[ p ] = numberToShifting[p+1];

Should omit the spaces around the "p". Should include spaces around the "+".

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:218
> +        numberToShifting[i]=0;

Missing spaces here.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:141
> +    static unsigned precisionOfResult(int, unsigned, unsigned);
> +    static int firstDigitOfOriginalNumber(char*, int);

These should go into NumericFormatting.h/cpp, not this class.

I am puzzled. Why is it we need this new code and can’t use the DecimalNumber class that CSSPrimitiveValue’s formatNumber was using?

The argument names here need to be listed. It’s not at all clear what the arguments are.

The name “first digit of original number” does not make it clear that the function will have a side effect of changing the passed in character buffer.

> Source/WebCore/css/CSSPrimitiveValue.cpp:828
> +            text.append(result.toString());

We should make it so you can append one StringBuilder to another. This toString() seems like bad idea.

> Source/WebCore/css/CSSPrimitiveValue.cpp:851
> +            text.append(result.toString());

We should make it so you can append one StringBuilder to another. This toString() seems like bad idea.

> Source/WebCore/css/CSSPrimitiveValue.cpp:871
> +           text.append(result.toString());

We should make it so you can append one StringBuilder to another. This toString() seems like bad idea. Also, incorrect indent here.

> Source/WebCore/css/CSSPrimitiveValue.cpp:961
> +    String resultOfText = text.toString();

I think this could just be named “result”. The phrase “result of text” is not good grammar here. If you really wanted both words it would just be “result text” so, resultText.
Comment 36 Oliver Varga 2011-11-07 07:54:03 PST
Thank you for your comment, I am goint to check your new instructions shortly.
Comment 37 Szilard Ledan 2012-01-09 07:00:27 PST
Created attachment 121666 [details]
Correcting v2

Added a new class: NumericFormatting.h/cpp. The StringBuilder::append(double) uses it.

Modified CSSPrimitiveValue::customCssText() to use StringBuilder instead of string.
Comment 38 WebKit Review Bot 2012-01-09 07:32:47 PST
Comment on attachment 121666 [details]
Correcting v2

Attachment 121666 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11187504
Comment 39 Gyuyoung Kim 2012-01-09 07:39:23 PST
Comment on attachment 121666 [details]
Correcting v2

Attachment 121666 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11183501
Comment 40 Gustavo Noronha (kov) 2012-01-09 08:06:18 PST
Comment on attachment 121666 [details]
Correcting v2

Attachment 121666 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11187518
Comment 41 Darin Adler 2012-01-09 15:25:59 PST
Comment on attachment 121666 [details]
Correcting v2

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

I didn’t have time to review the whole thing, but I have a few initial comments.

> Source/JavaScriptCore/wtf/text/NumericFormatting.h:4
> + * Copyright (C) 2011 University of Szeged
> + * Copyright (C) 2011 Oliver Varga
> + * Copyright (C) 2012 Szilard Ledan

Seems like too much copyright for these two function declarations. Did all of these contribute to this?

> Source/JavaScriptCore/wtf/text/NumericFormatting.h:31
> +#include "string.h"

This include is not needed and should be removed, but if it was needed it would be <string.h>.

> Source/JavaScriptCore/wtf/text/NumericFormatting.h:44
> +class NumericFormatting {
> +public:
> +    // Is is used by append(double). Return the precision of the the result number.
> +    unsigned precisionOfResult(int exponent, unsigned precision, unsigned digitsBeforeDecimalPoint);
> +
> +    // Is is used by append(double). If the input number is like '999.999', and the value of the rounding is 0 we have to move forward the carry
> +    // over the decimal point. This kind of rounding implemented the number less then 1 and more than 0, and before the rounding we convert '999.999'
> +    // to '0.999999'. The firstDigitOfOriginalNumber() helps get the real number back.
> +    int firstDigitOfOriginalNumber(char* numberToShifting, int measurementOfShift);
> +};

These are functions, not a class. They could be free functions, or if you really want this to be a class, they should be static member functions, not member functions.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:34
>  #include "config.h"
>  #include "StringBuilder.h"
> +#include "NumericFormatting.h"
> +#include <wtf/MathExtras.h>
> +#include <wtf/dtoa.h>
>  
>  #include "WTFString.h"
>  
> +#include <stdio.h>

This is incorrect. All the includes except for "StringBuilder.h" should be in a single paragraph, and sorted.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:303
> +        StringBuilder::append("-", 1);

This should be:

    append('-');

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:317
> +    printf("debug: exponent:%d precision:%d\n", exponent, precision);

Please don't submit the patch with the printf still here.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:323
> +    int sizeOfsignificand = strlen(significand);

The word significand should be capitalized.

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:326
> +    int sizeOfMSignificand = sizeof(significand);

What is this? What does the “M” mean?
Comment 42 Antonio Gomes 2012-02-25 13:00:34 PST
Comment on attachment 121666 [details]
Correcting v2

based on Darin's review and lack of update.