[webkit-reviews] review granted: [Bug 207374] [Cocoa] Slightly improve performance of Font::getCFStringAttributes() : [Attachment 390048] 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 207374: [Cocoa] Slightly improve performance of
Font::getCFStringAttributes()
https://bugs.webkit.org/show_bug.cgi?id=207374

Attachment 390048: Patch

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




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

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

This looks good. Like I said earlier, if this is *really* hot then we should go
even further with some kind of caching/memoizing so we don’t create a
dictionary at all in most cases.

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:38
> +    CFTypeRef keys[5];
> +    CFTypeRef values[5];

Easy mistake would be to add a new case below and not update the array sizes
here. Would be nice if we could find a way to assert or static_assert or
something to make it trivial to catch the mistake either at compile time or
runtime.

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:59
> +    static CTParagraphStyleRef paragraphStyle = nullptr;
> +    if (!paragraphStyle) {
> +	   paragraphStyle = CTParagraphStyleCreate(nullptr, 0);
> +	   CTParagraphStyleSetCompositionLanguage(paragraphStyle,
kCTCompositionLanguageNone);
> +    }

This can be written something like this:

    static CTParagraphStyleRef paragraphStyle = [] () {
	auto style = CTParagraphStyleCreate(nullptr, 0);
	CGParagraphStyleSetCompositionLanguage(style,
kCTCompositionLanguageNone);
	return style;
     }();

I think it’s slightly better coding style. Could even put this into a separate
function for clarity.