Bug 137738 – Move TextCodec classes to std::unique_ptr

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 239861 [details]
Patch

[Gyuyoung Kim]

Created attachment 239864 [details]
Patch

[Darin Adler]

Comment on attachment 239864 [details]
Patch

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

OK to do this, but please use make_unique, not unique_ptr/new.

> Source/WebCore/platform/text/TextCodecICU.cpp:72
> +    return std::unique_ptr<TextCodec>(new TextCodecICU(encoding.name(), static_cast<const char*>(additionalData)));

This should use std::make_unique, not new combined with std::unique_ptr.

> Source/WebCore/platform/text/TextCodecLatin1.cpp:109
> +    return std::unique_ptr<TextCodec>(new TextCodecLatin1);

Ditto.

> Source/WebCore/platform/text/TextCodecUTF16.cpp:53
> +    return std::unique_ptr<TextCodec>(new TextCodecUTF16(true));

Ditto.

> Source/WebCore/platform/text/TextCodecUTF16.cpp:58
> +    return std::unique_ptr<TextCodec>(new TextCodecUTF16(false));

Ditto.

> Source/WebCore/platform/text/TextCodecUTF8.cpp:43
> +    return std::unique_ptr<TextCodec>(new TextCodecUTF8);

Ditto.

> Source/WebCore/platform/text/TextCodecUserDefined.cpp:44
> +    return std::unique_ptr<TextCodec>(new TextCodecUserDefined);

Ditto.

> Source/WebCore/platform/text/mac/TextCodecMac.cpp:71
> +    return std::unique_ptr<TextCodec>(new TextCodecMac(*static_cast<const TECTextEncodingID*>(additionalData)));

Ditto.

[Gyuyoung Kim]

Created attachment 239912 [details]
Patch for landing

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 239864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239864&action=review
> 
> OK to do this, but please use make_unique, not unique_ptr/new.
> 
> > Source/WebCore/platform/text/TextCodecICU.cpp:72
> > +    return std::unique_ptr<TextCodec>(new TextCodecICU(encoding.name(), static_cast<const char*>(additionalData)));
> 
> This should use std::make_unique, not new combined with std::unique_ptr.
> 
> > Source/WebCore/platform/text/TextCodecLatin1.cpp:109
> > +    return std::unique_ptr<TextCodec>(new TextCodecLatin1);
> 
> Ditto.
> 
> > Source/WebCore/platform/text/TextCodecUTF16.cpp:53
> > +    return std::unique_ptr<TextCodec>(new TextCodecUTF16(true));
> 
> Ditto.
> 
> > Source/WebCore/platform/text/TextCodecUTF16.cpp:58
> > +    return std::unique_ptr<TextCodec>(new TextCodecUTF16(false));
> 
> Ditto.
> 
> > Source/WebCore/platform/text/TextCodecUTF8.cpp:43
> > +    return std::unique_ptr<TextCodec>(new TextCodecUTF8);
> 
> Ditto.
> 
> > Source/WebCore/platform/text/TextCodecUserDefined.cpp:44
> > +    return std::unique_ptr<TextCodec>(new TextCodecUserDefined);
> 
> Ditto.
> 
> > Source/WebCore/platform/text/mac/TextCodecMac.cpp:71
> > +    return std::unique_ptr<TextCodec>(new TextCodecMac(*static_cast<const TECTextEncodingID*>(additionalData)));
> 
> Ditto.

Thank you for pointing those things out. Fixed all.

[Gyuyoung Kim]

Created attachment 239914 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 239914 [details]
Patch for landing

Clearing flags on attachment: 239914

Committed r174757: <http://trac.webkit.org/changeset/174757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 239914 [details]
Patch for landing

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

> Source/WebCore/platform/text/TextCodecICU.h:39
> +        static std::unique_ptr<TextCodec> create(const TextEncoding&, const void* additionalData);

It is a bit sad that we had to make all these public :(

> Source/WebCore/platform/text/TextCodecUTF8.h:36
> +    TextCodecUTF8()

ditto.

[Gyuyoung Kim]

(In reply to comment #9)
> Comment on attachment 239914 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239914&action=review
> 
> > Source/WebCore/platform/text/TextCodecICU.h:39
> > +        static std::unique_ptr<TextCodec> create(const TextEncoding&, const void* additionalData);
> 
> It is a bit sad that we had to make all these public :(
> 
> > Source/WebCore/platform/text/TextCodecUTF8.h:36
> > +    TextCodecUTF8()
> 
> ditto.

I will also fix it after finishing to land the patch on Bug 137765.