Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1406313002:
Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer (Closed)
DescriptionAdding Default Handlers from SpawnedTestServer to EmbeddedTestServer.
Implementing default handlers from SpawnedTestServer to allow us to
migrate more tests to EmbeddedTestServer.
BUG=492672
R=bnc@chromium.org,mmenke@chromium.org
Committed: https://crrev.com/7d25c56175e1da831953c0a7eb500f33be051b0f
Cr-Commit-Position: refs/heads/master@{#357162}
Patch Set 1 #Patch Set 2 : Adding call to DefaultHandlers. #Patch Set 3 : Rebase. #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Patch Set 6 : Rebase. #Patch Set 7 : Rebase. #Patch Set 8 : Rebase. #Patch Set 9 : Removing handler used once. #Patch Set 10 : Rebase. #Patch Set 11 : Rebase. #Patch Set 12 : Rebase. #Patch Set 13 : Rebase. #Patch Set 14 : Rebase. #
Total comments: 12
Patch Set 15 : Fixing comments. #Patch Set 16 : Rebase. #
Total comments: 2
Patch Set 17 : Cleanup. #Patch Set 18 : Typo. #
Total comments: 48
Patch Set 19 : Fixing up handlers. #Patch Set 20 : Fixing auth parsing. #
Total comments: 20
Patch Set 21 : Adding comments. #
Dependent Patchsets: Messages
Total messages: 40 (17 generated)
svaldez@chromium.org changed reviewers: + davidben@chromium.org, mmenke@chromium.org
The handlers split out from CL 1376593007.
You should fill the BUG= line with some relevant crbug.
Description was changed from ========== Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer Implementing default handlers from SpawnedTestServer to allow us to migrate more tests to EmbeddedTestServer. BUG= ========== to ========== Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer Implementing default handlers from SpawnedTestServer to allow us to migrate more tests to EmbeddedTestServer. BUG=492672 ==========
mmenke@chromium.org changed reviewers: + bnc@chromium.org
[+bnc]: Do you have the cycles to tackle this? Chris suggested I hand this off to someone else, so semi-randomly picked you, as someone I haven't handed a whole lot of reviews to in the past. If not, I can try someone else. The goal here is to match the behavior of testserver.py. I have done a quick pass over the code, but needs more careful review.
https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:34: #define PREFIXED_HANDLER(prefix, handler) \ Move this down to around line 548, like right before or right after. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:34: #define PREFIXED_HANDLER(prefix, handler) \ And #undef PREFIXED_HANDLER shortly after last use of this macro. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:54: return http_response.Pass(); Please consider removing Pass() from every return statement in this file to allow for return value optimization (unless MSVC gives compilation errors). See https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ICA3hiR4Z3A... for a discussion. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:546: } Add newline above this line. Add two spaces and // namespace anonymous. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.h (right): https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.h:8: #include "net/test/embedded_test_server/embedded_test_server.h" Move this include to .cc and add forward declaration, since only pointer to EmbeddedTestServer is used in this header file.
https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:34: #define PREFIXED_HANDLER(prefix, handler) \ On 2015/10/27 14:47:36, Bence wrote: > And #undef PREFIXED_HANDLER shortly after last use of this macro. Done. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:34: #define PREFIXED_HANDLER(prefix, handler) \ On 2015/10/27 14:47:36, Bence wrote: > Move this down to around line 548, like right before or right after. Done. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:54: return http_response.Pass(); On 2015/10/27 14:47:36, Bence wrote: > Please consider removing Pass() from every return statement in this file to > allow for return value optimization (unless MSVC gives compilation errors). See > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ICA3hiR4Z3A... > for a discussion. There currently needs to be an explicit Pass() since the move constructor optimization doesn't handle subclass casting. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:546: } On 2015/10/27 14:47:36, Bence wrote: > Add newline above this line. Add two spaces and // namespace anonymous. Done. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.h (right): https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.h:8: #include "net/test/embedded_test_server/embedded_test_server.h" On 2015/10/27 14:47:36, Bence wrote: > Move this include to .cc and add forward declaration, since only pointer to > EmbeddedTestServer is used in this header file. We can't forward declare EmbeddedTestServer because of how its namespace is currently defined as part of the migration from net::test_server:: to net::.
LGTM https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:54: return http_response.Pass(); On 2015/10/27 15:00:50, svaldez wrote: > On 2015/10/27 14:47:36, Bence wrote: > > Please consider removing Pass() from every return statement in this file to > > allow for return value optimization (unless MSVC gives compilation errors). > See > > > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ICA3hiR4Z3A... > > for a discussion. > > There currently needs to be an explicit Pass() since the move constructor > optimization doesn't handle subclass casting. Sorry, I didn't realize that. https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.h (right): https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.h:8: #include "net/test/embedded_test_server/embedded_test_server.h" On 2015/10/27 15:00:50, svaldez wrote: > On 2015/10/27 14:47:36, Bence wrote: > > Move this include to .cc and add forward declaration, since only pointer to > > EmbeddedTestServer is used in this header file. > > We can't forward declare EmbeddedTestServer because of how its namespace is > currently defined as part of the migration from net::test_server:: to net::. Sorry, I don't quite understand that. Could you not just forward declare it in the appropriate namespace, and when you move the declaration into a different namespace, move the forward declaration accordingly? Or if it's not possible in this CL, could it be done in the future, in which case it might make sense to add a TODO?
On 2015/10/27 15:13:20, Bence wrote: > LGTM > > https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... > File net/test/embedded_test_server/default_handlers.cc (right): > > https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... > net/test/embedded_test_server/default_handlers.cc:54: return > http_response.Pass(); > On 2015/10/27 15:00:50, svaldez wrote: > > On 2015/10/27 14:47:36, Bence wrote: > > > Please consider removing Pass() from every return statement in this file to > > > allow for return value optimization (unless MSVC gives compilation errors). > > See > > > > > > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ICA3hiR4Z3A... > > > for a discussion. > > > > There currently needs to be an explicit Pass() since the move constructor > > optimization doesn't handle subclass casting. > > Sorry, I didn't realize that. > > https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... > File net/test/embedded_test_server/default_handlers.h (right): > > https://codereview.chromium.org/1406313002/diff/260001/net/test/embedded_test... > net/test/embedded_test_server/default_handlers.h:8: #include > "net/test/embedded_test_server/embedded_test_server.h" > On 2015/10/27 15:00:50, svaldez wrote: > > On 2015/10/27 14:47:36, Bence wrote: > > > Move this include to .cc and add forward declaration, since only pointer to > > > EmbeddedTestServer is used in this header file. > > > > We can't forward declare EmbeddedTestServer because of how its namespace is > > currently defined as part of the migration from net::test_server:: to net::. > > Sorry, I don't quite understand that. Could you not just forward declare it in > the appropriate namespace, and when you move the declaration into a different > namespace, move the forward declaration accordingly? Or if it's not possible in > this CL, could it be done in the future, in which case it might make sense to > add a TODO? We're currently 'forward-declaring' EmbeddedTestServer from the net::test_server::EmbeddedTestServer namespace as net::EmbeddedTestServer so that we can use both net::EmbeddedTestServer and net::test_server::EmbeddedTestServer to refer to it, until the mass CL to migrate all the tests and references to EmbeddedTestServer gets committed. Once that happens, we'll be moving EmbeddedTestServer to actually live in net::EmbeddedTestServer, without needing the "forward-declaration". Unfortunately this means that the compiler is going to complain anytime we try forward declaring EmbeddedTestServer separately, since it won't know how to link it properly. Once we've finished migrating, we'll do another CL to fix up this forward declaration and all the ones from the parent CL.
On 2015/10/27 15:18:57, svaldez wrote: > > We're currently 'forward-declaring' EmbeddedTestServer from the > net::test_server::EmbeddedTestServer namespace as net::EmbeddedTestServer so > that we can use both net::EmbeddedTestServer and > net::test_server::EmbeddedTestServer to refer to it, until the mass CL to > migrate all the tests and references to EmbeddedTestServer gets committed. Once > that happens, we'll be moving EmbeddedTestServer to actually live in > net::EmbeddedTestServer, without needing the "forward-declaration". > Unfortunately this means that the compiler is going to complain anytime we try > forward declaring EmbeddedTestServer separately, since it won't know how to link > it properly. Once we've finished migrating, we'll do another CL to fix up this > forward declaration and all the ones from the parent CL. Thanks for the explanation.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org Link to the patchset: https://codereview.chromium.org/1406313002/#ps300001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406313002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406313002/300001
The CQ bit was unchecked by svaldez@chromium.org
And just so you can land this, LGTM, rubberstamp - deferring to bnc.
After this lands, I will be able to update my CL?
On 2015/10/27 17:39:22, tfarina wrote: > After this lands, I will be able to update my CL? Yes, you can.
The CQ bit was checked by svaldez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406313002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406313002/300001
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
I think a lot of comments Matt did on my CL still stand for this CL (there are style issues yet and probably other worth improvements). https://codereview.chromium.org/1406313002/diff/300001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/300001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:276: std::string expectedPassword = "secret"; expected_password https://codereview.chromium.org/1406313002/diff/300001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:283: scoped_ptr<BasicHttpResponse> http_response(new BasicHttpResponse); this can be moved down below.
The CQ bit was unchecked by tfarina@chromium.org
The CQ bit was unchecked by svaldez@chromium.org
Description was changed from ========== Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer Implementing default handlers from SpawnedTestServer to allow us to migrate more tests to EmbeddedTestServer. BUG=492672 ========== to ========== Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer Implementing default handlers from SpawnedTestServer to allow us to migrate more tests to EmbeddedTestServer. BUG=492672 R=bnc@chromium.org,mmenke@chromium.org ==========
Description was changed from ========== Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer Implementing default handlers from SpawnedTestServer to allow us to migrate more tests to EmbeddedTestServer. BUG=492672 R=bnc@chromium.org,mmenke@chromium.org ========== to ========== Adding Default Handlers from SpawnedTestServer to EmbeddedTestServer. Implementing default handlers from SpawnedTestServer to allow us to migrate more tests to EmbeddedTestServer. BUG=492672 R=bnc@chromium.org,mmenke@chromium.org ==========
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1406313002/#ps340001 (title: "Typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406313002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406313002/340001
The CQ bit was unchecked by mmenke@chromium.org
Didn't quite make it through the entire CL. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:64: std::string cache_control, const std::string& (x2) https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:71: GURL request_gurl = request.GetURL(); Suggest using request_url everywhere in this file rather than request_gurl - calling GURL's url is much more common in net/. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:99: http_response->set_content("Echo"); The old handler just sent an empty body in this case, right? Any reason for the change? https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:125: std::vector<std::string> qs = base::SplitString( Avoid uncommon abbreviations in variable names. Suggest just query or query_strings or query_list or somesuch. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:152: for (size_t i = 0; i < cookies.size(); ++i) { nit: Range loop. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:169: } nit: Remove braces. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:183: std::vector<std::string> sentCookies; sent_cookies - maybe received_cookies would be clearer? https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:189: sentCookies.push_back(cookies[i]); You're copying everything in one vector of std::strings to another. I don't think this is needed? Can just make sent_cookies = base::SplitString(). https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:192: bool got_expected = true; nit: got_all_expected? Think it's a little clearer. Also move it below the blank line, to make it more clearly assocaited with the next code block. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:195: RequestQuery qs = ParseQuery(request_gurl); Avoid uncommon abbreviations in variable names. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:197: std::vector<std::string> expected = qs.at("expect"); expected_cookies? https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:198: for (size_t i = 0; i < expected.size(); ++i) { Suggest range loops (x4). Perf doesn't really matter here, but it's generally assume that size() is more expensive than the alternatives. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:200: for (size_t j = 0; j < sentCookies.size(); ++j) { Could just use std::find, though suppose it doesn't make the code that much cleaner. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:211: std::vector<std::string> setCookies = qs.at("set"); set_cookies https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:239: for (auto header : headers) { const auto& https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:241: std::string value = header.first.substr(header.first.find(": ") + 2); Should be careful about find returning npos. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:297: error = "Invalid Credentials"; Use braces. (Both parts of an if/else should either use or not use braces) https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:319: request.headers.at("If-None-Match") == "abc") { "abc" should probably be a local constant. kEtag or somesuch. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:325: base::FilePath().AppendASCII(request.relative_url.substr(1)); Hrm... This seems kind of silly, but I guess GenerateFileName would also be rather silly. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:330: server_root.AppendASCII("chrome/test/data/google/logo.gif"); Suggest making this path a const char[] at the top of the file (Mostly to indicate this file has weird path dependencies). https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:371: auth, ", ", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); May be a little cleaner to use SplitStringIntoKeyValuePairs. Suppose it affects whitespace handling (And you'd need to use whatever the trim whitespace method is), but AUTH shouldn't care about extra whitespace, anyways. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:372: for (auto authPair : authVector) { const auto& https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:374: std::string value = authPair.substr(authPair.find("=") + 1); Should handle the case authPair.find("=") is npos. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:417: std::string authHeader = base::StringPrintf( auth_header
https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:64: std::string cache_control, On 2015/10/29 20:20:29, mmenke wrote: > const std::string& (x2) Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:71: GURL request_gurl = request.GetURL(); On 2015/10/29 20:20:28, mmenke wrote: > Suggest using request_url everywhere in this file rather than request_gurl - > calling GURL's url is much more common in net/. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:99: http_response->set_content("Echo"); On 2015/10/29 20:20:29, mmenke wrote: > The old handler just sent an empty body in this case, right? Any reason for the > change? The other server had the behavior that it would always fallback to a 200 default response, which we don't do here and as such break tests that rely on some response from "/echo" responses. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:125: std::vector<std::string> qs = base::SplitString( On 2015/10/29 20:20:29, mmenke wrote: > Avoid uncommon abbreviations in variable names. Suggest just query or > query_strings or query_list or somesuch. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:152: for (size_t i = 0; i < cookies.size(); ++i) { On 2015/10/29 20:20:29, mmenke wrote: > nit: Range loop. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:169: } On 2015/10/29 20:20:29, mmenke wrote: > nit: Remove braces. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:183: std::vector<std::string> sentCookies; On 2015/10/29 20:20:29, mmenke wrote: > sent_cookies - maybe received_cookies would be clearer? Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:189: sentCookies.push_back(cookies[i]); On 2015/10/29 20:20:29, mmenke wrote: > You're copying everything in one vector of std::strings to another. I don't > think this is needed? Can just make sent_cookies = base::SplitString(). Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:192: bool got_expected = true; On 2015/10/29 20:20:28, mmenke wrote: > nit: got_all_expected? Think it's a little clearer. Also move it below the > blank line, to make it more clearly assocaited with the next code block. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:195: RequestQuery qs = ParseQuery(request_gurl); On 2015/10/29 20:20:28, mmenke wrote: > Avoid uncommon abbreviations in variable names. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:197: std::vector<std::string> expected = qs.at("expect"); On 2015/10/29 20:20:29, mmenke wrote: > expected_cookies? Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:198: for (size_t i = 0; i < expected.size(); ++i) { On 2015/10/29 20:20:29, mmenke wrote: > Suggest range loops (x4). Perf doesn't really matter here, but it's generally > assume that size() is more expensive than the alternatives. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:200: for (size_t j = 0; j < sentCookies.size(); ++j) { On 2015/10/29 20:20:28, mmenke wrote: > Could just use std::find, though suppose it doesn't make the code that much > cleaner. Acknowledged. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:211: std::vector<std::string> setCookies = qs.at("set"); On 2015/10/29 20:20:28, mmenke wrote: > set_cookies Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:239: for (auto header : headers) { On 2015/10/29 20:20:29, mmenke wrote: > const auto& Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:241: std::string value = header.first.substr(header.first.find(": ") + 2); On 2015/10/29 20:20:29, mmenke wrote: > Should be careful about find returning npos. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:297: error = "Invalid Credentials"; On 2015/10/29 20:20:29, mmenke wrote: > Use braces. (Both parts of an if/else should either use or not use braces) Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:319: request.headers.at("If-None-Match") == "abc") { On 2015/10/29 20:20:29, mmenke wrote: > "abc" should probably be a local constant. kEtag or somesuch. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:325: base::FilePath().AppendASCII(request.relative_url.substr(1)); On 2015/10/29 20:20:28, mmenke wrote: > Hrm... This seems kind of silly, but I guess GenerateFileName would also be > rather silly. Acknowledged. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:330: server_root.AppendASCII("chrome/test/data/google/logo.gif"); On 2015/10/29 20:20:29, mmenke wrote: > Suggest making this path a const char[] at the top of the file (Mostly to > indicate this file has weird path dependencies). Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:371: auth, ", ", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); On 2015/10/29 20:20:29, mmenke wrote: > May be a little cleaner to use SplitStringIntoKeyValuePairs. Suppose it affects > whitespace handling (And you'd need to use whatever the trim whitespace method > is), but AUTH shouldn't care about extra whitespace, anyways. Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:372: for (auto authPair : authVector) { On 2015/10/29 20:20:29, mmenke wrote: > const auto& Done. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:374: std::string value = authPair.substr(authPair.find("=") + 1); On 2015/10/29 20:20:28, mmenke wrote: > Should handle the case authPair.find("=") is npos. Acknowledged. https://codereview.chromium.org/1406313002/diff/340001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:417: std::string authHeader = base::StringPrintf( On 2015/10/29 20:20:29, mmenke wrote: > auth_header Done.
LGTM https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:233: if (header.first.find(": ") == std::string::npos) Suggest just stuffing this in a local. 3 find calls seems a bit much (Same for any other instances of this) https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:372: if (value.substr(0, 1) == "\"" && value.substr(value.size() - 1) == "\"") Just use "at" instead of substr? https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:373: value = value.substr(1, value.size() - 2); Should handle size() <= 1 here and above (Normally not a huge fan of handling incorrect usage that shouldn't happen, but since this is purely for testing other code, and the response bodies are designed for people to actually visually inspect, think it makes sense) https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:464: server->port(), dest_all.substr(dest_all.find("/") + 1).c_str()); Again, should handle "/" not appearing in the string. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:498: class DelayedHttpResponse : public BasicHttpResponse { Think this class is worth a comment. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:500: DelayedHttpResponse(double delay) : delay_(delay) {} explicit https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:510: double delay_; nit: const? https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:510: double delay_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:513: scoped_ptr<HttpResponse> HandleSlowServer(const HttpRequest& request) { Think this method is worth a comment. Should probably have brief descriptions of all of these, actually, rather than rely on consumers reading the code (Hrm...Should probably think about having documentation in some public location, actually, but function comments are fine for now). https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:519: } nit: Remove braces
https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:233: if (header.first.find(": ") == std::string::npos) On 2015/10/30 15:26:35, mmenke wrote: > Suggest just stuffing this in a local. 3 find calls seems a bit much (Same for > any other instances of this) Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:372: if (value.substr(0, 1) == "\"" && value.substr(value.size() - 1) == "\"") On 2015/10/30 15:26:35, mmenke wrote: > Just use "at" instead of substr? Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:373: value = value.substr(1, value.size() - 2); On 2015/10/30 15:26:35, mmenke wrote: > Should handle size() <= 1 here and above (Normally not a huge fan of handling > incorrect usage that shouldn't happen, but since this is purely for testing > other code, and the response bodies are designed for people to actually visually > inspect, think it makes sense) Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:464: server->port(), dest_all.substr(dest_all.find("/") + 1).c_str()); On 2015/10/30 15:26:35, mmenke wrote: > Again, should handle "/" not appearing in the string. Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:498: class DelayedHttpResponse : public BasicHttpResponse { On 2015/10/30 15:26:35, mmenke wrote: > Think this class is worth a comment. Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:500: DelayedHttpResponse(double delay) : delay_(delay) {} On 2015/10/30 15:26:35, mmenke wrote: > explicit Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:510: double delay_; On 2015/10/30 15:26:34, mmenke wrote: > nit: const? Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:510: double delay_; On 2015/10/30 15:26:35, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:513: scoped_ptr<HttpResponse> HandleSlowServer(const HttpRequest& request) { On 2015/10/30 15:26:34, mmenke wrote: > Think this method is worth a comment. Should probably have brief descriptions > of all of these, actually, rather than rely on consumers reading the code > (Hrm...Should probably think about having documentation in some public location, > actually, but function comments are fine for now). Done. https://codereview.chromium.org/1406313002/diff/380001/net/test/embedded_test... net/test/embedded_test_server/default_handlers.cc:519: } On 2015/10/30 15:26:35, mmenke wrote: > nit: Remove braces Done.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1406313002/#ps400001 (title: "Adding comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406313002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406313002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/7d25c56175e1da831953c0a7eb500f33be051b0f Cr-Commit-Position: refs/heads/master@{#357162} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
