Bug 48886 - Avoid CFAttributedString creation in ComplexTextController
Summary: Avoid CFAttributedString creation in ComplexTextController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-02 16:31 PDT by Ned Holbrook
Modified: 2010-11-08 20:55 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (8.34 KB, patch)
2010-11-02 18:25 PDT, Ned Holbrook
mitz: review-
Details | Formatted Diff | Diff
changes per review. (8.61 KB, patch)
2010-11-02 22:57 PDT, Ned Holbrook
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ned Holbrook 2010-11-02 16:31:48 PDT
Adopt UniChar provider SPI for ~20% speedup.
Comment 1 Ned Holbrook 2010-11-02 18:25:55 PDT
Created attachment 72781 [details]
proposed patch
Comment 2 mitz 2010-11-02 21:15:24 PDT
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
Comment 3 Ned Holbrook 2010-11-02 22:57:39 PDT
Created attachment 72790 [details]
changes per review.
Comment 4 Adam Barth 2010-11-04 15:46:37 PDT
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.
Comment 5 WebKit Commit Bot 2010-11-08 17:54:13 PST
Comment on attachment 72790 [details]
changes per review.

Clearing flags on attachment: 72790

Committed r71590: <http://trac.webkit.org/changeset/71590>
Comment 6 WebKit Commit Bot 2010-11-08 17:54:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Ryosuke Niwa 2010-11-08 19:44:29 PST
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.
Comment 8 Ned Holbrook 2010-11-08 20:55:14 PST
(In reply to comment #7)
> Please verify my fix.

Looks fine to me, thanks!