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
