[webkit-reviews] review denied: [Bug 6484] font-weight does not properly support graded weights : [Attachment 12829] nickshanks' work-in-progress patch

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6484: font-weight does not properly support graded weights
http://bugs.webkit.org/show_bug.cgi?id=6484

Attachment 12829: nickshanks' work-in-progress patch
http://bugs.webkit.org/attachment.cgi?id=12829&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this. The most important issue is testing and not breaking
websites. This slightly more advanced capability is not nearly as important as
working properly with existing websites.

Here's a quick pass of review:

+#if PLATFORM(MAC)
+		     case CSS_VAL_BOLDER:
+			
fontDescription.setWeight(fontDescription.bolderWeight());
+			 break;

Why is this code in #if PLATFORM_MAC? I think the platform-specific code should
lower level.

+    case CSS_PROP_FONT_STRETCH:
+	{

Brace should be on same line with property name.

+			switch (primitiveValue->getIdent()) {

Incorrect indenting here using tabs. Need to indent with spaces. I saw tabs
elsewhere.

+    FontPlatformDataCacheKey key(familyName,
fontDescription.computedPixelSize(), fontDescription.weight(), 
fontDescription.stretch(), fontDescription.italic());

Extra space here.

+#if PLATFORM(MAC)
+const unsigned cUltraLightWeight = 1;	// CSS 100

This is not the way this should work. The whole idea of these classes is to
make a cross-platform abstraction. Instead, the platform-specific code should
go a bit lower level.

+    // FIXME: Does not necessarilly return the immediate next weight up or
down.
+    //  The font cache will return the lowest weight if the two closest
available weights are equidistant from the
+    // requested weight, potentially the same weight as was used before. Using
+3/-2 decreases the liklyhood of that.
+    unsigned bolderWeight() const { return (m_weight +3 < cUltraBoldWeight)?
m_weight +3 : cUltraBoldWeight; }
+    unsigned lighterWeight() const { return (m_weight -2 > cUltraLightWeight)?
m_weight -2 : cUltraLightWeight; }

This code does not belong in a header. Please don't make it all inlined like
this. Also some typos here.

+	 unsigned actualWeight = (unsigned) [fontManager
weightOfFont:substituteFont];

Please use static_cast, not C-style cast.

+					(actualWeight +3) <
font.fontDescription().weight(),

We use spaces before and after operators like "+".

This code needs a comment. The magic number +3 makes no sense.

+    desiredTraits &= ~NSBoldFontMask;	// NOTE: you shouldn't be using this
trait anymore; make sure you don't pass it in

Who is "you" here? I think this should just be an assertion. This is not
general purpose code, so it doens't have to be passed in.

You made a change to the small caps rule. We need a test case that covers that.


Does this affect any layout test results? Which ones?

+    // FIXME: cannot represent weights accuratly
+    // FIXME: cannot represent stretches accuratly

How can these be fixed? FIXME is supposed to be for bugs that can be fixed.

We'll need a test case that uses default-installed fonts if possible. We may
need to rig a test mode where some fonts are intentionally installed so we can
regression-test this properly.