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

[bugzilla-daemon at webkit.org]

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





--- Comment #41 from Darin Adler <darin at apple.com>  2012-01-09 15:25:59 PST ---
(From update of attachment 121666)
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?

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