Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694923006:
[Embedded Test Server] Enable handler HandleEchoHeader echoing multiple headers (Closed)
|
Created:
3 years, 10 months ago by shenghuazhang Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org, jbudorick, Paweł Hajdan Jr., sgurun-gerrit only Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Embedded Test Server] Enable handler HandleEchoHeader echoing multiple headers
The default handler HandleEchoHeader allows single header value setting in the
http response when calls /echoheader. This CL enables the handler
HandleEchoHeader echoing multiple headers.
BUG=686247
Review-Url: https://codereview.chromium.org/2694923006
Cr-Commit-Position: refs/heads/master@{#450857}
Committed: https://chromium.googlesource.com/chromium/src/+/4672b3b6953050cf8a9fd4ba54bb511c24f16d8c
Patch Set 1 #
Total comments: 3
Patch Set 2 : svaldez comment #Messages
Total messages: 21 (9 generated)
shenghuazhang@chromium.org changed reviewers: + svaldez@chromium.org
This CL enables default handler HandleEchoHeader to echo multiple headers, which I believe is a generic feature to modify. https://codereview.chromium.org/2694923006/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/2694923006/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/default_handlers.cc:92: content += "\n"; I used "\n" to separate the header values here. But not sure if this is a good separator type?
Should set the Vary header once with the comma-concatenated list of headers, and should use ParseQuery instead of manually parsing the query string. I believe the code in the comment is roughly correct, but might need minor tweaks to compile. https://codereview.chromium.org/2694923006/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/2694923006/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/default_handlers.cc:87: std::vector<std::string> header_names = base::SplitString( std::string vary; std::string content; RequestQuery headers = ParseQuery(request_url); for (const auto& header : headers) { std::string header_name = header.first; std::string header_value = "None"; if (request.headers.find(header_name) != request.headers.end()) header_value = request.headers.at(header_name); if (!vary.empty()) vary += ","; vary += header_name; if (!content.empty()) content += "\n"; content += header_value; } http_response->AddCustomHeader("Vary", vary); http_response->set_content(content);
@svaldez thank you for the comments with detailed code! And yeah, actually your code in the comment was exactly correct and compiled successfully :) https://codereview.chromium.org/2694923006/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/2694923006/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/default_handlers.cc:87: std::vector<std::string> header_names = base::SplitString( On 2017/02/14 20:23:43, svaldez wrote: > std::string vary; > std::string content; > RequestQuery headers = ParseQuery(request_url); > for (const auto& header : headers) { > std::string header_name = header.first; > std::string header_value = "None"; > if (request.headers.find(header_name) != request.headers.end()) > header_value = request.headers.at(header_name); > if (!vary.empty()) > vary += ","; > vary += header_name; > if (!content.empty()) > content += "\n"; > content += header_value; > } > > http_response->AddCustomHeader("Vary", vary); > http_response->set_content(content); Done.
shenghuazhang@chromium.org changed reviewers: + phajdan.jr@chromium.org
+phajdan.jr Hi Paweł, can you please have a review on the CL?
LGTM
lgtm
The CQ bit was checked by shenghuazhang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by shenghuazhang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shenghuazhang@chromium.org
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": 1487211181291780,
"parent_rev": "8983a9ebba43f3935f7e1481022f4865e6c5c5bd", "commit_rev":
"4672b3b6953050cf8a9fd4ba54bb511c24f16d8c"}
Message was sent while issue was closed.
Description was changed from ========== [Embedded Test Server] Enable handler HandleEchoHeader echoing multiple headers The default handler HandleEchoHeader allows single header value setting in the http response when calls /echoheader. This CL enables the handler HandleEchoHeader echoing multiple headers. BUG=686247 ========== to ========== [Embedded Test Server] Enable handler HandleEchoHeader echoing multiple headers The default handler HandleEchoHeader allows single header value setting in the http response when calls /echoheader. This CL enables the handler HandleEchoHeader echoing multiple headers. BUG=686247 Review-Url: https://codereview.chromium.org/2694923006 Cr-Commit-Position: refs/heads/master@{#450857} Committed: https://chromium.googlesource.com/chromium/src/+/4672b3b6953050cf8a9fd4ba54bb... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4672b3b6953050cf8a9fd4ba54bb... |
