[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 #112126|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #32 from Darin Adler <darin at apple.com>  2011-10-23 22:12:52 PST ---
(From update of attachment 112126)
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.

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