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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 251213004: HttpServer: avoid DCHECK'ing on non-HTTP/1.1 requests. (Closed)

Created:
6 years, 7 months ago by gunsch
Modified:
6 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

HttpServer: avoid DCHECK'ing on non-HTTP/1.1 requests. BUG=b/14249697 R=mef@chromium.org,byungchul@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267066

Patch Set 1 #

Total comments: 6

Patch Set 2 : review round 1 #

Total comments: 2

Patch Set 3 : rm spurious nl #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -58 lines) Patch
M net/http/http_response_headers.h View 1 chunk +0 lines, -8 lines 0 comments Download
M net/http/http_response_headers.cc View 1 2 2 chunks +7 lines, -48 lines 0 comments Download
M net/http/http_util.h View 1 chunk +6 lines, -0 lines 0 comments Download
M net/http/http_util.cc View 1 chunk +42 lines, -0 lines 0 comments Download
M net/server/http_server.cc View 2 chunks +5 lines, -2 lines 0 comments Download
M net/server/http_server_request_info.h View 2 chunks +4 lines, -0 lines 0 comments Download
M net/server/http_server_unittest.cc View 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
gunsch
6 years, 7 months ago (2014-04-28 23:31:35 UTC) #1
byungchul
https://codereview.chromium.org/251213004/diff/1/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/251213004/diff/1/net/http/http_response_headers.cc#newcode638 net/http/http_response_headers.cc:638: std::string version = std::string(line_begin, first_space); std::string version(line_begin, first_space); https://codereview.chromium.org/251213004/diff/1/net/server/http_server_unittest.cc ...
6 years, 7 months ago (2014-04-28 23:57:09 UTC) #2
gunsch
https://codereview.chromium.org/251213004/diff/1/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/251213004/diff/1/net/http/http_response_headers.cc#newcode638 net/http/http_response_headers.cc:638: std::string version = std::string(line_begin, first_space); On 2014/04/28 23:57:10, byungchul ...
6 years, 7 months ago (2014-04-29 00:02:31 UTC) #3
byungchul
lgtm
6 years, 7 months ago (2014-04-29 00:18:02 UTC) #4
mef
lgtm https://codereview.chromium.org/251213004/diff/20001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/251213004/diff/20001/net/http/http_response_headers.cc#newcode634 net/http/http_response_headers.cc:634: nit: spurious nl
6 years, 7 months ago (2014-04-29 15:43:26 UTC) #5
gunsch
The CQ bit was checked by gunsch@chromium.org
6 years, 7 months ago (2014-04-29 16:04:43 UTC) #6
gunsch
https://codereview.chromium.org/251213004/diff/20001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/251213004/diff/20001/net/http/http_response_headers.cc#newcode634 net/http/http_response_headers.cc:634: On 2014/04/29 15:43:26, mef wrote: > nit: spurious nl ...
6 years, 7 months ago (2014-04-29 16:05:06 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gunsch@chromium.org/251213004/40001
6 years, 7 months ago (2014-04-29 16:05:09 UTC) #8
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-04-29 16:33:27 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
6 years, 7 months ago (2014-04-29 16:33:28 UTC) #10
gunsch-google
The CQ bit was checked by gunsch@google.com
6 years, 7 months ago (2014-04-29 16:39:45 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gunsch@chromium.org/251213004/40001
6 years, 7 months ago (2014-04-29 16:40:13 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-04-29 22:01:11 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
6 years, 7 months ago (2014-04-29 22:01:12 UTC) #14
gunsch
The CQ bit was checked by gunsch@chromium.org
6 years, 7 months ago (2014-04-29 23:08:37 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gunsch@chromium.org/251213004/40001
6 years, 7 months ago (2014-04-29 23:10:13 UTC) #16
commit-bot: I haz the power
Change committed as 267066
6 years, 7 months ago (2014-04-30 04:49:34 UTC) #17
henrika (OOO until Aug 14)
A revert of this CL has been created in https://codereview.chromium.org/265603002/ by henrika@chromium.org. The reason for ...
6 years, 7 months ago (2014-04-30 08:54:57 UTC) #18
Ryan Sleevi
6 years, 7 months ago (2014-05-07 22:00:32 UTC) #19
Message was sent while issue was closed. 
Post-review comments:
When landing Chromium changes, please file a public (Chromium) bug so that the
community can know why a given change was made.

Having reviewed both the bug and this CL, it is still unclear to me *why* this
change is necessary/good, which is an important part of a CL.

Powered by Google App Engine
This is Rietveld 408576698