Bug 92976 - Move number localization code in LocaleICU.cpp to new class
Summary: Move number localization code in LocaleICU.cpp to new class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 93085 93236
  Show dependency tree / graph
 
Reported: 2012-08-02 04:51 PDT by Kent Tamura
Modified: 2012-08-06 02:02 PDT (History)
2 users (show)

See Also:

Attachments
Patch (18.50 KB, patch)
2012-08-02 04:55 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Patch 2 (24.17 KB, patch)
2012-08-02 06:08 PDT, Kent Tamura 		haraken: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-08-02 04:51:49 PDT 
Move number localization code in LocaleICU.cpp to new class
Comment 1 Kent Tamura 2012-08-02 04:55:49 PDT 
Created attachment 156041 [details]
Patch
Comment 2 Kentaro Hara 2012-08-02 05:20:58 PDT 
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().
Comment 3 Kent Tamura 2012-08-02 05:25:55 PDT 
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.
Comment 4 Kentaro Hara 2012-08-02 05:37:47 PDT 
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.
Comment 5 Kent Tamura 2012-08-02 06:08:12 PDT 
Created attachment 156059 [details]
Patch 2
Comment 6 Kentaro Hara 2012-08-02 06:19:58 PDT 
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.
Comment 7 Kent Tamura 2012-08-02 08:54:21 PDT 
Committed r124459: <http://trac.webkit.org/changeset/124459>