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

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Anatoli Papirovski <apapirovski at mac.com>'s request
for review:
Bug 18413: hixie test fails for CSS: sanity tests for relative keyword values
of font-size
http://bugs.webkit.org/show_bug.cgi?id=18413

Attachment 20711: patch
http://bugs.webkit.org/attachment.cgi?id=20711&action=edit

------- Additional Comments from mitz at webkit.org
This patch is on the right track, but has several issues:

+	 Implement larger and smaller keywords for font-size, bug 18413

It is convenient to have the bugs.webkit.org URL in the change log. You should
also describe the changes.

-    if (childFont.keywordSize()) {
+     if (childFont.keywordSize()) {

Oops, extra space there.

+    float fallbackSize = size * 1.2f;
+    int modifier = 1;
+    
+    if (!increaseFontSize) {
+	 fallbackSize = size / 1.2f;
+	 modifier = -1;
+    }

Initializing the variables to one value only to reassign a different value if
increaseFontSize is false is somewhat inelegant. Even though it might be one or
two more lines of code, I think this would be better:

    float fallbackSize; = size * 1.2f;
    int modifier; = 1;

    if (increaseFontSize) {
	fallbackSize = size * 1.2f;
	modifier = 1;
    }else {
	fallbackSize = size / 1.2f;
	modifier = -1;
    }

Or maybe just
   float fallbackSize = increaseFontSize ? size * 1.2f : size / 1.2f;

And do the same for the modifier variable right before you use it (right now
you're defining and initializing it even if it's not used).

+    if (mediumSize >= fontSizeTableMin && mediumSize <= fontSizeTableMax) {

You could check for the opposite condition and return early, and then the rest
of the could wouldn't need to be indented.

The definition

+    const int (*currentFontSizeTable)[fontSizeTableMax - fontSizeTableMin + 1]
= quirksMode ? quirksFontSizeTable : strictFontSizeTable;

could be moved after checking that the medium size falls inside the table sizes
range.

+	 //if outside the scope of the table, return approximate value

Please add a space after the //, capitalize the If and end with a period.

+	 if (size > currentFontSizeTable[row][totalKeywords-1]) return
fallbackSize;

Missing spaces around the minus sign. The return statement  should be on a
separate line.

But more importantly, why is the check one-sided? What about sizes smaller than
the smallest size in the table?

+    long double deviation = (float) INT_MAX;

I don't think you need a long double for that.

+		 if (deviation == 0) break;

The break statement should go on a separate line.

+	 if (currentMatch == totalKeywords - 1) 
+	     return fallbackSize;

Do we really want to use the fallback here even if we are going to decrease the
font size? The given size is between the last two entries on the table, closer
to the last one (it cannot be past the last one because of the "if outside the
scope" check), so why not snap to the closest value?

+	 fontDescription.setKeywordSize(currentMatch+modifier+1);

Missing spaces around the plus signs.

On a higher level, I think it is somewhat confusing  that this function takes a
FontDescription and modifies its keyword size, but the numeric size is returned
from the function and fed back into the FontDescription by the caller. It would
be really great if you could rework it so that either everything happens inside
the function or it just takes a reference to a keyword size and sets it.