Summary: | Avoid CFAttributedString creation in ComplexTextController | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ned Holbrook <ned> | ||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, ned, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.6 | ||||||||
Attachments: |
|
Description
Ned Holbrook
2010-11-02 16:31:48 PDT
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 Created attachment 72790 [details]
changes per review.
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 on attachment 72790 [details] changes per review. Clearing flags on attachment: 72790 Committed r71590: <http://trac.webkit.org/changeset/71590> All reviewed patches have been landed. Closing bug. 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. (In reply to comment #7) > Please verify my fix. Looks fine to me, thanks! |