[webkit-reviews] review denied: [Bug 66851] Fix CSSPrimitiveValue::cssText() to use StringBuilder : [Attachment 113473] Correcting

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 113473: Correcting
https://bugs.webkit.org/attachment.cgi?id=113473&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.