Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648253004:
Add detection of Shoutcast responses and accept them as valid HTTP/0.9 (Closed)
DescriptionAdd detection of Shoutcast responses and accept them as valid HTTP/0.9
responses over HTTP ports other than port 80, unlike other HTTP/0.9
responses.
BUG=669800
Review-Url: https://codereview.chromium.org/2648253004
Cr-Commit-Position: refs/heads/master@{#446867}
Committed: https://chromium.googlesource.com/chromium/src/+/d78f5290a5585d318081007097b9b2ba3b1cf311
Patch Set 1 #
Total comments: 14
Patch Set 2 : Response to comments #
Messages
Total messages: 18 (13 generated)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mmenke@chromium.org changed reviewers: + eroman@chromium.org
This is what I propose as the long term shoutcast hack, per the intent thread at https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qS63pYso4P0. Not treating this as an HTTP/1.x header (Which FF does) because that isn't previous behavior, and it would encourage use of it in the future by people who want a more feature rich API to use it, though it is unfortunate that it further fragments browser behavior.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:971: base::StringPiece scheme(request_->url.scheme_piece()); [optional] style-nit: use assignment https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:975: // Allow Shoutcast responses over HTTP through, as it's somewhat common nit: delete "through" as it feels redundant with "allow" https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:976: // and relies on HTTP/0.9 on weird ports to work. Can you add a link to an explanation of what the icy headers are and how they are used? (I was unfamiliar with this until reading the discussion on our mailinglist). Could be a bug link, or even the intent to implement thread which explains it well. https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... File net/http/http_stream_parser_unittest.cc (right): https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1217: // the function call. This version worse with inlined C-style strings. worse --> works https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1218: void AddRead(const char* data) { Or could have a single version which takes a base::StringPiece. https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1291: // non-shoutcast response. nit: consistency in writing shoutcast (unittest) vs Shoutcast (non-test code) https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1354: const std::string kShoutcastResponse = "ICY 200 blah\r\n\r\n"; Can you also include a test where icy is the prefix for a longer word? Say, "iCyCastle" Speaking of which, is the decision to test for a prefix (rather than whitespace separated word) for implementation simplicity, or for compatibility?
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:971: base::StringPiece scheme(request_->url.scheme_piece()); On 2017/01/24 00:37:29, eroman (slow) wrote: > [optional] style-nit: use assignment Done. I find I tend to default to this style because it's what I use for unique_ptr https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:975: // Allow Shoutcast responses over HTTP through, as it's somewhat common On 2017/01/24 00:37:29, eroman (slow) wrote: > nit: delete "through" as it feels redundant with "allow" Done. https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:976: // and relies on HTTP/0.9 on weird ports to work. On 2017/01/24 00:37:29, eroman (slow) wrote: > Can you add a link to an explanation of what the icy headers are and how they > are used? (I was unfamiliar with this until reading the discussion on our > mailinglist). > > Could be a bug link, or even the intent to implement thread which explains it > well. Done, link to the intent to implement (Bug has way too much noise) https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... File net/http/http_stream_parser_unittest.cc (right): https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1217: // the function call. This version worse with inlined C-style strings. On 2017/01/24 00:37:29, eroman (slow) wrote: > worse --> works Done. https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1218: void AddRead(const char* data) { On 2017/01/24 00:37:29, eroman (slow) wrote: > Or could have a single version which takes a base::StringPiece. Having a StringPiece that's created for a function call and destroyed immediately after, but that is required to guarantee that the memory isn't freed until the end of the test seems ugly. The AddRead call that takes a std::string has the same issue, but the StringPiece approach seems to even further obscure things, so going to keep the overload. https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1291: // non-shoutcast response. On 2017/01/24 00:37:29, eroman (slow) wrote: > nit: consistency in writing shoutcast (unittest) vs Shoutcast (non-test code) Done. https://codereview.chromium.org/2648253004/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:1354: const std::string kShoutcastResponse = "ICY 200 blah\r\n\r\n"; On 2017/01/24 00:37:29, eroman (slow) wrote: > Can you also include a test where icy is the prefix for a longer word? > > Say, "iCyCastle" > > Speaking of which, is the decision to test for a prefix (rather than whitespace > separated word) for implementation simplicity, or for compatibility? Test added (Though I went with iCyCreamSundae, a very important distinction). I did it for compatibility. FireFox treats it like "HTTP". Considered checking for a "/" or whitespace, but then I looked at our HTTP treatment, and all we require is that a response start with "HTTP", regardless of what follows it. Admittedly, FireFox may be pickier there than we are in the HTTP case, not sure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2648253004/#ps20001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485560858365860,
"parent_rev": "ea757161c1067010f73c9ab922d867a6648b202f", "commit_rev":
"d78f5290a5585d318081007097b9b2ba3b1cf311"}
Message was sent while issue was closed.
Description was changed from ========== Add detection of Shoutcast responses and accept them as valid HTTP/0.9 responses over HTTP ports other than port 80, unlike other HTTP/0.9 responses. BUG=669800 ========== to ========== Add detection of Shoutcast responses and accept them as valid HTTP/0.9 responses over HTTP ports other than port 80, unlike other HTTP/0.9 responses. BUG=669800 Review-Url: https://codereview.chromium.org/2648253004 Cr-Commit-Position: refs/heads/master@{#446867} Committed: https://chromium.googlesource.com/chromium/src/+/d78f5290a5585d318081007097b9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d78f5290a5585d318081007097b9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
