|
Description
Yusuke Suzuki
2018-11-15 03:02:35 PST
Created attachment 354906 [details]
Patch
Created attachment 354907 [details]
Patch
Comment on attachment 354907 [details] Patch Attachment 354907 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10001803 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html css3/escape-dom-api.html Created attachment 354910 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354907 [details] Patch Attachment 354907 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10001810 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html css3/escape-dom-api.html Created attachment 354911 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 354907 [details] Patch Attachment 354907 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10002068 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html css3/escape-dom-api.html Created attachment 354913 [details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 354907 [details] Patch Attachment 354907 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10002075 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html css3/escape-dom-api.html Created attachment 354915 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 355026 [details]
Patch
Comment on attachment 355026 [details] Patch Attachment 355026 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10014029 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html js/dom/JSON-parse.html Created attachment 355030 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 355026 [details] Patch Attachment 355026 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10014035 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html js/dom/JSON-parse.html Created attachment 355032 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 355026 [details] Patch Attachment 355026 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10014105 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html Created attachment 355033 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 355026 [details] Patch Attachment 355026 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10014133 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html js/dom/JSON-parse.html Created attachment 355034 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 355026 [details] Patch Attachment 355026 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10014819 New failing tests: js/dom/JSON-stringify.html js/dom/webidl-type-mapping.html js/dom/JSON-parse.html Created attachment 355038 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 355191 [details]
Patch
Created attachment 355192 [details]
Patch
Created attachment 355194 [details]
Patch
Ping? Ping? Looks like there's a test262 test for this too, but it's newer than what's currently in the WebKit repo: https://github.com/tc39/test262/blob/master/test/built-ins/JSON/stringify/string-escape-unicode.js (In reply to Ross Kirsling from comment #27) > Looks like there's a test262 test for this too, but it's newer than what's > currently in the WebKit repo: > https://github.com/tc39/test262/blob/master/test/built-ins/JSON/stringify/ > string-escape-unicode.js Yes, and this patch passes this test. Ping? Ping? Informal r+. This seems quite straightforward (and even matches closely with the SpiderMonkey implementation). Ping review? (In reply to Ross Kirsling from comment #31) > Informal r+. > > This seems quite straightforward (and even matches closely with the > SpiderMonkey implementation). Thanks ;) Ping? Comment on attachment 355194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355194&action=review > Source/WTF/wtf/text/StringBuilderJSON.cpp:85 > + uint8_t upper = static_cast<uint32_t>(character) >> 8; I don’t understand what value the static_cast<uint32_t> has here. It seems the code would do the right thing without that. > Source/WTF/wtf/text/StringBuilderJSON.cpp:86 > + uint8_t lower = static_cast<uint8_t>(character); I don’t understand what value the static_cast<uint8_t> has here. It seems the code would do the right thing without that. Comment on attachment 355194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355194&action=review > Source/WTF/wtf/text/StringBuilderJSON.cpp:84 > + if (U16_IS_SURROGATE_TRAIL(character) || next == end || !U16_IS_SURROGATE_TRAIL(*next)) { Oops, this is wrong! Details to come. Comment on attachment 355194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355194&action=review >> Source/WTF/wtf/text/StringBuilderJSON.cpp:84 >> + if (U16_IS_SURROGATE_TRAIL(character) || next == end || !U16_IS_SURROGATE_TRAIL(*next)) { > > Oops, this is wrong! Details to come. This needs to use U16_IS_TRAIL, not U16_IS_SURROGATE_TRAIL, on *next. Test cases would be string where the last two characters are: 1) A surrogate head. 2) A non-surrogate character with the bit 0x400 set. That case would work incorrectly. I also think I would write it more like this to make the logic clearer: bool isValidSurrogatePair = U16_IS_SURROGATE_HEAD(character) && next != end && U16_IS_TRAIL(*next); if (!isValidSurrogatePair) (In reply to Darin Adler from comment #37) > Comment on attachment 355194 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=355194&action=review > > >> Source/WTF/wtf/text/StringBuilderJSON.cpp:84 > >> + if (U16_IS_SURROGATE_TRAIL(character) || next == end || !U16_IS_SURROGATE_TRAIL(*next)) { > > > > Oops, this is wrong! Details to come. > > This needs to use U16_IS_TRAIL, not U16_IS_SURROGATE_TRAIL, on *next. Test > cases would be string where the last two characters are: > > 1) A surrogate head. > 2) A non-surrogate character with the bit 0x400 set. > > That case would work incorrectly. > > I also think I would write it more like this to make the logic clearer: > > bool isValidSurrogatePair = U16_IS_SURROGATE_HEAD(character) && next != > end && U16_IS_TRAIL(*next); > if (!isValidSurrogatePair) Oops, super nice catch! Right. I'll fix this and update the patch soon. Created attachment 357828 [details]
Patch
Committed r239537: <https://trac.webkit.org/changeset/239537> |