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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 8 22:41:56 PDT 2008


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





------- Comment #6 from mitz at webkit.org  2008-05-08 22:41 PDT -------
(From update of attachment 20996)
+    Settings* settings = m_document->settings();
+    if (!settings)
+        return fallbackSize;

I think you should not define fallbackSize and assign to it before checking
settings, because you only use the 1.2 factor in this early return case.

+    const int (*currentFontSizeTable);

Why the parentheses?

+    // If medium size is not within existing tables, then create new table
using fontSizeFactors,
+    // otherwise just point to the correct row of an existing table.
+    if (!(mediumSize >= fontSizeTableMin && mediumSize <= fontSizeTableMax)) {

Please reverse the order so that you check for the non-negated (and perhaps
common) condition first.

+        for (i = 0; i < totalKeywords; i++) {
+            customFontSizeTable[i] = mediumSize * fontSizeFactors[i];
+        }

The WebKit coding style is that single-line blocks do not have braces around
them.

+        currentFontSizeTable = (const int *) customFontSizeTable;

I think you don't need this cast.

Setting this to point to a variable that goes out of scope by the time you
dereference the variable seems fragile (I am not a C++ expert so I do not know
whether it is in fact safe). I would define customFontSizeTable outside the if
statement. But do you really want to define this table and populate it when you
may end up not even comparing with all the entries in it? I guess it allows you
handle the "table found" and "no table found" cases uniformly.

+    float deviation = (float) INT_MAX;

Please use static_cast.

+    // If outside the scope of the table, return approximate value.
+    if ((size > currentFontSizeTable[totalKeywords - 1] && increaseFontSize)
|| (size < currentFontSizeTable[0] && !increaseFontSize))
+        return fallbackSize;

Why not >= and <=, respectively?

+    for (i = 0; i < totalKeywords; i++) {
+        if (fabsf(size - currentFontSizeTable[i]) <= fabsf(deviation)) {
+            currentMatch = i;
+            deviation = size - currentFontSizeTable[i];
+            if (roundf(deviation) == 0)
+                break;
+        }
+    }

Seems to me like you are not really interested in which table value you are
closest to, but rather which two table values you lie between.

+    int modifier = increaseFontSize ? 1 : -1;

You can move this definition further down.

+    // This is set-up to match the Mozilla behavior. They determine what the
relative position
+    // of the font-size is between fontSizeA and fontSizeB and then put it in
the same relative
+    // position only between fontSizeB and fontSizeC.

Kind of weird that it's using arithmetic rather than geometric (weighted) means
given that this is essentially multiplicative.

+    float distance, position, newDeviation;
+    int fontSizeA, fontSizeB, fontSizeC;

Please declare each variable in a separate statement.

+    if ((deviation > 0 && increaseFontSize) || (deviation < 0 &&
!increaseFontSize)) {
+        fontSizeA = currentFontSizeTable[currentMatch];
+        fontSizeB = currentFontSizeTable[currentMatch + modifier];
+        if (currentMatch == totalKeywords - 2 && increaseFontSize)
+            fontSizeC = fontSizeB * 1.5f;
+        else if (currentMatch == 1 && !increaseFontSize)
+            fontSizeC = fontSizeB / 1.1f;
+        else
+            fontSizeC = currentFontSizeTable[currentMatch + modifier * 2];
+    } else {
+        fontSizeB = currentFontSizeTable[currentMatch];
+        fontSizeC = currentFontSizeTable[currentMatch + modifier];
+        if (currentMatch == totalKeywords - 1)
+            fontSizeA = fontSizeB * 1.5f;
+        else if (currentMatch == 0) 
+            fontSizeA = fontSizeB / 1.1f;
+        else
+            fontSizeA = currentFontSizeTable[currentMatch - modifier];
+    }

I think this can be simplified along with changing the loop that finds
"currentMatch".

We talked about behavior with sizes that lie outside the table and you said
you'd fix the bug I pointed out to you. I think it might be better to stick to
1.5 above and 1.1 below if that's what Firefox does.


-- 
Configure bugmail: http://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.



More information about the webkit-unassigned mailing list