http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental/datetime-format.cc File src/extensions/experimental/datetime-format.cc (right):
http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental/datetime-format.cc#newcode113 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/datetime-format.cc#newcode265 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/datetime-format.h File src/extensions/experimental/datetime-format.h (right): http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental/datetime-format.h#newcode60 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/i18n-utils.cc File src/extensions/experimental/i18n-utils.cc (right): http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental/i18n-utils.cc#newcode60 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/i18n-utils.h File src/extensions/experimental/i18n-utils.h (right): http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental/i18n-utils.h#newcode36 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/i18n.js File src/extensions/experimental/i18n.js (right): http://codereview.chromium.org/7014019/diff/12007/src/extensions/experimental/i18n.js#newcode162 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. http://codereview.chromium.org/7014019/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
