[webkit-reviews] review denied: [Bug 5166] Remaining cases where the ATSUI and CG code paths measure and render the same text differently : [Attachment 4247] fix more ATSUI/CG differences

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied opendarwin.org at mitzpettel.com's
request for review:
Bug 5166: Remaining cases where the ATSUI and CG code paths measure and render
the same text differently
http://bugzilla.opendarwin.org/show_bug.cgi?id=5166

Attachment 4247: fix more ATSUI/CG differences
http://bugzilla.opendarwin.org/attachment.cgi?id=4247&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I think that turning off kerning and ligatures in the ATSU path is actually the
wrong direction -- in the long run I'd like to support kerning and ligatures in
our CG code path, but still keep the code path very fast. That doesn't mean I
this patch is unacceptable, but I would like to go the other direction as much
as possible in the future.

In disposeATSULayoutParameters, there's no need for the if before free; free is
defined to do nothing for 0.

Generally we only allocate with calloc when we need the "initialized with 0"
behvior, otherwise, we don't want to spend the extra time to zero the entire
buffer. So perhaps some of those calloc calls should just be malloc with
multiplication.

There is one place with two spaces after an =, a call to ATSUMatchFontsToText.
Should remove the extra space.

An question unrelated to this patch: can we use ATSUMatchFontsToText in
substituteFontForString in the future? I think ATSU's subsitute fonts might be
better than the ones we get from AppKit calls, and perhaps could wean us from
AppKit SPI.

I'm concerned about the performance impliciations of checking more characters
in the calls to shouldUseATSU. I understand the check is necessary, but we'll
need to test that this doesn't slow down key benchmarks.

Also, perhaps the rule of checking from the beginning of the run could be
inside shouldUseATSU instead of having to repeat it in two places.

Perhaps the attribute arrays should have size declared explicitly as 4; I think
that makes it slighly less likely the sizes will get ouf of sync with the size
parameter to ATSUSetAttributes.

I'm concerned that the heuristic about when to turn off ligatures is not good.
If there's some great Arabic font that also happens to include Roman characters
in it (perhaps it's a universal font that covers many characters), then I still
want the ligatures to work properly. We should probably contemplate another
heuristic.

Perhaps for a later patch, not this one: As long as we're copying the buffer
for the small caps case, we should also copy the buffer for runs that have
'\n', '\t', NBSP, or other control characters in them. Then '\n', '\t', and
NBSP can just be replaced with spaces; for other control characters perhaps we
can use zero-width spaces as the CG code path does. Or we could adjust offsets
and lengths and remove those other control characters.

Why does this patch omit the "render bold twice" part? It seems we could do
that relatively easily by calling ATSUDrawText twice; do we really need to
stage this in two separate patches?

I think these things should be done separately to make it easier to catch
performance regressions. So ideally one patch should add the multiple renderers
and small caps. A separate one could add synthetic oblique and bold. Still
another could turn off kerning and ligatures. And a final one could turn on the
more-correct check for when to use ATSU. This will make it much easier for the
people who test performance to notice effects of the individual changes.

I'd like to be more clear on which of these suggestions are mandatory and which
are optional, but my thinking on this is not so clear at the moment; instead I
want to get my feedback to you ASAP.