LGTM !!
On 2011/05/19 20:44:41, Nebojša Ćirić wrote:
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);
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/datetime-format.cc#newcode265
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/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.
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/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);
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/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;
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/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();
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.
http://codereview.chromium.org/7014019/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
