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

Issue 22875053: Do not normalize into NFC the data sent by XMLHttpRequest (Closed)

Created:
7 years, 3 months ago by dtrebbien
Modified:
7 years, 3 months ago
Reviewers:
tkent, abarth-chromium
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Do not normalize into NFC the data sent by XMLHttpRequest This is a follow-up to http://crbug.com/117128, the patch for which (https://src.chromium.org/viewvc/blink?view=rev&revision=154881) introduced the TextEncoding::normalizeAndEncode API method for when normalization into Unicode NFC is desired in addition to encoding. If normalization is not desired, then the TextEncoding::encode API can be used instead. As reported by David Benjamin, XHR request data was being normalized. This causes issues when the data sent is in JSON format, and a string literal has a JSON newline escape sequence "\n" followed by a Unicode combining char. Before this change, the combining char was combined with the 'n', leading to invalid JSON like "\วน". The following browsers were tested and found not to normalize the XHR request data: - IE 7 - IE 8 - IE 9 - IE 10 - IE 11 Preview - Firefox 3.6.28 - Firefox 23.0.1 - Opera 12.16 Three new tests are added: - request-encoding3.html: Test that passing a string to the send() method does not result in the string being normalized. - request-encoding4.html: Test that passing a document to the send() method does not result in the serialized data being normalized. - response-encoding2.html: Test that XHR response text is not normalized. BUG=277526 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156678

Patch Set 1 #

Messages

Total messages: 4 (0 generated)
dtrebbien
7 years, 3 months ago (2013-08-24 21:20:53 UTC) #1
abarth-chromium
Sounds like this change makes us more compatible with other browsers. LGTM.
7 years, 3 months ago (2013-08-24 23:34:24 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/22875053/1
7 years, 3 months ago (2013-08-25 21:14:49 UTC) #3
commit-bot: I haz the power
7 years, 3 months ago (2013-08-25 22:21:11 UTC) #4
Message was sent while issue was closed.
Change committed as 156678

Powered by Google App Engine
This is Rietveld 408576698