Bug 136327 – Initialization for some member variable of FontPlatformData

• Component: Text
• Status: RESOLVED FIXED

There are initialization missing on FontPlatformData::FontPlatformData(const FontPlatformData& other, float size).

This function is located on FontPlatformDataFreeType.cpp.

I think m_fallbacks and m_scaledFont should be initialized on this creator.

Because they are used as condition on FontPlatformData::operator= and FontPlatformData() is copying other to itself.

Created attachment 237278 [details]
Patch

[Daniel Bates]

Comment on attachment 237278 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:229
>      *this = other;

I know you didn't write this, but this isn't a good idiom. That is, calling the assignment operator from inside a copy constructor isn't a good idiom because the former only applies to a fully initialized object and the latter initializes a new object. Instead we should extract the common code into a member function that is called from both the copy assignment and copy constructor. Notice that we also call the copy assignment operator from the default copy constructor: <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp?rev=167768#L216>.

You mean new function is needed for copying member variables like copyFontPlatformData() or something?
But I think this patch is also needed in the case of using same copy function on assignment operator and copy constructor.
Should we use different code between assignment op. and copy constructor??

You mean new function is needed for copying member variables like copyFontPlatformData() or something?
But I think this patch is also needed in the case of using same copy function on assignment operator and copy constructor.
Should we use different code between assignment op. and copy constructor??

I think idiom issue should be handled on another issue. Because, even though we add copy function for FontPlatformData, this patch is still needed.

[Myles C. Maxfield]

Comment on attachment 237278 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:226
> +    : m_fallbacks(0)
> +    , m_scaledFont(0)

Please use nullptr.

[Myles C. Maxfield]

Comment on attachment 237278 [details]
Patch

Please file an issue for refactoring common code between the copy constructor and copy assignment operator.

Created attachment 245052 [details]
Patch

I have filed an issue for idiom.
https://bugs.webkit.org/show_bug.cgi?id=140718

Created attachment 245108 [details]
Patch for landing

[Gyuyoung Kim]

Comment on attachment 245108 [details]
Patch for landing

LGTM. cq=me

[WebKit Commit Bot]

Comment on attachment 245108 [details]
Patch for landing

Clearing flags on attachment: 245108

Committed r179006: <http://trac.webkit.org/changeset/179006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.