[webkit-reviews] review denied: [Bug 48886] Avoid CFAttributedString creation in ComplexTextController : [Attachment 72781] proposed patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied nholbrook at apple.com's request for review:
Bug 48886: Avoid CFAttributedString creation in ComplexTextController
https://bugs.webkit.org/show_bug.cgi?id=48886

Attachment 72781: proposed patch
https://bugs.webkit.org/attachment.cgi?id=72781&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=72781&action=review

r- because this will break the Snow Leopard build. Also, a couple of minor
comments.

> WebCore/WebCore.exp.in:1161
> +_wkCreateCTTypesetterWithUniCharProviderAndOptions

This will break the Leopard and Snow Leopard builds, because this symbols is
not defined on those platforms. You should make a new #if
!defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD) section
further down and put this export in that section. Then the export file
generator will omit it from the export list for those platforms.

> WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:119
> +    return &info->cp[stringIndex];

I would write this as info->cp + stringIndex but either way is fine.

> WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:145
> +	   ProviderInfo info = {cp, length,
fontData->getCFStringAttributes(m_font.typesettingFeatures())};

WebKit style is to put spaces inside the braces (see the initialization of
rtlOptionValues[] above). Another thing you could do is define a constructor
for ProviderInfo.

> WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:155
> +	   ProviderInfo info = {cp, length,
fontData->getCFStringAttributes(m_font.typesettingFeatures())};

Ditto