Bug 136327 - Initialization for some member variable of FontPlatformData
Summary: Initialization for some member variable of FontPlatformData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-27 18:46 PDT by byeongha.cho
Modified: 2015-01-23 02:33 PST (History)
2 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2014-08-27 18:57 PDT, byeongha.cho
no flags Details | Formatted Diff | Diff
Patch (3.92 KB, patch)
2015-01-20 23:58 PST, byeongha.cho
no flags Details | Formatted Diff | Diff
Patch for landing (3.92 KB, patch)
2015-01-21 18:38 PST, byeongha.cho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description byeongha.cho 2014-08-27 18:46:16 PDT
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.
Comment 1 byeongha.cho 2014-08-27 18:57:05 PDT
Created attachment 237278 [details]
Patch
Comment 2 Daniel Bates 2014-08-27 21:14:07 PDT
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>.
Comment 3 byeongha.cho 2014-08-29 03:02:18 PDT
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??
Comment 4 byeongha.cho 2014-08-29 04:25:30 PDT
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??
Comment 5 byeongha.cho 2015-01-20 00:27:21 PST
I think idiom issue should be handled on another issue. Because, even though we add copy function for FontPlatformData, this patch is still needed.
Comment 6 Myles C. Maxfield 2015-01-20 18:05:11 PST
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.
Comment 7 Myles C. Maxfield 2015-01-20 18:06:44 PST
Comment on attachment 237278 [details]
Patch

Please file an issue for refactoring common code between the copy constructor and copy assignment operator.
Comment 8 byeongha.cho 2015-01-20 23:58:40 PST
Created attachment 245052 [details]
Patch
Comment 9 byeongha.cho 2015-01-21 00:00:31 PST
I have filed an issue for idiom.
https://bugs.webkit.org/show_bug.cgi?id=140718
Comment 10 byeongha.cho 2015-01-21 18:38:31 PST
Created attachment 245108 [details]
Patch for landing
Comment 11 Gyuyoung Kim 2015-01-23 01:51:11 PST
Comment on attachment 245108 [details]
Patch for landing

LGTM. cq=me
Comment 12 WebKit Commit Bot 2015-01-23 02:33:30 PST
Comment on attachment 245108 [details]
Patch for landing

Clearing flags on attachment: 245108

Committed r179006: <http://trac.webkit.org/changeset/179006>
Comment 13 WebKit Commit Bot 2015-01-23 02:33:34 PST
All reviewed patches have been landed.  Closing bug.