Bug 137905

Summary: Use std::unique_ptr | std::make_unique in FontCacheFoo
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebCore Misc.Assignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mmaxfield, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128007    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Gyuyoung Kim 2014-10-20 18:19:38 PDT
SSIA
Comment 1 Gyuyoung Kim 2014-10-20 18:43:26 PDT
Created attachment 240171 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-20 18:44:12 PDT
Attachment 240171 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2014-10-20 18:58:13 PDT
Comment on attachment 240171 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:87
> +std::unique_ptr<TextLayout> Font::createLayout(RenderText* text, float xPos, bool collapseWhiteSpace) const

Need to update this in Font.h too.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:91
> +    return std::make_unique<TextLayout>(text, *this, xPos));

Extra end parenthesis on this line.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243
> +    auto platformData = std::make_unique<FontPlatformData>(platformFont, size, fontDescription.usePrinterFont(), syntheticBold, syntheticOblique, fontDescription.orientation(), fontDescription.widthVariant()));
> +    return platformData;

No need for a local variable here. Just return make_unique.
Comment 4 Gyuyoung Kim 2014-10-20 20:44:12 PDT
Created attachment 240181 [details]
Patch
Comment 5 Gyuyoung Kim 2014-10-20 20:47:29 PDT
(In reply to comment #3)
> Comment on attachment 240171 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240171&action=review
> 
> > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:87
> > +std::unique_ptr<TextLayout> Font::createLayout(RenderText* text, float xPos, bool collapseWhiteSpace) const
> 
> Need to update this in Font.h too.

It is hard to touch createLayout() in this patch, because Font::createLayout() of Font.cpp generates build error as below, 

std::unique_ptr<TextLayout> Font::createLayout(RenderText*, float, bool) const
{
    return nullptr;
}


/platform/graphics/Font.cpp.o -c ../../Source/WebCore/platform/graphics/Font.cpp
In file included from /usr/include/c++/4.8/memory:81:0,
                 from ../../Source/WTF/wtf/StdLibExtras.h:31,
                 from ../../Source/WTF/wtf/FastMalloc.h:27,
                 from ../../Source/WebCore/config.h:75,
                 from ../../Source/WebCore/platform/graphics/Font.cpp:24:
/usr/include/c++/4.8/bits/unique_ptr.h: In instantiation of ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = WebCore::TextLayout]’:
/usr/include/c++/4.8/bits/unique_ptr.h:184:16:   required from ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = WebCore::TextLayout; _Dp = std::default_delete<WebCore::TextLayout>]’
/usr/include/c++/4.8/bits/unique_ptr.h:157:61:   required from ‘constexpr std::unique_ptr<_Tp, _Dp>::unique_ptr(std::nullptr_t) [with _Tp = WebCore::TextLayout; _Dp = std::default_delete<WebCore::TextLayout>; std::nullptr_t = std::nullptr_t]’
../../Source/WebCore/platform/graphics/Font.cpp:420:12:   required from here
/usr/include/c++/4.8/bits/unique_ptr.h:65:22: error: invalid application of ‘sizeof’ to incomplete type ‘WebCore::TextLayout’
  static_assert(sizeof(_Tp)>0,

I would like to handle createLayout() in new bug.


> > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:91
> > +    return std::make_unique<TextLayout>(text, *this, xPos));
> 
> Extra end parenthesis on this line.
> 
> > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243
> > +    auto platformData = std::make_unique<FontPlatformData>(platformFont, size, fontDescription.usePrinterFont(), syntheticBold, syntheticOblique, fontDescription.orientation(), fontDescription.widthVariant()));
> > +    return platformData;
> 
> No need for a local variable here. Just return make_unique.

I'm sorry, I missed to love mac port files.
Comment 6 Gyuyoung Kim 2014-10-20 21:30:33 PDT
Created attachment 240183 [details]
Patch
Comment 7 Darin Adler 2014-10-21 08:45:30 PDT
Comment on attachment 240183 [details]
Patch

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

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:589
> +        result = nullptr;

Wait, I didn’t notice. This line of code is not needed and can just be omitted.
Comment 8 Gyuyoung Kim 2014-10-21 16:54:31 PDT
Created attachment 240229 [details]
Patch for landing
Comment 9 Gyuyoung Kim 2014-10-21 16:55:23 PDT
(In reply to comment #7)
> Comment on attachment 240183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240183&action=review
> 
> > Source/WebCore/platform/graphics/win/FontCacheWin.cpp:589
> > +        result = nullptr;
> 
> Wait, I didn’t notice. This line of code is not needed and can just be
> omitted.

Ok, removed it.
Comment 10 WebKit Commit Bot 2014-10-21 18:53:05 PDT
Comment on attachment 240229 [details]
Patch for landing

Clearing flags on attachment: 240229

Committed r175013: <http://trac.webkit.org/changeset/175013>
Comment 11 WebKit Commit Bot 2014-10-21 18:53:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Tim Horton 2014-10-21 19:57:24 PDT
Hopeful iOS build fix in http://trac.webkit.org/changeset/175017.
Comment 13 Gyuyoung Kim 2014-10-21 20:44:09 PDT
(In reply to comment #12)
> Hopeful iOS build fix in http://trac.webkit.org/changeset/175017.

Oops, thank you for fixing it.