[webkit-reviews] review granted: [Bug 215688] [Cocoa] CTFontHasTable() is faster than iterating over every table in the font : [Attachment 408077] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 215688: [Cocoa] CTFontHasTable() is faster than iterating over every table
in the font
https://bugs.webkit.org/show_bug.cgi?id=215688

Attachment 408077: Patch

https://bugs.webkit.org/attachment.cgi?id=408077&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 408077
  --> https://bugs.webkit.org/attachment.cgi?id=408077
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408077&action=review

> Source/WTF/wtf/PlatformUse.h:309
> +#define USE_CTFONTHASTABLE 1

Is this really a USE and not a HAVE?

USE for policy. HAVE for capability. I assume we’d use this whenever we have a
working implementation.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:65
>  static bool fontHasVerticalGlyphs(CTFontRef ctFont)

Seems like the argument can just be named font.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:71
>      RetainPtr<CFArrayRef> tableTags =
adoptCF(CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions));

auto

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:76
> +	   CTFontTableTag tag =
static_cast<CTFontTableTag>(reinterpret_cast<uintptr_t>(CFArrayGetValueAtIndex(
tableTags.get(), index)));

auto

Is the static_cast really needed? I would think that conversion between integer
types would work without it. In fact we could even compare uintptr_t values
with CTFontTableTag constants without having to make the types match. I think
using auto and taking out the static_cast would be good.

> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:42
>  bool fontHasMathTable(CTFontRef ctFont)

Could this share code with the other some day?

A function that takes a CTFontRef and an argument list of tables and checks for
any of them? Would be nice to not have this twice.

> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:47
>      RetainPtr<CFArrayRef> tableTags =
adoptCF(CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions));

Ditto.

> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:52
> +	   CTFontTableTag tag =
static_cast<CTFontTableTag>(reinterpret_cast<uintptr_t>(CFArrayGetValueAtIndex(
tableTags.get(), index)));

Ditto.