Summary: | Initialization for some member variable of FontPlatformData | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | byeongha.cho | ||||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, mmaxfield | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
byeongha.cho
2014-08-27 18:46:16 PDT
Created attachment 237278 [details]
Patch
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. 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 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
Comment on attachment 245108 [details]
Patch for landing
LGTM. cq=me
Comment on attachment 245108 [details] Patch for landing Clearing flags on attachment: 245108 Committed r179006: <http://trac.webkit.org/changeset/179006> All reviewed patches have been landed. Closing bug. |