[webkit-reviews] review denied: [Bug 66851] Fix CSSPrimitiveValue::cssText() to use StringBuilder : [Attachment 112126] Fix CSSPrimitiveValue::cssText() to use StringBuilder and add two StringBuilder append overload

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Oliver Varga
<voliver at inf.u-szeged.hu>'s request for review:
Bug 66851: Fix CSSPrimitiveValue::cssText() to use StringBuilder
https://bugs.webkit.org/show_bug.cgi?id=66851

Attachment 112126: Fix CSSPrimitiveValue::cssText() to use StringBuilder and
add two StringBuilder append overload
https://bugs.webkit.org/attachment.cgi?id=112126&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.