http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/datetime-format.cc
File src/extensions/experimental/datetime-format.cc (right):
http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/datetime-format.cc#newcode79
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/i18n-utils.cc
File src/extensions/experimental/i18n-utils.cc (right):
http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/i18n-utils.cc#newcode63
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/i18n.js
File src/extensions/experimental/i18n.js (right):
http://codereview.chromium.org/7014019/diff/8001/src/extensions/experimental/i18n.js#newcode155
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/i18n.js#newcode163
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.
http://codereview.chromium.org/7014019/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
