Bug 48886 – Avoid CFAttributedString creation in ComplexTextController

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Ned Holbrook]

Adopt UniChar provider SPI for ~20% speedup.

[Ned Holbrook]

Created attachment 72781 [details]
proposed patch

Comment on attachment 72781 [details]
proposed patch

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

[Ned Holbrook]

Created attachment 72790 [details]
changes per review.

[Adam Barth]

Comment on attachment 72790 [details]
changes per review.

I'm not an expert here, but this looks reasonable to me and you seem to have addressed the earlier review comments.  Thanks.

[WebKit Commit Bot]

Comment on attachment 72790 [details]
changes per review.

Clearing flags on attachment: 72790

Committed r71590: <http://trac.webkit.org/changeset/71590>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This broke Chromium Mac builds:
/b/build/slave/webkit-rel-mac-webkit-org/build/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/mac/ComplexTextControllerCoreText.cpp: In function 'const UniChar* WebCore::provideStringAndAttributes(CFIndex, CFIndex*, const __CFDictionary**, void*)':
/b/build/slave/webkit-rel-mac-webkit-org/build/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/mac/ComplexTextControllerCoreText.cpp:114: warning: comparison between signed and unsigned integer expressions

I fixed the warning in:
http://trac.webkit.org/changeset/71598

Please verify my fix.

[Ned Holbrook]

(In reply to comment #7)
> Please verify my fix.

Looks fine to me, thanks!