Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(613)

Issue 2724373002: [date] Add ICU backend for timezone info behind a flag (Closed)

Created:
3 years, 9 months ago by Dan Ehrenberg
Modified:
3 years, 8 months ago
CC:
v8-reviews_googlegroups.com, Michael Hablich, Yang, Benedikt Meurer, jgruber
Target Ref:
refs/heads/master
Project:
v8
Visibility:
Public.

Description

[date] Add ICU backend for timezone info behind a flag This patch implements a timezone backend which is based on ICU, rather than operating system calls. It can be turned on by passing the --icu-timezone-data flag. The goal here is to take advantage of ICU's data, which is more complete than the data that some system calls expose. For example, without any special code, this patch fixes the time zone of Lord Howe Island to have a correct 30 minute DST offset, rather than 60 minutes as the OS backends assume it to have. Unfortunately, the parenthized timezone name in Date.prototype.toString() differs across platforms. This patch chooses the long timezone name, which matches Windows behavior and might be the most intelligible, but the web compatibility impact is unclear. BUG=v8:6031, v8:2137, v8:6076 Review-Url: https://codereview.chromium.org/2724373002 Cr-Commit-Position: refs/heads/master@{#44562} Committed: https://chromium.googlesource.com/v8/v8/+/b213f2399038a615cdfbfa0201cddc113d304018

Patch Set 1 #

Patch Set 2 : Make cast explicit #

Patch Set 3 : Make casts even better #

Patch Set 4 : More cast tweaks #

Patch Set 5 : Fix a negation #

Patch Set 6 : Fall back to OS time zone name #

Patch Set 7 : Don't leak the icu::TimeZone* #

Total comments: 16

Patch Set 8 : Changes from review #

Patch Set 9 : ulan's formatting suggestion #

Total comments: 5

Patch Set 10 : Make it a non-Harmony flag and remove OS fallback #

Patch Set 11 : Convert to UTF8 rather than check-failing #

Total comments: 2

Patch Set 12 : Check-fail on overflow #

Patch Set 13 : Rebase and add a couple tests #

Patch Set 14 : add test file #

Patch Set 15 : Use distinctive prefix #

Total comments: 13

Patch Set 16 : Fix up test infrastructure #

Patch Set 17 : or {} #

Unified diffs Side-by-side diffs Delta from patch set Stats (+157 lines, -10 lines) Patch
M src/date.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -3 lines 0 comments Download
M src/date.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +16 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +5 lines, -0 lines 0 comments Download
M src/i18n.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +30 lines, -0 lines 0 comments Download
M src/i18n.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +53 lines, -0 lines 0 comments Download
A test/mjsunit/icu-date-lord-howe.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +14 lines, -0 lines 0 comments Download
A test/mjsunit/icu-date-to-string.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +14 lines, -0 lines 0 comments Download
M test/mjsunit/mjsunit.status View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments Download
M test/mjsunit/testcfg.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +7 lines, -0 lines 0 comments Download
M tools/testrunner/local/commands.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +4 lines, -3 lines 0 comments Download
M tools/testrunner/local/execution.py View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +5 lines, -3 lines 0 comments Download
M tools/testrunner/objects/testcase.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +4 lines, -1 line 0 comments Download

Messages

Total messages: 101 (74 generated)
Dan Ehrenberg
3 years, 9 months ago (2017-03-02 23:07:41 UTC) #26
jgruber
https://codereview.chromium.org/2724373002/diff/120001/src/i18n.cc File src/i18n.cc (right): https://codereview.chromium.org/2724373002/diff/120001/src/i18n.cc#newcode902 src/i18n.cc:902: ICUTimezoneCache::ICUTimezoneCache() { Clear(); } I'm surprised this doesn't fail. ...
3 years, 9 months ago (2017-03-03 08:35:30 UTC) #31
ulan
https://codereview.chromium.org/2724373002/diff/120001/src/date.cc File src/date.cc (right): https://codereview.chromium.org/2724373002/diff/120001/src/date.cc#newcode32 src/date.cc:32: FLAG_icu_timezone_data ? new ICUOSTimezoneCache() : nit: let's keep the ...
3 years, 9 months ago (2017-03-03 13:20:27 UTC) #32
Dan Ehrenberg
https://codereview.chromium.org/2724373002/diff/120001/src/i18n.cc File src/i18n.cc (right): https://codereview.chromium.org/2724373002/diff/120001/src/i18n.cc#newcode902 src/i18n.cc:902: ICUTimezoneCache::ICUTimezoneCache() { Clear(); } On 2017/03/03 08:35:29, jgruber wrote: ...
3 years, 9 months ago (2017-03-03 13:20:27 UTC) #33
jgruber
https://codereview.chromium.org/2724373002/diff/120001/src/i18n.cc File src/i18n.cc (right): https://codereview.chromium.org/2724373002/diff/120001/src/i18n.cc#newcode908 src/i18n.cc:908: char* name = is_dst ? dst_timezone_name_ : timezone_name_; On ...
3 years, 9 months ago (2017-03-03 13:29:08 UTC) #37
Dan Ehrenberg
Seems like the big unknown is what the right way to get the short timezone ...
3 years, 9 months ago (2017-03-03 15:06:54 UTC) #43
jungshik at Google
> for example, CET becomes GMT+1 in en_US. To avoid changing currently correct short acronyms, ...
3 years, 9 months ago (2017-03-08 06:33:01 UTC) #46
jungshik at Google
After writing the previous comment and an in-line comment below, I realized that there's one ...
3 years, 9 months ago (2017-03-08 06:44:41 UTC) #47
jungshik at Google
https://codereview.chromium.org/2724373002/diff/160001/src/i18n.cc File src/i18n.cc (right): https://codereview.chromium.org/2724373002/diff/160001/src/i18n.cc#newcode911 src/i18n.cc:911: GetTimeZone()->getDisplayName(is_dst, icu::TimeZone::SHORT, result); By using SHORT, you're matching v8 ...
3 years, 9 months ago (2017-03-08 07:15:20 UTC) #48
jungshik at Google
On 2017/03/08 06:33:01, jungshik at Google wrote: > > for example, CET becomes GMT+1 in ...
3 years, 9 months ago (2017-03-08 07:23:07 UTC) #49
jungshik at Google
https://codereview.chromium.org/2724373002/diff/160001/src/i18n.cc File src/i18n.cc (right): https://codereview.chromium.org/2724373002/diff/160001/src/i18n.cc#newcode911 src/i18n.cc:911: GetTimeZone()->getDisplayName(is_dst, icu::TimeZone::SHORT, result); On 2017/03/08 07:15:20, jungshik at Google ...
3 years, 9 months ago (2017-03-08 07:27:43 UTC) #50
ulan
https://codereview.chromium.org/2724373002/diff/120001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2724373002/diff/120001/src/flag-definitions.h#newcode214 src/flag-definitions.h:214: V(icu_timezone_data, "get information about timezones from ICU") On 2017/03/03 ...
3 years, 9 months ago (2017-03-10 17:55:38 UTC) #51
Dan Ehrenberg
https://codereview.chromium.org/2724373002/diff/120001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2724373002/diff/120001/src/flag-definitions.h#newcode214 src/flag-definitions.h:214: V(icu_timezone_data, "get information about timezones from ICU") On 2017/03/10 ...
3 years, 9 months ago (2017-03-11 09:10:50 UTC) #53
Dan Ehrenberg
The new version switches to a LONG time format, and it also fixes an issue ...
3 years, 9 months ago (2017-03-12 00:00:51 UTC) #54
ulan
From implementation point of view the CL looks good to me. I don't know how ...
3 years, 9 months ago (2017-03-17 11:00:06 UTC) #59
Dan Ehrenberg
Since this patch is behind a flag, what if we land this version for now ...
3 years, 8 months ago (2017-04-06 15:35:21 UTC) #60
Dan Ehrenberg
I added some tests which change the default timezone and locale. I'm not sure how ...
3 years, 8 months ago (2017-04-06 17:38:20 UTC) #69
Michael Achenbach
https://codereview.chromium.org/2724373002/diff/280001/tools/testrunner/local/commands.py File tools/testrunner/local/commands.py (left): https://codereview.chromium.org/2724373002/diff/280001/tools/testrunner/local/commands.py#oldcode53 tools/testrunner/local/commands.py:53: def RunProcess(verbose, timeout, args, **rest): Why not keep **rest? ...
3 years, 8 months ago (2017-04-07 13:02:10 UTC) #76
Dan Ehrenberg
Made changes based on machenbach's comments. I can't tell if the CQ failures have to ...
3 years, 8 months ago (2017-04-07 14:06:09 UTC) #81
Dan Ehrenberg
Made changes based on machenbach's comments. I can't tell if the CQ failures have to ...
3 years, 8 months ago (2017-04-07 14:06:12 UTC) #82
ulan
lgtm from my side.
3 years, 8 months ago (2017-04-07 17:08:40 UTC) #83
Michael Achenbach
lgtm https://codereview.chromium.org/2724373002/diff/280001/tools/testrunner/local/commands.py File tools/testrunner/local/commands.py (right): https://codereview.chromium.org/2724373002/diff/280001/tools/testrunner/local/commands.py#newcode129 tools/testrunner/local/commands.py:129: def Execute(args, verbose=False, timeout=None, env={}): On 2017/04/07 14:06:09, ...
3 years, 8 months ago (2017-04-11 08:38:36 UTC) #88
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2724373002/320001
3 years, 8 months ago (2017-04-11 11:30:35 UTC) #95
Dan Ehrenberg
https://codereview.chromium.org/2724373002/diff/280001/tools/testrunner/local/commands.py File tools/testrunner/local/commands.py (right): https://codereview.chromium.org/2724373002/diff/280001/tools/testrunner/local/commands.py#newcode129 tools/testrunner/local/commands.py:129: def Execute(args, verbose=False, timeout=None, env={}): On 2017/04/11 08:38:36, Michael ...
3 years, 8 months ago (2017-04-11 11:31:51 UTC) #96
Dan Ehrenberg
3 years, 8 months ago (2017-04-11 11:31:58 UTC) #97
commit-bot: I haz the power
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/v8/v8/+/b213f2399038a615cdfbfa0201cddc113d304018
3 years, 8 months ago (2017-04-11 11:37:40 UTC) #100
Michael Achenbach
3 years, 8 months ago (2017-04-11 12:07:13 UTC) #101
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2811103002/ by machenbach@chromium.org.

The reason for reverting is: Breaks noi18n:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20noi18n%20-%2....

Powered by Google App Engine
This is Rietveld 408576698