Bug 137738 - Move TextCodec classes to std::unique_ptr
Summary: Move TextCodec classes to std::unique_ptr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 128007
  Show dependency treegraph
 
Reported: 2014-10-15 03:17 PDT by Gyuyoung Kim
Modified: 2014-11-23 18:25 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.29 KB, patch)
2014-10-15 03:18 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (13.25 KB, patch)
2014-10-15 04:18 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for landing (13.78 KB, patch)
2014-10-15 18:06 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for landing (13.86 KB, patch)
2014-10-15 18:35 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2014-10-15 03:17:17 PDT
SSIA
Comment 1 Gyuyoung Kim 2014-10-15 03:18:04 PDT
Created attachment 239861 [details]
Patch
Comment 2 Gyuyoung Kim 2014-10-15 04:18:32 PDT
Created attachment 239864 [details]
Patch
Comment 3 Darin Adler 2014-10-15 09:39:31 PDT
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.
Comment 4 Gyuyoung Kim 2014-10-15 18:06:48 PDT
Created attachment 239912 [details]
Patch for landing
Comment 5 Gyuyoung Kim 2014-10-15 18:23:09 PDT
(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.
Comment 6 Gyuyoung Kim 2014-10-15 18:35:07 PDT
Created attachment 239914 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2014-10-15 19:51:00 PDT
Comment on attachment 239914 [details]
Patch for landing

Clearing flags on attachment: 239914

Committed r174757: <http://trac.webkit.org/changeset/174757>
Comment 8 WebKit Commit Bot 2014-10-15 19:51:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Dumez 2014-10-16 21:32:31 PDT
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.
Comment 10 Gyuyoung Kim 2014-10-16 22:48:37 PDT
(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.