Bug 218681

Summary: [JSC] Add TimeZone range cache over ICU TimeZone API
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 219915    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch ross.kirsling: review+

Yusuke Suzuki
Reported 2020-11-06 23:17:07 PST
[JSC] Add TimeZone range cache over ICU TimeZone API
Attachments
Patch (15.47 KB, patch)
2020-11-06 23:19 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (15.47 KB, patch)
2020-11-06 23:21 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (15.75 KB, patch)
2020-11-07 19:38 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (15.80 KB, patch)
2020-11-07 20:42 PST, Yusuke Suzuki 		ross.kirsling: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2020-11-06 23:19:48 PST
Created attachment 413519 [details] Patch
Yusuke Suzuki
Comment 2 2020-11-06 23:21:29 PST
Created attachment 413520 [details] Patch
Yusuke Suzuki
Comment 3 2020-11-07 19:38:39 PST
Created attachment 413538 [details] Patch
Yusuke Suzuki
Comment 4 2020-11-07 20:42:01 PST
Created attachment 413540 [details] Patch
Ross Kirsling
Comment 5 2020-11-08 21:16:52 PST
Comment on attachment 413540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413540&action=review r=me, looks good based on the code that existed prior to r269320. > Source/JavaScriptCore/runtime/JSDateMath.cpp:104 > + auto& timeZoneCache = *bitwise_cast<icu::TimeZone*>(this->timeZoneCache()); Is the `this->` necessary? > Source/JavaScriptCore/runtime/JSDateMath.cpp:151 > + if (millisecondsFromEpoch <= newEnd) { nit: You could flip this to be an early out for less nesting if you wanted (ditto below). > Source/JavaScriptCore/runtime/JSDateMath.cpp:166 > + // the point in time where the DST offset change occurred. Updated nit: Updated -> Update (ditto below).
Yusuke Suzuki
Comment 6 2020-11-08 21:25:12 PST
Comment on attachment 413540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413540&action=review Thanks! >> Source/JavaScriptCore/runtime/JSDateMath.cpp:104 >> + auto& timeZoneCache = *bitwise_cast<icu::TimeZone*>(this->timeZoneCache()); > > Is the `this->` necessary? Yes, because `timeZoneCache` name is also used by the variable :) >> Source/JavaScriptCore/runtime/JSDateMath.cpp:151 >> + if (millisecondsFromEpoch <= newEnd) { > > nit: You could flip this to be an early out for less nesting if you wanted (ditto below). Nice. Done >> Source/JavaScriptCore/runtime/JSDateMath.cpp:166 >> + // the point in time where the DST offset change occurred. Updated > > nit: Updated -> Update (ditto below). Nice, fixed.
Yusuke Suzuki
Comment 7 2020-11-08 21:31:40 PST
Committed r269576: <https://trac.webkit.org/changeset/269576>
Radar WebKit Bug Importer
Comment 8 2020-11-08 21:32:21 PST
<rdar://problem/71176359>
WebKit Commit Bot
Comment 9 2020-12-15 13:20:22 PST
Re-opened since this is blocked by bug 219915
Yusuke Suzuki
Comment 10 2020-12-17 15:47:27 PST
The reverted patch is once integrated since we workaround ICU missing API issue differently.
Note You need to log in before you can comment on or make changes to this bug.