HttpServer: Handling of multiple header fields with the same name and multiple values of "Connectio…

HttpServer: Handling of multiple header fields with the same name and multiple values of "Connection".

BUG=370437
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269619

[pfeldman, 2014-05-08 13:38:18]

https://codereview.chromium.org/274813002/diff/1/net/server/http_server.cc
File net/server/http_server.cc (right):

https://codereview.chromium.org/274813002/diff/1/net/server/http_server.cc#ne...
net/server/http_server.cc:146: {
There is no need for this.

https://codereview.chromium.org/274813002/diff/1/net/server/http_server.cc#ne...
net/server/http_server.cc:147: std::string connection_header =
request.GetHeaderValue("connection");
if (request.HasHeaderValue("connection", "upgrade"))

[pfeldman, 2014-05-08 15:19:16]

https://codereview.chromium.org/274813002/diff/20001/net/server/http_server_r...
File net/server/http_server_request_info.cc (right):

https://codereview.chromium.org/274813002/diff/20001/net/server/http_server_r...
net/server/http_server_request_info.cc:36: if (*it == "upgrade")
This should be header_value. Your test below should test this method.

https://codereview.chromium.org/274813002/diff/20001/net/server/http_server_u...
File net/server/http_server_unittest.cc (right):

https://codereview.chromium.org/274813002/diff/20001/net/server/http_server_u...
net/server/http_server_unittest.cc:272: TEST_F(HttpServerTest,
RequestWithDuplicateHeaders) {
Please add the test for HasHeaderValue.

[pfeldman, 2014-05-08 20:10:41]

lgtm.

+mmenke for OWNERS review.

[mmenke, 2014-05-09 15:31:43]

On 2014/05/08 20:10:41, pfeldman wrote:
> lgtm.
> 
> +mmenke for OWNERS review.

Rubberstamp LGTM, but we really should not spend any more time maintaining this
class, should switch to using a TCPServerSocket-based class instead.

[pfeldman, 2014-05-09 17:52:53]

> Rubberstamp LGTM, but we really should not spend any more time maintaining
this
> class, should switch to using a TCPServerSocket-based class instead.

@mmenke: we have enough important code using this code path now, so it might be
good time to invest into doing it the right way.

We basically need an HTTP server that supports subset of HTTP remote debugging
requires. And it needs to upgrade to WebSocket. Could you follow up on where we
should be looking for that?
(I'm not sure I understand how TCPServerSocket is helping with parsing HTTP
headers).

[pfeldman, 2014-05-09 17:55:54]

Or did you mean migrating from TCPListenSocket to TCPServerSocket? That one we
definitely could do, but that won't remove the changes like this from our plate.

[pfeldman, 2014-05-09 17:56:04]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-05-09 18:00:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/274813002/40001

[mmenke, 2014-05-09 18:02:35]

On 2014/05/09 17:55:54, pfeldman wrote:
> Or did you mean migrating from TCPListenSocket to TCPServerSocket? That one we
> definitely could do, but that won't remove the changes like this from our
plate.

Sorry for not being clearer - migrating from TCPListenSocket to TCPServerSocket
is exactly what I meant.

[commit-bot: I haz the power, 2014-05-10 19:20:36]

Change committed as 269619