Adding DateTimeFormat class to i18n API.

Adding DateTimeFormat class to i18n API with following methods:

- format
- getWeekdays
- getMonths
- get Eras
- getAmPm

TEST=Visit i18n.kaziprst.org/datetimeformat.html
Committed: http://code.google.com/p/v8/source/detail?r=7967

[Nebojša Ćirić, 2011-05-13 18:47:24]

http://codereview.chromium.org/7014019/diff/5002/src/extensions/experimental/...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/5002/src/extensions/experimental/...
src/extensions/experimental/datetime-format.cc:80: ->NumberValue();
Mads: Is this a good way to get current date? The v8::Date::New requires double
to create new Date so I had to actually invoke JS script to do it.

[jungshik at Google, 2011-05-13 23:37:53]

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/datetime-format.h (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.h:37: }
U_USING_ICU_NAMESPACE is set to 0 for all the projects that rely on ICU in the
latest version of icu.gyp. So, I guess you don't need this any more.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.h:60: // Get list of months - narrow
or wide format.
how about formatting vs stand-alone? For use in menus, stand-alone names (as
opposed to "formatting" names) are needed. We need to review the spec and add
this distinction if that's not yet covered.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/i18n-utils.cc (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n-utils.cc:63: result->setTo(tmp);
*result = icu::UnicodeString::fromUTF8(*ascii_value) 
would work, too. However, what you did is as good.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/i18n-utils.h (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n-utils.h:38: }
this is not necessary any more with the latest icu.gyp in the chrome tree.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n.js:155: var shortDate = 'yMMdd';
Hmm... this is problematic in some locales. In Korean, SHORT date format does
not use zero-padding, but there is a pattern for MMdd in case zero-padded month
number and day number are desired. If the skeleton has 'MMdd', the pattern
picked up will do zero-padding, which is not actual 'Short' format for Korean in
CLDR. 

Perhaps, 'yMd' works better for some locales, but it may break other locales in
the opposite direction. 

I guess the best is to use real SHORT date style directly. It's more involved,
but I don't see any work around that.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n.js:163: settings.hasOwnProperty('timeType')) {
After our discussion, this point became unapplicable because you're gonna change
datetype/timetype handling per our discussion/the comment above. Just for the
record, I'm leaving it alone (in case we change our mind again :-))

How about this?

var skeleton = "";

if (has skeleton) {
   skeleton = settings...
} else {
  if (has ('dateType')) {
    var dt = settings['dateType'];
    if (dt === 'short') 
      skeleton = shortDate;
    else if (dt === 'medium')
      skeleton = mediumDate;
  }
  if (has timeType) {
     var tt = ....
     if (tt is short)
        skeleton += shortTime;
     ....
  }

  if (skeleton == "") 
     skeleton = shortTime + shortDate
}

[jungshik at Google, 2011-05-13 23:43:27]

BTW, CLDR has 'abbreviated' (e.g. Jan), 'narrow' (e.g. J in English) and 'wide'
(e.g. January) for month names and weekday names. (
http://unicode.org/repos/cldr-tmp/trunk/diff/by_type/calendar-gregorian.month...
). 

I guess we decided to support only wide and 'narrow' (that maps to CLDR's
'abbreviated'). We may need to get back to the spec and use 'abbreviated'
instead of 'narrow'. That way, we can add 'narrow' (CLDR sense) later.

[Nebojša Ćirić, 2011-05-14 00:11:37]

I'll take care of the rest on Monday.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/datetime-format.h (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.h:37: }
I need this for forward declaration of icu::SimpleDateFormat. You need to say:

namespace X {
class Y;
}

You can't say:

class X::Y;

On 2011/05/13 23:37:53, Jungshik Shin wrote:
> U_USING_ICU_NAMESPACE is set to 0 for all the projects that rely on ICU in the
> latest version of icu.gyp. So, I guess you don't need this any more.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.h:60: // Get list of months - narrow
or wide format.
We decided (last meeting) to use only standalone symbols, to avoid problems like
people concatenating symbols to get "proper" dates.

On 2011/05/13 23:37:53, Jungshik Shin wrote:
> how about formatting vs stand-alone? For use in menus, stand-alone names (as
> opposed to "formatting" names) are needed. We need to review the spec and add
> this distinction if that's not yet covered.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/i18n-utils.h (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n-utils.h:38: }
Same as for datetime-format.h. Forward declaration of class from icu namespace.

On 2011/05/13 23:37:53, Jungshik Shin wrote:
> this is not necessary any more with the latest icu.gyp in the chrome tree.

[Mads Ager (chromium), 2011-05-16 07:20:52]

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.cc:79: millis =
v8::Script::Compile(v8::String::New("eval('new Date()')"))->Run()
Since we don't have a Date::New() method not taking a double this is the right
way to get the current date as a date object. I would split this over a couple
of lines with named variables instead of having one big line.

More seriously, creating a date can throw an exception, you need to handle that
here. Consider the case where a user overwrites Date:

function Date() { throw 42; }

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.cc:172:
CreateDateTimeFormat(args[0]->ToString(), args[1]->ToString());
String conversions can throw exceptions. If you expect the arguments to be
strings you should check before converting. If not, you need to check for
exceptions and deal with the exception case.

 V8 version 3.3.7 (candidate)
 > var o = { valueOf: function() { throw 42; } }
 > o + ''

The string conversion of 'o' throws.

[Nebojša Ćirić, 2011-05-17 20:40:37]

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/datetime-format.cc:79: millis =
v8::Script::Compile(v8::String::New("eval('new Date()')"))->Run()
On 2011/05/16 07:20:52, Mads Ager wrote:
> Since we don't have a Date::New() method not taking a double this is the right
> way to get the current date as a date object. I would split this over a couple
> of lines with named variables instead of having one big line.
> 
> More seriously, creating a date can throw an exception, you need to handle
that
> here. Consider the case where a user overwrites Date:
> 
> function Date() { throw 42; }
> 

Done.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/i18n-utils.cc (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n-utils.cc:63: result->setTo(tmp);
Yours is more readable.

On 2011/05/13 23:37:53, Jungshik Shin wrote:
> *result = icu::UnicodeString::fromUTF8(*ascii_value) 
> would work, too. However, what you did is as good. 
>

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n.js:155: var shortDate = 'yMMdd';
On 2011/05/13 23:37:53, Jungshik Shin wrote:
> Hmm... this is problematic in some locales. In Korean, SHORT date format does
> not use zero-padding, but there is a pattern for MMdd in case zero-padded
month
> number and day number are desired. If the skeleton has 'MMdd', the pattern
> picked up will do zero-padding, which is not actual 'Short' format for Korean
in
> CLDR. 
> 
> Perhaps, 'yMd' works better for some locales, but it may break other locales
in
> the opposite direction. 
> 
> I guess the best is to use real SHORT date style directly. It's more involved,
> but I don't see any work around that.

Done.

http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/...
src/extensions/experimental/i18n.js:163: settings.hasOwnProperty('timeType')) {
On 2011/05/13 23:37:53, Jungshik Shin wrote:
> After our discussion, this point became unapplicable because you're gonna
change
> datetype/timetype handling per our discussion/the comment above. Just for the
> record, I'm leaving it alone (in case we change our mind again :-))
> 
> How about this?
> 
> var skeleton = "";
> 
> if (has skeleton) {
>    skeleton = settings...
> } else {
>   if (has ('dateType')) {
>     var dt = settings['dateType'];
>     if (dt === 'short') 
>       skeleton = shortDate;
>     else if (dt === 'medium')
>       skeleton = mediumDate;
>   }
>   if (has timeType) {
>      var tt = ....
>      if (tt is short)
>         skeleton += shortTime;
>      ....
>   }
> 
>   if (skeleton == "") 
>      skeleton = shortTime + shortDate
> }

Done.

[Mads Ager (chromium), 2011-05-18 05:36:35]

Bindings part LGTM

http://codereview.chromium.org/7014019/diff/10001/src/extensions/experimental...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/10001/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:84: return
v8::ThrowException(v8::Exception::Error(
In case of an exception here, do you want to catch it and throw something else
or do you want to just rethrow the exception from 'new Date()'?

I think I would lean towards using try_catch.ReThrow() here, but I'll leave that
to your judgement.

http://codereview.chromium.org/7014019/diff/10001/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:177: CHECK_EQ(2, args.Length());
Ah, so this can only ever be called with two strings?

These should be ASSERT_EQ and ASSERT. Thanks for adding the assertions!

[Nebojša Ćirić, 2011-05-18 17:50:12]

http://codereview.chromium.org/7014019/diff/10001/src/extensions/experimental...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/10001/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:84: return
v8::ThrowException(v8::Exception::Error(
Re-throwing is what I wanted to do. Thanks.

On 2011/05/18 05:36:35, Mads Ager wrote:
> In case of an exception here, do you want to catch it and throw something else
> or do you want to just rethrow the exception from 'new Date()'?
> 
> I think I would lean towards using try_catch.ReThrow() here, but I'll leave
that
> to your judgement.

http://codereview.chromium.org/7014019/diff/10001/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:177: CHECK_EQ(2, args.Length());
It's called with languageID (a string) and settings object. JavaScript part of
the binding is supposed to create them and pass in.

On 2011/05/18 05:36:35, Mads Ager wrote:
> Ah, so this can only ever be called with two strings?
> 
> These should be ASSERT_EQ and ASSERT. Thanks for adding the assertions!

[jungshik at Google, 2011-05-19 18:24:05]

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:113: const icu::UnicodeString*
wide = symbols->getMonths(wide_count);
Again, how about 'narrow'? 

More importantly, using getMonths/getShortMonths method, you're returning month
names of 'context = format' instead of 'context=stand-alone'. (see the ICU
source code ( i18n/dtfmtsym.cpp). ICU documentation needs to make it clear. I've
filed a bug against ICU ( http://bugs.icu-project.org/trac/ticket/8587)

Please, use a version of getMonths accepting 'context' and 'width' : 

getMonths(int32_t & 	count, DtContextType 	context, DtWidthType) 

The same is true of GetWeekDays

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:265: icu::UnicodeString short_dt
= icu::UnicodeString::fromUTF8("short");
short_dt("short", -1, US_INV);


is more succinct. 

BTW, wouldn't you just go ahead to support all the types - full, long, medium
and short ?  Perhaps, those not yet standardized can have "v8" prefixes.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/datetime-format.h (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/datetime-format.h:60: // Get list of months - narrow
or wide format.
In LDML, there are 3 widths for month and weekday names : wide, abbreviated and
narrow and 2 contexts, format and stand-alone.

( http://unicode.org/reports/tr35/#Calendar_Elements )

 So, it'd better be made clear that what this function returns is 

stand-alone month names with the width of either abbreviated or wide.

BTW, we don't want to support stand-alone narrow names? 

Narrow names can be useful when you have a small amount of space for menu or
checkbox/radio buttons. 

I think it's a good idea to follow the terminology/convention of LDML. 

The same is true of other lists below.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/i18n-utils.cc (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/i18n-utils.cc:60: v8::String::AsciiValue
ascii_value(value);
JFYI: For now, all the settings have ASCII values, but if we add 'pattern'
(v8pattern) to DateTimeFormatter, 'value' can be non-ASCII, too.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/i18n-utils.h (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/i18n-utils.h:36: class SimpleDateFormat;
You don't need to forward-declare SimpleDateFormat here, do you?

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/i18n.js:162: var dt =
settings['dateType'].toLowerCase();
hmm.... I don't think it's a good idea to call toLowerCase() here. V8's
toLowerCase is ok as it is now for this use case, but ....


 I'd rather not accept anything but 'long', 'short' and 'v8full', 'v8medium')
because I don't see any reason to be lenient for dateType values.

[Nebojša Ćirić, 2011-05-19 20:44:41]

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/datetime-format.cc (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:113: const icu::UnicodeString*
wide = symbols->getMonths(wide_count);
Narrow was not part of the proposed standard, but I added support for it (now).

Switched to getMonths(, DtContext...) methods.

On 2011/05/19 18:24:05, Jungshik Shin wrote:
> Again, how about 'narrow'? 
> 
> More importantly, using getMonths/getShortMonths method, you're returning
month
> names of 'context = format' instead of 'context=stand-alone'. (see the ICU
> source code ( i18n/dtfmtsym.cpp). ICU documentation needs to make it clear.
I've
> filed a bug against ICU ( http://bugs.icu-project.org/trac/ticket/8587)
> 
> Please, use a version of getMonths accepting 'context' and 'width' : 
> 
> getMonths(int32_t & 	count, DtContextType 	context, DtWidthType) 
> 
> The same is true of GetWeekDays

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/datetime-format.cc:265: icu::UnicodeString short_dt
= icu::UnicodeString::fromUTF8("short");
I support all types now. We should state in proposal that those names should be
reserved.

Also, I don't really like , -1, US_INV part of that method. It's not readable
vs. fromUTF8. It's gone anyways.

On 2011/05/19 18:24:05, Jungshik Shin wrote:
> short_dt("short", -1, US_INV);
> 
> 
> is more succinct. 
> 
> BTW, wouldn't you just go ahead to support all the types - full, long, medium
> and short ?  Perhaps, those not yet standardized can have "v8" prefixes.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/datetime-format.h (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/datetime-format.h:60: // Get list of months - narrow
or wide format.
On 2011/05/19 18:24:05, Jungshik Shin wrote:
> In LDML, there are 3 widths for month and weekday names : wide, abbreviated
and
> narrow and 2 contexts, format and stand-alone.
> 
> ( http://unicode.org/reports/tr35/#Calendar_Elements )
> 
>  So, it'd better be made clear that what this function returns is 
> 
> stand-alone month names with the width of either abbreviated or wide.
> 
> BTW, we don't want to support stand-alone narrow names? 
> 
> Narrow names can be useful when you have a small amount of space for menu or
> checkbox/radio buttons. 
> 
> I think it's a good idea to follow the terminology/convention of LDML. 
> 
> The same is true of other lists below.

Done.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/i18n-utils.cc (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/i18n-utils.cc:60: v8::String::AsciiValue
ascii_value(value);
I made it Ut8Value.

On 2011/05/19 18:24:05, Jungshik Shin wrote:
> JFYI: For now, all the settings have ASCII values, but if we add 'pattern'
> (v8pattern) to DateTimeFormatter, 'value' can be non-ASCII, too.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/i18n-utils.h (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/i18n-utils.h:36: class SimpleDateFormat;
On 2011/05/19 18:24:05, Jungshik Shin wrote:
> You don't need to forward-declare SimpleDateFormat here, do you? 
>  

Done.

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
src/extensions/experimental/i18n.js:162: var dt =
settings['dateType'].toLowerCase();
On 2011/05/19 18:24:05, Jungshik Shin wrote:
> hmm.... I don't think it's a good idea to call toLowerCase() here. V8's
> toLowerCase is ok as it is now for this use case, but ....
> 
> 
>  I'd rather not accept anything but 'long', 'short' and 'v8full', 'v8medium')
> because I don't see any reason to be lenient for dateType values. 

Done.

[jungshik at Google, 2011-05-19 21:00:34]

LGTM !!

On 2011/05/19 20:44:41, Nebojša Ćirić wrote:
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> File src/extensions/experimental/datetime-format.cc (right):
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> src/extensions/experimental/datetime-format.cc:113: const icu::UnicodeString*
> wide = symbols->getMonths(wide_count);
> Narrow was not part of the proposed standard, but I added support for it
(now).
> 
> Switched to getMonths(, DtContext...) methods.
> 
> On 2011/05/19 18:24:05, Jungshik Shin wrote:
> > Again, how about 'narrow'? 
> > 
> > More importantly, using getMonths/getShortMonths method, you're returning
> month
> > names of 'context = format' instead of 'context=stand-alone'. (see the ICU
> > source code ( i18n/dtfmtsym.cpp). ICU documentation needs to make it clear.
> I've
> > filed a bug against ICU ( http://bugs.icu-project.org/trac/ticket/8587)
> > 
> > Please, use a version of getMonths accepting 'context' and 'width' : 
> > 
> > getMonths(int32_t & 	count, DtContextType 	context, DtWidthType) 
> > 
> > The same is true of GetWeekDays
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> src/extensions/experimental/datetime-format.cc:265: icu::UnicodeString
short_dt
> = icu::UnicodeString::fromUTF8("short");
> I support all types now. We should state in proposal that those names should
be
> reserved.
> 
> Also, I don't really like , -1, US_INV part of that method. It's not readable
> vs. fromUTF8. It's gone anyways.
> 
> On 2011/05/19 18:24:05, Jungshik Shin wrote:
> > short_dt("short", -1, US_INV);
> > 
> > 
> > is more succinct. 
> > 
> > BTW, wouldn't you just go ahead to support all the types - full, long,
medium
> > and short ?  Perhaps, those not yet standardized can have "v8" prefixes.
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> File src/extensions/experimental/datetime-format.h (right):
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> src/extensions/experimental/datetime-format.h:60: // Get list of months -
narrow
> or wide format.
> On 2011/05/19 18:24:05, Jungshik Shin wrote:
> > In LDML, there are 3 widths for month and weekday names : wide, abbreviated
> and
> > narrow and 2 contexts, format and stand-alone.
> > 
> > ( http://unicode.org/reports/tr35/#Calendar_Elements )
> > 
> >  So, it'd better be made clear that what this function returns is 
> > 
> > stand-alone month names with the width of either abbreviated or wide.
> > 
> > BTW, we don't want to support stand-alone narrow names? 
> > 
> > Narrow names can be useful when you have a small amount of space for menu or
> > checkbox/radio buttons. 
> > 
> > I think it's a good idea to follow the terminology/convention of LDML. 
> > 
> > The same is true of other lists below.
> 
> Done.
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> File src/extensions/experimental/i18n-utils.cc (right):
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> src/extensions/experimental/i18n-utils.cc:60: v8::String::AsciiValue
> ascii_value(value);
> I made it Ut8Value.
> 
> On 2011/05/19 18:24:05, Jungshik Shin wrote:
> > JFYI: For now, all the settings have ASCII values, but if we add 'pattern'
> > (v8pattern) to DateTimeFormatter, 'value' can be non-ASCII, too.
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> File src/extensions/experimental/i18n-utils.h (right):
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> src/extensions/experimental/i18n-utils.h:36: class SimpleDateFormat;
> On 2011/05/19 18:24:05, Jungshik Shin wrote:
> > You don't need to forward-declare SimpleDateFormat here, do you? 
> >  
> 
> Done.
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> File src/extensions/experimental/i18n.js (right):
> 
>
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental...
> src/extensions/experimental/i18n.js:162: var dt =
> settings['dateType'].toLowerCase();
> On 2011/05/19 18:24:05, Jungshik Shin wrote:
> > hmm.... I don't think it's a good idea to call toLowerCase() here. V8's
> > toLowerCase is ok as it is now for this use case, but ....
> > 
> > 
> >  I'd rather not accept anything but 'long', 'short' and 'v8full',
'v8medium')
> > because I don't see any reason to be lenient for dateType values. 
> 
> Done.