Bug 139395

Created attachment 242808 [details] WIP

Created attachment 242876 [details] Patch

Created attachment 242978 [details] Patch

CC'ing Darin. Could you take a look this ?

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).

Created attachment 243182 [details] Patch

(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 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.

Created attachment 243195 [details] Patch

(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 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));

Created attachment 243248 [details] Patch

(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 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>.

Created attachment 243269 [details] Patch

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.

Created attachment 243272 [details] Patch

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?

(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 on attachment 243272 [details] Patch Clearing flags on attachment: 243272 Committed r177280: <http://trac.webkit.org/changeset/177280>

All reviewed patches have been landed. Closing bug.