[webkit-reviews] review denied: [Bug 4355] use small-caps variant font if it's available : [Attachment 3322] small-caps patch 2.1.1

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Nicholas Shanks
<contact at nickshanks.com>'s request for review:
Bug 4355: use small-caps variant font if it's available
http://bugzilla.opendarwin.org/show_bug.cgi?id=4355

Attachment 3322: small-caps patch 2.1.1
http://bugzilla.opendarwin.org/attachment.cgi?id=3322&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looking good!

Here are the outstanding issues I see:

1) Still a few cases of "if(" instead of "if (" in this patch.

2) Using the ATSU code path is going to have a number of undesirable effects,
including incorrect handling of "\n" characters -- so we might not want to do
this until we fix those. I think Justin Garcia from WebKit team was just
looking at that recently.

3) I still don't see this handling cases where the small caps variant doesn't
cover all the characters needed. Can this result in a mix of two different
fonts on a page? Other browsers don't do this, so this might cause us to look
broken on pages they look fine on. Is there a reason I should not be worried
about this?

4) This:

+	 // will choke on surrogate pairs?
+//	 if(!u_isUUppercase(run->characters[i]))

is not a practical issue. I believe the function does the right thing with half
of a surrogate pair (nothing). Also, why land the u_isUUppercase check
commented out? Lets just omit it. I don't like adding commented-out code.

6) I don't understand why this patch both triggers the ATSU code path for all
"true small caps" cases, but also still adds true small caps support to the CG
path. We don't need to do both.

7) A simpler way to do this:

+    if(style->smallCaps && simulatedSmallCaps)
+	 free((void *)capsRun.characters);

is to initialize capsRun.characters to NULL and then just leave out the if.

8) Misspelled separate in a comment "variant as seperate font".

9) Please omit the uneeded cast to (void *) here:

+		 free((void *)selectors);

10) Why does small caps also set old-style numbers? Need a rationale in the
code at least so later contributors will know why we should keep this around.
And I'd like to see the test case include a test of that unless it's
impossible.