This adds a formatter for plurals that works for all locales.

This adds a formatter for plurals that works for all locales.

This uses icu to generate plural strings for a given integer when
initially set up with a set of six grit message ids.

It should vastly simplify creating plurals, at least on the
programming side.

BUG=none
TEST=Ran new unit test.

[Greg Spencer (Chromium), 2011-03-24 19:39:37]

I needed to do a plural string in the UI, and couldn't find any code that
already existed for this.  Rather than embed it into my class, I though it best
to provide a common way to do it that was well documented and easier.

Let me know if I succeeded. :-)

[Paweł Hajdan Jr., 2011-03-25 09:32:17]

I'm not familiar enough with ICU to review code, so I'll let Jungshik focus on
he ICU parts.

In this review I focused on the public interface of the new class and its unit
tests.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.cc (right):

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:27: }  // anonymous namespace
nit: Add empty line above.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:29: class PluralFormatterImpl {
Is it worth to have an Impl class here? How about just forward-declaring
icu::PluralFormat (if possible) and using the scoped_ptr directly in
PluralFormatter?

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:56: DCHECK(U_SUCCESS(err));
DCHECKs are only for Debug builds. Don't you want to check the return value also
for Release builds?

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:80: DCHECK(U_SUCCESS(err));
Similarly here, I think you want to always check the return value. Maybe it
would be better to have a bool Init method?

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.h (right):

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:71: 
nit: Remove the empty line for the class comment.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:74: typedef std::string
(*StringSourceFunction)(int);
nit: Can this typedef be moved to the private part?

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:78: explicit PluralFormatter(const int
message_ids[]);
nit: Why do we have two ctors that do the same thing? It seems like the latter
(with 6 arguments) is better, because it is a compile-time failure if the number
of arguments doesn't match. With an array, we can only check its size during
runtime, at least with the [] declaration.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:88: private:
nit: Add empty line above.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:102: PluralFormatterImpl* impl_;
Why not just scoped_ptr?

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
File chrome/common/plural_formatter_unittest.cc (right):

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter_unittest.cc:76: #define TEST_MESSAGE_USED(msg)   
                        \
Do we really need to have macros for those? How about using functions?

[Greg Spencer (Chromium), 2011-03-25 23:05:31]

Thanks for the review, Paweł!

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.cc (right):

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:27: }  // anonymous namespace
On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> nit: Add empty line above.

Done.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:29: class PluralFormatterImpl {
On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> Is it worth to have an Impl class here? How about just forward-declaring
> icu::PluralFormat (if possible) and using the scoped_ptr directly in
> PluralFormatter?

This is a good point, and I considered it, but an implementation class
is necessary here because icu does some funky things with namespaces:
it declares its stuff under a macro-constructed namespace name and
then aliases it to 'icu', which means you have to include the icu
header before any forward declaration (which defeats the purpose of
forward declarations) in order to use the namespace "icu" or the
macro.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:56: DCHECK(U_SUCCESS(err));
On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> DCHECKs are only for Debug builds. Don't you want to check the return value
also
> for Release builds?

In this case, we want to fall back to a simple rule if we can't load the rule
from the locale.  It's still probably a programming error, though, so a DCHECK
is warranted.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:80: DCHECK(U_SUCCESS(err));
On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> Similarly here, I think you want to always check the return value. Maybe it
> would be better to have a bool Init method?

I had that at first, and went back to DCHECK to be consistent with what the
TimeFormat code (which a lot of this code is based on) already does, and because
I was thinking that any error here really indicates either a programming or
translator error, not an expected failure condition.

I think you're probably right though: a translator error is really a bad input,
not a programming error, so I'll switch this to an Init.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
File chrome/common/plural_formatter_unittest.cc (right):

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter_unittest.cc:76: #define TEST_MESSAGE_USED(msg)   
                        \
On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> Do we really need to have macros for those? How about using functions?

Good point! Done.

[tfarina, 2011-03-26 02:05:15]

As Pawel, I just focused on the public interface and the style things.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.cc (right):

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:7: #include <vector>
is this necessary/used in this source file?

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:11: #include "base/string16.h"
you can remove this include.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:23: static
PluralFormatter::StringSourceFunction g_string_source =
the static keyword is not necessary. Please, could you remove it here, and
below?

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:31: }  // anonymous namespace
Although I see in many places, and it's misleading (the correct is unnamed), I
think you can stick with just: // namespace (nothing is also ok).

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:152: bool PluralFormatter::Init(const int
message_ids[]) {
this should be implemented above. The order of the definition should match with
the order in the header file.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.h (right):

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:6: #define CHROME_COMMON_PLURAL_FORMATTER_H__
remove extra _ ;)

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:13: #include <vector>
I think you can remove this. It's not used in this header.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:16: #include "base/scoped_ptr.h"
you can remove this include.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:19: class PluralFormatterImpl;
Please, could you move this to line 103? It doesn't need to be visible by anyone
else except the PluralFormatter (PIMPL pattern ;))

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:90: private:
please, add a blank line above.

[Paweł Hajdan Jr., 2011-03-28 18:52:28]

LGTM with comments.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.cc (right):

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:29: class PluralFormatterImpl {
On 2011/03/25 23:05:31, Greg Spencer (Chromium) wrote:
> On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> > Is it worth to have an Impl class here? How about just forward-declaring
> > icu::PluralFormat (if possible) and using the scoped_ptr directly in
> > PluralFormatter?
> 
> This is a good point, and I considered it, but an implementation class
> is necessary here because icu does some funky things with namespaces:
> it declares its stuff under a macro-constructed namespace name and
> then aliases it to 'icu', which means you have to include the icu
> header before any forward declaration (which defeats the purpose of
> forward declarations) in order to use the namespace "icu" or the
> macro.

Makes sense.

http://codereview.chromium.org/6736003/diff/5001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:56: DCHECK(U_SUCCESS(err));
On 2011/03/25 23:05:31, Greg Spencer (Chromium) wrote:
> On 2011/03/25 09:32:19, Paweł Hajdan Jr. wrote:
> > DCHECKs are only for Debug builds. Don't you want to check the return value
> also
> > for Release builds?
> 
> In this case, we want to fall back to a simple rule if we can't load the rule
> from the locale.  It's still probably a programming error, though, so a DCHECK
> is warranted.

My understanding of this code is that this DCHECK will fire if we fail to load
the _fallback_ rules. If that's still intended, please add a short comment, the
intention of this DCHECK might be non-obvious.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:114: : impl_(new
PluralFormatterImpl(message_ids)) {
Later case handles U_FAILURE, and this only DCHECKs on it. It might be worth it
to signal failure via bool return parameter or some other way, but it's up to
you.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:119: }
nit: I'd suggest to make this a NOTREACHED() in the "if" below. It's checking
U_SUCCESS(error) and then right after U_FAILURE(error), which is a bit
surprising.

[Greg Spencer (Chromium), 2011-03-28 23:13:37]

Thanks for the drive-by, tfarina!

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.cc (right):

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:7: #include <vector>
On 2011/03/26 02:05:15, tfarina wrote:
> is this necessary/used in this source file?

Nope, not any more.  Removed.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:11: #include "base/string16.h"
On 2011/03/26 02:05:15, tfarina wrote:
> you can remove this include.

Done.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:23: static
PluralFormatter::StringSourceFunction g_string_source =
On 2011/03/26 02:05:15, tfarina wrote:
> the static keyword is not necessary. Please, could you remove it here, and
> below?

Done.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:31: }  // anonymous namespace
On 2011/03/26 02:05:15, tfarina wrote:
> Although I see in many places, and it's misleading (the correct is unnamed), I
> think you can stick with just: // namespace (nothing is also ok).

I hate that convention: it looks like someone forgot to type the name of the
namespace.  But you're correct: it is in the style guide, so I changed it.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:114: DCHECK(U_SUCCESS(error));
On 2011/03/28 18:52:29, Paweł Hajdan Jr. wrote:
> Later case handles U_FAILURE, and this only DCHECKs on it. It might be worth
it
> to signal failure via bool return parameter or some other way, but it's up to
> you.

OK, I added handling for this to make it consistent.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:119: DCHECK(U_SUCCESS(error));
On 2011/03/28 18:52:29, Paweł Hajdan Jr. wrote:
> nit: I'd suggest to make this a NOTREACHED() in the "if" below. It's checking
> U_SUCCESS(error) and then right after U_FAILURE(error), which is a bit
> surprising.

NOTREACHED is a good suggestion.  I switched to that.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.cc:152: bool PluralFormatter::Init(const int
message_ids[]) {
On 2011/03/26 02:05:15, tfarina wrote:
> this should be implemented above. The order of the definition should match
with
> the order in the header file.

Sorry, missed one. Done.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
File chrome/common/plural_formatter.h (right):

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:6: #define CHROME_COMMON_PLURAL_FORMATTER_H__
On 2011/03/26 02:05:15, tfarina wrote:
> remove extra _ ;)

Huh.  How did that get there?  Removed.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:13: #include <vector>
On 2011/03/26 02:05:15, tfarina wrote:
> I think you can remove this. It's not used in this header.

Done.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:16: #include "base/scoped_ptr.h"
On 2011/03/26 02:05:15, tfarina wrote:
> you can remove this include.

Done.

http://codereview.chromium.org/6736003/diff/8001/chrome/common/plural_formatt...
chrome/common/plural_formatter.h:19: class PluralFormatterImpl;
On 2011/03/26 02:05:15, tfarina wrote:
> Please, could you move this to line 103? It doesn't need to be visible by
anyone
> else except the PluralFormatter (PIMPL pattern ;))

Done.

[Greg Spencer (Chromium), 2011-03-29 17:17:56]

Wow.  All those change sets are because the server was returning a 500 when I
was trying to upload.  Please ignore all but the last one.

-Greg.

[tfarina, 2011-04-12 01:25:01]

Hi Greg, what is the status of this change?

http://codereview.chromium.org/6736003/diff/17043/chrome/common/plural_format...
File chrome/common/plural_formatter.h (right):

http://codereview.chromium.org/6736003/diff/17043/chrome/common/plural_format...
chrome/common/plural_formatter.h:67: 
remove this blank line, please.

http://codereview.chromium.org/6736003/diff/17043/chrome/common/plural_format...
chrome/common/plural_formatter.h:78: bool Init(int zero_message_id,
could add a blank line between these two methods?

http://codereview.chromium.org/6736003/diff/17043/chrome/common/plural_format...
chrome/common/plural_formatter.h:86: private:
please, add a blank line above.

http://codereview.chromium.org/6736003/diff/17043/chrome/common/plural_format...
chrome/common/plural_formatter.h:100: class PluralFormatterImpl;
Could you rename to simply Impl? So in the implementation you avoid being
redundant, because now it looks like: PluralFormatter::PluralFormatterImpl { ...
};

instead of: PluralFormatter::Impl { ... };

http://codereview.chromium.org/6736003/diff/17043/chrome/common/plural_format...
chrome/common/plural_formatter.h:101: 
no blank line here too, please.

[Greg Spencer (Chromium), 2011-04-12 16:11:31]

On 2011/04/12 01:25:01, tfarina wrote:
> Hi Greg, what is the status of this change?

Well... I ran into a road block because jshin wants to have this use the new TC
syntax used in google3 instead of the six separate messages.   It will take some
extra work to make that happen (perhaps even writing a parser), so this change
may be dead in the water, or it may change significantly.

I was writing this because I had a use case for it, and thought I'd do the right
thing and
make a general solution, but I won't have time to work on it for a couple of
weeks,  and there's no pressing need from my side.