Post-review comments on r188146

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Post-review comments on r188146

[Myles C. Maxfield]

Created attachment 258526 [details]
Patch

[Daniel Bates]

Comment on attachment 258526 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:55
> +        return originalFont;

Take originalFont to be non-null and features to be null. Then this will cause a memory leak because on return from this function the originalFont will have a retain count of 2. Assuming you like the design of having a non void return type for this function, one way to prevent this is to change the data type of originalFont from CTFontRef to RetainPtr<CTFontRef>.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:60
> +        appendTrueTypeFeature(featureArray.get(), (*features)[i]);
> +        appendOpenTypeFeature(featureArray.get(), (*features)[i]);

Is there a better way to write (*features)[i]? One idea is to rename the argument features to featuresPtr and then define a local variable, features, that is constant lvalue reference to FontFeatureSettings after line 55. Then write the remaining code for this function in terms of features.

[Daniel Bates]

(In reply to comment #2)
> Comment on attachment 258526 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258526&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:55
> > +        return originalFont;
> 
> [...] Then this will
> cause a memory leak because on return from this function the originalFont
> will have a retain count of 2. 

Disregard this remark. It will not leak. Though it will ref-count churn :/

[Myles C. Maxfield]

Committed r188168: <http://trac.webkit.org/changeset/188168>