Bug 92976 – Move number localization code in LocaleICU.cpp to new class

• Component: Platform
• Status: RESOLVED FIXED

[Kent Tamura]

Move number localization code in LocaleICU.cpp to new class

[Kent Tamura]

Created attachment 156041 [details]
Patch

[Kentaro Hara]

Comment on attachment 156041 [details]
Patch

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

r- due to a wrong copyright.

> Source/WebCore/platform/text/NumberLocalizer.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

> Source/WebCore/platform/text/NumberLocalizer.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND

Please use Google's copyright.

> Source/WebCore/platform/text/NumberLocalizer.h:54
> +    String m_positivePrefix;
> +    String m_positiveSuffix;
> +    String m_negativePrefix;
> +    String m_negativeSuffix;
> +    bool m_hasNumberLocalizerData;

It looks a bit buggy to set these values one by one in the initialization method of a sub class (i.e. LocaleICU::initializeNumberLocalizerData()).

How about implementing NumberLocalizer::initializeNumberLocalizerData(String positivePrefix, String positiveSuffix, ...) that sets all data and flips m_hasNumberLocalizerData? LocaleICU::initializeNumberLocalizerData() can call NumberLocalizer::initializeNumberLocalizerData().

[Kent Tamura]

Comment on attachment 156041 [details]
Patch

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

>> Source/WebCore/platform/text/NumberLocalizer.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> 2012

This is the copyright header of the original code. We should not replace it.
But we should add 2012 because I added some code.

>> Source/WebCore/platform/text/NumberLocalizer.h:13
>> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND
> 
> Please use Google's copyright.

This is our standard copyright header. Please read a recent thread in our internal mail list.

[Kentaro Hara]

Comment on attachment 156041 [details]
Patch

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

>>> Source/WebCore/platform/text/NumberLocalizer.h:13
>>> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND
>> 
>> Please use Google's copyright.
> 
> This is our standard copyright header. Please read a recent thread in our internal mail list.

Thanks for clarification. Makes sense.

[Kent Tamura]

Created attachment 156059 [details]
Patch 2

[Kentaro Hara]

Comment on attachment 156059 [details]
Patch 2

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

> Source/WebCore/ChangeLog:11
> +        No new tests. This is just a refactoring.

This patch is not a trivial refactoring. It would be better to list up a couple of existing tests that cover this change.

[Kent Tamura]

Committed r124459: <http://trac.webkit.org/changeset/124459>