[Webkit-unassigned] [Bug 66851] Fix CSSPrimitiveValue::cssText() to use StringBuilder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66851


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113473|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #35 from Darin Adler <darin at apple.com>  2011-11-03 10:00:25 PST ---
(From update of attachment 113473)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.