Summary: | [JSC] Add TimeZone range cache over ICU TimeZone API | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||
Component: | New Bugs | Assignee: | 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
Yusuke Suzuki
2020-11-06 23:17:07 PST
Created attachment 413519 [details]
Patch
Created attachment 413520 [details]
Patch
Created attachment 413538 [details]
Patch
Created attachment 413540 [details]
Patch
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). 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. Committed r269576: <https://trac.webkit.org/changeset/269576> Re-opened since this is blocked by bug 219915 The reverted patch is once integrated since we workaround ICU missing API issue differently. |