[Webkit-unassigned] [Bug 18413] hixie test fails for CSS: sanity tests for relative keyword values of font-size

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26360|review?                     |review-
               Flag|                            |




------- Comment #9 from mitz at webkit.org  2009-01-06 11:48 PDT -------
(From update of attachment 26360)
Thanks for coming back to this.

> +    int i;

In WebKit C++ code, loop iterators are usually defined inside the for
statement. If the iterator is used outside the loop's scope, then it should be
defined just before the loop.

> +    const int *currentFontSizeTable;

The * should go next to the type.

> +        currentFontSizeTable = const_cast<int *> (customFontSizeTable);

There should not be a space before the *. There also shouldn't be a space
before the left parenthesis.

> +    // If outside the scope of the table, return approximate value.
> +    if (size >= currentFontSizeTable[totalKeywords - 1])
> +        return increaseFontSize ? size * (static_cast<float> (currentFontSizeTable[totalKeywords - 1]) / static_cast<float> (currentFontSizeTable[totalKeywords - 2]))
> +                                : size / (static_cast<float> (currentFontSizeTable[totalKeywords - 1]) / static_cast<float> (currentFontSizeTable[totalKeywords - 2]));
> +    
> +    if (size < currentFontSizeTable[0] || (size == currentFontSizeTable[0] && !increaseFontSize))
> +        return size + static_cast<float> (modifier);

Why is adding or subtracting 1 the correct behavior for sizes smaller than the
smallest size in the table? Wouldn't a multiplicative scale factor make more
sense? The asymmetry with the other side is odd.

> +    int currentMatch = 0;
> +    float deviation = static_cast<float> (INT_MAX);

There should not be a space before the left parenthesis.

> +    // find which two values from the table the size falls between
> +    for (i = 0; i < totalKeywords - 1; i++) {
> +        if (size - currentFontSizeTable[i] < currentFontSizeTable[i + 1] - currentFontSizeTable[i] && size >= currentFontSizeTable[i]) {

No need to subtract currentFontSizeTable[i] from both sides of the inequality.

> +    float newDeviation;
> +    float position = deviation / static_cast<float> (currentFontSizeTable[currentMatch + 1] - currentFontSizeTable[currentMatch]);

No need to cast because deviation is a float.

> +    if (increaseFontSize) {
> +        if (currentMatch == totalKeywords - 2)
> +            newDeviation = position * ((static_cast<float> (currentFontSizeTable[currentMatch+1]) * static_cast<float> (currentFontSizeTable[currentMatch+1]) / static_cast<float> (currentFontSizeTable[currentMatch]))
> +                                            - static_cast<float> (currentFontSizeTable[currentMatch + 1]));

Not all of the casts are necessary. There should be spaces around the +
operator and no spaces between > and (.

> +        else
> +            newDeviation = position * static_cast<float> (currentFontSizeTable[currentMatch + 2] - currentFontSizeTable[currentMatch + 1]);
> +    } else
> +        newDeviation = position * static_cast<float> (currentFontSizeTable[currentMatch] - currentFontSizeTable[currentMatch - 1]);

How do you know that currentMatch is greater than 0?

> +    if (roundf(newDeviation) == 0) {
> +        fontDescription.setKeywordSize(currentMatch + modifier + 1);
> +        return currentFontSizeTable[currentMatch + modifier];
> +    }
> +    
> +    return currentFontSizeTable[currentMatch + modifier] + newDeviation;

Same question.


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