Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
836823002:
Encode the Last-Event-ID header in EventSource as UTF-8. (Closed)
|
Created:
5 years, 11 months ago by davidben Modified:
5 years, 3 months ago Reviewers:
Nate Chapin CC:
blink-reviews, blink-reviews-events_chromium.org, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionEncode the Last-Event-ID header in EventSource as UTF-8.
HTTP headers are byte strings, so anything outside of
ASCII needs to be explicitly encoded one way or another.
BUG=429569
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201248
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #
Messages
Total messages: 31 (12 generated)
davidben@chromium.org changed reviewers: + japhet@chromium.org
There's been no response on https://codereview.chromium.org/731773002/, so I'm going to go ahead and do it. I'm not very happy about this "encode as UTF-8 and stuff back into a unicode-aware string" arrangement, but that's the current API for setHTTPHeaderField. It might be better to change the type of setHTTPHeaderField to use some AtomicString version of CString or otherwise get type-checking that we remember to pick an encoding at each call. I dunno. Who would have opinions on that? (HTTP headers don't have a Unicode encoding. Officially they're latin-1. APIs like xhr.setRequestHeader and Fetch use ByteString in WebIDL which takes a JS string and requires all code points be under 256. setHTTPHeaderField behaves the same. Actually, now that we have ByteString support in WebIDL, I should maybe switch XHR to using that...)
https://codereview.chromium.org/836823002/diff/1/Source/core/page/EventSource... File Source/core/page/EventSource.cpp (right): https://codereview.chromium.org/836823002/diff/1/Source/core/page/EventSource... Source/core/page/EventSource.cpp:133: request.setHTTPHeaderField("Last-Event-ID", AtomicString(reinterpret_cast<const LChar*>(lastEventIdUtf8.data()), lastEventIdUtf8.length())); My WTFString-fu is not very good. Can you use AtomicString::fromUTF8() instead of all this verbosity?
https://codereview.chromium.org/836823002/diff/1/Source/core/page/EventSource... File Source/core/page/EventSource.cpp (right): https://codereview.chromium.org/836823002/diff/1/Source/core/page/EventSource... Source/core/page/EventSource.cpp:133: request.setHTTPHeaderField("Last-Event-ID", AtomicString(reinterpret_cast<const LChar*>(lastEventIdUtf8.data()), lastEventIdUtf8.length())); On 2015/01/06 18:23:34, Nate Chapin wrote: > My WTFString-fu is not very good. Can you use AtomicString::fromUTF8() instead > of all this verbosity? I don't think that work. This is an awkward thing where the type AtomicString is arguably wrong for the API (it should be some variant of WTF::CString rather than WTF::String). Say m_lastEventId contains the UTF-16 string "\u2603" (with type uint16_t[]). Encoded as UTF-8, that is the byte string "\xe2\x98\x83" (with type uint8_t[]), so we want to send those three bytes on the wire. All the way at the //net level, headers are just std::string, so we can easily pass in a std::string containing those three bytes. From the entry point in setHTTPHeaderField, all the way through to the Blink API boundary, headers are AtomicString/WTF::String/WebString/etc. (Point is it's the Unicode-aware ones.) They are then extracted as latin-1 or byte strings. https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_... That is, you assume every uint16_t UTF-16 code unit in the WebString is below 256 and get a std::string out of it as uint8_t's. (Let's ignore the silliness where chars can be signed rather than unsigned.) I believe anything 256 or above gets replaced as '?', but there really shouldn't anything above 256 at this range. E.g. input to an XHR gets checked for invalid characters; this is a WebIDL ByteString: https://heycam.github.io/webidl/#idl-ByteString https://xhr.spec.whatwg.org/#interface-xmlhttprequest (And we can't defer the UTF-8 encode until it hits content/child and have headers in Blink internally represented as true UTF-16 strings because xhr.setRequestHeader may pass an input that's not the UTF-8 encoding of some Unicode string.) So, that's why this verbosity and the question of whether we should actually be changing the types of setHTTPHeaderField. Then it might look like request.setHTTPHeaderField("Last-Event-ID", m_lastEventId.utf8()); and we'd have the type system check that every time data is converted from WTF::String to/from a header, we make a decision about encodings. But I don't know enough about Blink's string situation here. There doesn't seem to be an AtomicCString.
Ok, LGTM. Though perhaps we should add a FIXME to find a general solution for this. You could arguably also create a special getter/setter for Last-Event-ID instead of stashing it in the HTTPHeaderMap, but that seems even worse. Maybe if other UTF-8 headers appear, we should have two separate header maps?
Whoops, I totally forgot about this CL. Given the age, I should probably ask for a fresh stamp. I've rebased it and added a TODO about the type-system issue. Shall I email blink-dev perhaps about what to do here?
lgtm again
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836823002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/836823002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836823002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/836823002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836823002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/836823002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836823002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/836823002/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836823002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/836823002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I'm really not having much luck with the CQ apparently... The errors this time all say "Service Unavailable (HTTP 503)" which still doesn't look like me, so I suppose I'll keep trying... :-/
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836823002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/836823002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201248 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
