[webkit-reviews] review denied: [Bug 18413] hixie test fails for CSS: sanity tests for relative keyword values of font-size : [Attachment 26360] patch3

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied mitz at webkit.org's request for review:
Bug 18413: hixie test fails for CSS: sanity tests for relative keyword values
of font-size
https://bugs.webkit.org/show_bug.cgi?id=18413

Attachment 26360: patch3
https://bugs.webkit.org/attachment.cgi?id=26360&action=review

------- Additional Comments from mitz at webkit.org
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.