Bug 139395 - Move WebCore/platform/text to std::unique_ptr
Summary: Move WebCore/platform/text to std::unique_ptr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore 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-12-08 07:07 PST by Gyuyoung Kim
Modified: 2014-12-15 09:17 PST (History)
4 users (show)

See Also:


Attachments
WIP (6.10 KB, patch)
2014-12-08 07:09 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.42 KB, patch)
2014-12-08 20:52 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.40 KB, patch)
2014-12-09 16:59 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.39 KB, patch)
2014-12-12 00:05 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.54 KB, patch)
2014-12-12 05:58 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.56 KB, patch)
2014-12-13 00:01 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.27 KB, patch)
2014-12-14 07:35 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (20.57 KB, patch)
2014-12-14 08:06 PST, 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-12-08 07:07:46 PST
SSIA
Comment 1 Gyuyoung Kim 2014-12-08 07:09:40 PST
Created attachment 242808 [details]
WIP
Comment 2 Gyuyoung Kim 2014-12-08 20:52:01 PST
Created attachment 242876 [details]
Patch
Comment 3 Gyuyoung Kim 2014-12-09 16:59:36 PST
Created attachment 242978 [details]
Patch
Comment 4 Gyuyoung Kim 2014-12-10 19:08:50 PST
CC'ing Darin. Could you take a look this ?
Comment 5 Mark Lam 2014-12-11 23:53:03 PST
Comment on attachment 242978 [details]
Patch

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

> Source/WebCore/platform/text/LocaleICU.cpp:167
> +std::unique_ptr<Vector<String> > LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)

nit: please remove the space between Vector<String> and the last >.

> Source/WebCore/platform/text/LocaleICU.cpp:193
> +static std::unique_ptr<Vector<String> > createFallbackMonthLabels()

Ditto.  Remove extra space.

> Source/WebCore/platform/text/LocaleICU.cpp:217
> +static std::unique_ptr<Vector<String> > createFallbackAMPMLabels()

Ditto.  Remove extra space.

> Source/WebCore/platform/text/LocaleICU.cpp:333
> -        if (OwnPtr<Vector<String> > labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {
> +        auto labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {

Strange.  How did this build?  The “if (” is missing.

> Source/WebCore/platform/text/mac/LocaleMac.h:50
> +    static std::unique_ptr<LocaleMac> create(NSLocale*);

I think no one calls this now that Locale::create() constructs LocaleMac directly with a make_unique.  If no one calls this anymore, please remove it (and the impl in the cpp file as well).
Comment 6 Gyuyoung Kim 2014-12-12 00:05:49 PST
Created attachment 243182 [details]
Patch
Comment 7 Gyuyoung Kim 2014-12-12 00:07:20 PST
(In reply to comment #5)
> Comment on attachment 242978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242978&action=review
> 
> > Source/WebCore/platform/text/LocaleICU.cpp:167
> > +std::unique_ptr<Vector<String> > LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)
> 
> nit: please remove the space between Vector<String> and the last >.
> 
> > Source/WebCore/platform/text/LocaleICU.cpp:193
> > +static std::unique_ptr<Vector<String> > createFallbackMonthLabels()
> 
> Ditto.  Remove extra space.
> 
> > Source/WebCore/platform/text/LocaleICU.cpp:217
> > +static std::unique_ptr<Vector<String> > createFallbackAMPMLabels()
> 
> Ditto.  Remove extra space.

All done.

> > Source/WebCore/platform/text/LocaleICU.cpp:333
> > -        if (OwnPtr<Vector<String> > labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {
> > +        auto labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {
> 
> Strange.  How did this build?  The “if (” is missing.

Weird. Anyway, fixed.

> > Source/WebCore/platform/text/mac/LocaleMac.h:50
> > +    static std::unique_ptr<LocaleMac> create(NSLocale*);
> 
> I think no one calls this now that Locale::create() constructs LocaleMac
> directly with a make_unique.  If no one calls this anymore, please remove it
> (and the impl in the cpp file as well).

Yes, If no one calls it. it should be removed.
Comment 8 Mark Lam 2014-12-12 00:20:31 PST
Comment on attachment 243182 [details]
Patch

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

Better but there’s a few other bits I missed last time.

> Source/WebCore/platform/text/LocaleICU.h:78
> +    std::unique_ptr<Vector<String> > createLabelVector(const UDateFormat*, UDateFormatSymbolType, int32_t startIndex, int32_t size);

Please remove the empty space.

> Source/WebCore/platform/text/LocaleICU.h:89
> +    std::unique_ptr<Vector<String> > m_monthLabels;

Please remove the empty space.

> Source/WebCore/platform/text/win/LocaleWin.cpp:149
>      return LocaleWin::create(LCIDFromLocale(locale));

For consistency, let’s change this to:
    return std::make_unique<LocaleWin>(LCIDFromLocale(locale));

> Source/WebCore/platform/text/win/LocaleWin.cpp:161
> -PassOwnPtr<LocaleWin> LocaleWin::create(LCID lcid)
> +std::unique_ptr<LocaleWin> LocaleWin::create(LCID lcid)
>  {
> -    return adoptPtr(new LocaleWin(lcid));
> +    return std::make_unique<LocaleWin>(lcid);
>  }

For consistency, let’s remove this.

> Source/WebCore/platform/text/win/LocaleWin.h:47
> -    static PassOwnPtr<LocaleWin> create(LCID);
> +    static std::unique_ptr<LocaleWin> create(LCID);

For consistency, let’s remove this and make the constructor public as is done in other ports.
Comment 9 Gyuyoung Kim 2014-12-12 05:58:04 PST
Created attachment 243195 [details]
Patch
Comment 10 Gyuyoung Kim 2014-12-12 05:58:41 PST
(In reply to comment #8)
> Comment on attachment 243182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243182&action=review
> 
> Better but there’s a few other bits I missed last time.
> 
> > Source/WebCore/platform/text/LocaleICU.h:78
> > +    std::unique_ptr<Vector<String> > createLabelVector(const UDateFormat*, UDateFormatSymbolType, int32_t startIndex, int32_t size);
> 
> Please remove the empty space.
> 
> > Source/WebCore/platform/text/LocaleICU.h:89
> > +    std::unique_ptr<Vector<String> > m_monthLabels;
> 
> Please remove the empty space.
> 
> > Source/WebCore/platform/text/win/LocaleWin.cpp:149
> >      return LocaleWin::create(LCIDFromLocale(locale));
> 
> For consistency, let’s change this to:
>     return std::make_unique<LocaleWin>(LCIDFromLocale(locale));
> 
> > Source/WebCore/platform/text/win/LocaleWin.cpp:161
> > -PassOwnPtr<LocaleWin> LocaleWin::create(LCID lcid)
> > +std::unique_ptr<LocaleWin> LocaleWin::create(LCID lcid)
> >  {
> > -    return adoptPtr(new LocaleWin(lcid));
> > +    return std::make_unique<LocaleWin>(lcid);
> >  }
> 
> For consistency, let’s remove this.
> 
> > Source/WebCore/platform/text/win/LocaleWin.h:47
> > -    static PassOwnPtr<LocaleWin> create(LCID);
> > +    static std::unique_ptr<LocaleWin> create(LCID);
> 
> For consistency, let’s remove this and make the constructor public as is
> done in other ports.

Ok, Fixed all.
Comment 11 Mark Lam 2014-12-12 09:40:51 PST
Comment on attachment 243195 [details]
Patch

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

> Source/WebCore/platform/text/win/LocaleWin.cpp:149
> +    return std::make_unique<LCIDFromLocale>(locale);

This is wrong.  It should be:
    return std::make_unique<LocaleWin>(LCIDFromLocale(locale));
Comment 12 Gyuyoung Kim 2014-12-13 00:01:19 PST
Created attachment 243248 [details]
Patch
Comment 13 Gyuyoung Kim 2014-12-13 00:01:48 PST
(In reply to comment #11)
> Comment on attachment 243195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243195&action=review
> 
> > Source/WebCore/platform/text/win/LocaleWin.cpp:149
> > +    return std::make_unique<LCIDFromLocale>(locale);
> 
> This is wrong.  It should be:
>     return std::make_unique<LocaleWin>(LCIDFromLocale(locale));

Oh, my stupid mistake. Fixed.
Comment 14 Darin Adler 2014-12-13 19:01:43 PST
Comment on attachment 243248 [details]
Patch

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

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:45
> +    LineBreakIteratorPool() { } 
> +

This line of code can and should just be omitted. The only reason we needed a declaration for the empty constructor before was to make it private, since it’s public by default. Leaving this out should give us what we want.

> Source/WebCore/platform/text/LocaleICU.cpp:167
> +std::unique_ptr<Vector<String>> LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)

This function should really just return a Vector, not a unique_ptr<Vector>. But that’s something someone else can fix later, I guess.

> Source/WebCore/platform/text/LocaleICU.cpp:193
> +static std::unique_ptr<Vector<String>> createFallbackMonthLabels()

As with the above, this function should return a Vector, not a std::unique_ptr<Vector>.

> Source/WebCore/platform/text/LocaleICU.cpp:217
> +static std::unique_ptr<Vector<String>> createFallbackAMPMLabels()

As with the above, this function should return a Vector, not a std::unique_ptr<Vector>.
Comment 15 Gyuyoung Kim 2014-12-14 07:35:06 PST
Created attachment 243269 [details]
Patch
Comment 16 Gyuyoung Kim 2014-12-14 07:37:41 PST
Comment on attachment 243248 [details]
Patch

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

>> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:45
>> +
> 
> This line of code can and should just be omitted. The only reason we needed a declaration for the empty constructor before was to make it private, since it’s public by default. Leaving this out should give us what we want.

yes, we don't need to keep empty constructor as public. Removed.

>> Source/WebCore/platform/text/LocaleICU.cpp:167
>> +std::unique_ptr<Vector<String>> LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)
> 
> This function should really just return a Vector, not a unique_ptr<Vector>. But that’s something someone else can fix later, I guess.

Let me file a new bug for handling your comment in near future.
Comment 17 Gyuyoung Kim 2014-12-14 08:06:06 PST
Created attachment 243272 [details]
Patch
Comment 18 Darin Adler 2014-12-14 17:04:37 PST
Comment on attachment 243272 [details]
Patch

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

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:44
> +    LineBreakIteratorPool() { }

It’s still here, but you said you removed it. Did something go wrong?
Comment 19 Gyuyoung Kim 2014-12-14 18:13:51 PST
(In reply to comment #18)
> Comment on attachment 243272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243272&action=review
> 
> > Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:44
> > +    LineBreakIteratorPool() { }
> 
> It’s still here, but you said you removed it. Did something go wrong?

Because there were a build break on EFL and GTK port when I removed it as below,

WTF_MAKE_NONCOPYABLE seems that LineBreakIteratorPool class doesn't allow to make a default constructor implicitly. Thus LineBreakIteratorPool seems to need to have it explicitly.


../../Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:49:18:   required from here
../../Source/WTF/wtf/ThreadSpecific.h:258:9: error: no matching function for call to ‘WebCore::LineBreakIteratorPool::LineBreakIteratorPool()’
         new (NotNull, ptr) T;
         ^
../../Source/WTF/wtf/ThreadSpecific.h:258:9: note: candidate is:
In file included from ../../Source/WebCore/platform/text/TextBreakIterator.cpp:25:0:
../../Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:42:14: note: WebCore::LineBreakIteratorPool::LineBreakIteratorPool(const WebCore::LineBreakIteratorPool&) <deleted>
     WTF_MAKE_NONCOPYABLE(LineBreakIteratorPool);
              ^
../../Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:42:14: note:   candidate expects 1 argument, 0 provided
Comment 20 WebKit Commit Bot 2014-12-15 09:17:08 PST
Comment on attachment 243272 [details]
Patch

Clearing flags on attachment: 243272

Committed r177280: <http://trac.webkit.org/changeset/177280>
Comment 21 WebKit Commit Bot 2014-12-15 09:17:14 PST
All reviewed patches have been landed.  Closing bug.