Summary: | stress/intl-datetimeformat-formatrange.js and stress/intl-datetimeformat-formatrange-relevant-extensions.js fail with ICU 65.1 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||
Component: | JavaScriptCore | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | mcatanzaro, ross.kirsling, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2020-11-10 11:25:05 PST
Created attachment 413718 [details]
Patch
Comment on attachment 413718 [details]
Patch
r=me
Committed r269667: <https://trac.webkit.org/changeset/269667> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413718 [details]. Reopening since these tests are broken again after bug #213822. I'll look closer and try to come up with something next week. (In reply to Michael Catanzaro from comment #5) > next week. Ah well, better late than never! Created attachment 415561 [details]
Patch
Comment on attachment 415561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415561&action=review > JSTests/stress/intl-datetimeformat-formatrange-relevant-extensions.js:51 > - shouldBe(fmt1.formatRange(date1, date2), `ä¸/ä¸ã/ãä¸, äº:ãã${range}ä¸:ãã AM`); > - shouldBe(fmt1.formatRange(date1, date3), `ä¸/ä¸ã/ãä¸, äº:ãã AM${range}ä¸/äºã/ãä¸, äº:ãã AM`); > + shouldBe(fmt1.formatRange(date1, date2), `ä¸/ä¸ã/ãä¸, äº:ãã â ä¸:ãã AM`); > + shouldBe(fmt1.formatRange(date1, date3), `ä¸/ä¸ã/ãä¸, äº:ãã AM â ä¸/äºã/ãä¸, äº:ãã AM`); Of course these diffs are unreadable in the Bugzilla review tool, since it doesn't use UTF-8.... Comment on attachment 415561 [details]
Patch
jsc EWS is unhappy. I'll need to look over its results.
Created attachment 415592 [details]
Patch
Comment on attachment 415592 [details]
Patch
r=me
Comment on attachment 415592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415592&action=review > JSTests/stress/intl-datetimeformat-formatrange-relevant-extensions-ja.js:5 > + if (Object.prototype.toString.call(actual) === '[object String]') We can just do, `typeof actual === 'string'`. Thanks for reviewing. (In reply to Yusuke Suzuki from comment #12) > We can just do, `typeof actual === 'string'`. I think it will work in this case since everything is a string literal or template, but StackOverflow says to avoid it since it has a tendency to return garbage results: see https://stackoverflow.com/a/7894320/1120203 linking to http://bonsaiden.github.io/JavaScript-Garden/#types. Do you want me to try changing this anyway? (In reply to Michael Catanzaro from comment #13) > Thanks for reviewing. > > (In reply to Yusuke Suzuki from comment #12) > > We can just do, `typeof actual === 'string'`. > > I think it will work in this case since everything is a string literal or > template, but StackOverflow says to avoid it since it has a tendency to > return garbage results: see https://stackoverflow.com/a/7894320/1120203 > linking to http://bonsaiden.github.io/JavaScript-Garden/#types. > > Do you want me to try changing this anyway? Yes, I think `typeof` is better. `Object.prototype.toString.call(actual)` returns `"[object String]"` when, 1. The input is actually String. Then "ToObject" abstract operation will be performed and it is converted to String object. Then, `Object.prototype.toString` returns "[object String]" 2. The input is weird String object (new String("Hello")). Then it just returns "[object String]" On the other hand, `typeof actual` returns "string" only when `actual` is actually string primitive type. It returns "object" when the input is String object. In this case, we will never pass String object. If String object appears, rather, this is a bug. The stack overflow entry attempts to find what class the given object is. `typeof` is not appropriate for that purpose. But here, what we would like to know is whether the input is string primitive type (not object), not a class of object. So `typeof` is the best. OK, 'typeof' it is. I don't pretend to understand JavaScript, anyway. :D Created attachment 415798 [details]
Patch
Committed r270608: <https://trac.webkit.org/changeset/270608> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415798 [details]. |