Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1376593007:
SSL in EmbeddedTestServer (Closed)
DescriptionSSL and add handlers in EmbeddedTestServer
As part of the migration of tests to EmbeddedTestServer, this CL modifies
EmbeddedTestServer to support SSLServerSocket and to add shared default handlers
that can be used by tests to mirror the SpawnedTestServer handlers. The major
changes are:
- SSL support through SSLServerSocket
- Adding SSLServerConfig to configure SSLServerSockets.
- Setting up default handlers for BrowserTestBase tests.
- Asynchronous HttpResponse.
- Moving EmbeddedTestServer to the net:: namespace.
BUG=496825
NOPRESUBMIT=TRUE
Committed: https://crrev.com/5c265faa70626b7754e8b62300f65771dae2adbf
Cr-Commit-Position: refs/heads/master@{#356305}
Patch Set 1 #Patch Set 2 : Adding missing comment fixes. #Patch Set 3 : Fixing SSLConfig reference. #Patch Set 4 : Removing requirement for RequestHandlers to be ordered. #Patch Set 5 : Fixing NSSHttpIO crash. #Patch Set 6 : Fix typo in ShutdownNSSHttpIO #Patch Set 7 : More fixes for NSSHttpIO and fixed Range issue. #Patch Set 8 : Split Request Handlers to match assumptions about ordering. #Patch Set 9 : Removing Range and adding Certificate accessors. #Patch Set 10 : Adding Range support. #Patch Set 11 : Adding async request handlers. #Patch Set 12 : Fixing cronet HttpResponse. #Patch Set 13 : Add spawned_test_server() method. #Patch Set 14 : Modifying SendResponse to allow for multiple writes. #Patch Set 15 : Rebase and fixing auth. #Patch Set 16 : Fixing final handlers for mass test conversion. #Patch Set 17 : Rebase. #
Total comments: 67
Patch Set 18 : Rebase. #
Total comments: 5
Patch Set 19 : Fixing based on comments. #Patch Set 20 : Fix default constructor to use HTTP. #Patch Set 21 : Fix ordering of initializer list. #Patch Set 22 : Fixing SSL constructor in ETS. #Patch Set 23 : Fixing auth handlers. #Patch Set 24 : More cleanup. #
Total comments: 54
Patch Set 25 : Fixing comments. #Patch Set 26 : Cleaning up request handler. #
Total comments: 82
Patch Set 27 : Fixing from comments. #Patch Set 28 : Fixing range #Patch Set 29 : Splitting out handlers. #Patch Set 30 : Removing unused macros. #
Total comments: 87
Patch Set 31 : Fixing comments and adding canceled connection test. #Patch Set 32 : Rebase. #Patch Set 33 : Fix handler for unittest. #Patch Set 34 : Preventing post-Start RegisterHandlers. #Patch Set 35 : Adding TODO to fix handlers post-Start. #
Total comments: 68
Patch Set 36 : Fixing up CL. #Patch Set 37 : Checking return from ParseRangeHeader. #Patch Set 38 : Fixing RunLoop and check. #
Total comments: 28
Patch Set 39 : Moving CustomHttpResponse and other comments. #Patch Set 40 : Fix syntax. #Patch Set 41 : Adding cert check unittest. #
Total comments: 8
Patch Set 42 : Fix formatting. #Patch Set 43 : Fixing checks. #Patch Set 44 : Logging. #Patch Set 45 : More logging. #Patch Set 46 : More logging for FilePath issues on Windows. #Patch Set 47 : Using as_string. #Patch Set 48 : Suppress extra logging. #Patch Set 49 : More syntax for Windows. #Patch Set 50 : Another typo. #Patch Set 51 : Skipping BAD_VALIDITY cert on Windows. #Patch Set 52 : Moving ServerCertificate out of SSLServerConfig. #Patch Set 53 : Fixing default cert_ value. #
Total comments: 12
Patch Set 54 : Last few nits. #Patch Set 55 : Typo fix. #Dependent Patchsets: Messages
Total messages: 82 (17 generated)
svaldez@chromium.org changed reviewers: + davidben@chromium.org
svaldez@chromium.org changed required reviewers: + davidben@chromium.org
More Code Reviews!
davidben@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke who probably also should look over the test server portions of this. So, this CL prepares a bunch of things, but the commit message only mentions some of them. I see: - SSL support - Client cert knob in SSLServerSocket - Some notion of "default" request handlers which is used to implement... - A bunch of default request handlers (in request_helpers.cc) - Always set up the default request handlers - Ability for HttpResponse to be async - Namespace thing - Empty Content-Type / Content-Length checks - spawned_test_server accessor in test harnesses Whatever ends up in this commit should also get listed in the commit message, preferably with some motivation behind it especially since we're landing a bunch of unused stuff. I did a first pass over it, though I didn't look at request_helpers.cc very carefully yet. https://codereview.chromium.org/1376593007/diff/320001/content/public/test/co... File content/public/test/content_browser_test.cc (left): https://codereview.chromium.org/1376593007/diff/320001/content/public/test/co... content/public/test/content_browser_test.cc:54: embedded_test_server()->ServeFilesFromDirectory(content_test_data_absolute); This is slightly different behavior than before. I think we do actually prefer giving it an absolute path? Not sure. I had people complain at me before that prerender browsertests were sensitive to cwd. (The spawned test server is weird and mangles paths internally which I think is why it wants a relative one.) https://codereview.chromium.org/1376593007/diff/320001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:619: SSL_CTX_set_verify(ssl_ctx.get(), SSL_VERIFY_PEER, NULL); So, I don't see any tests that currently use this. Are you sure this does what you want? I believe SSL_VERIFY_PEER, without also using SSL_CTX_set_cert_verify_callback will make verification failures fatal. It also still allows no certificates without SSL_VERIFY_FAIL_IF_NO_PEER_CERT. See https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#C... Although that documentation is just from me reading the source. Cast will also eventually want to actually verify the certificate, so we'll probably need to route in some ClientCertVerifier interface eventually. But for now it can just blindly accept all certs. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:14: // A collection of SSLServer-related configuration settings. Maybe change "SSLServer-related" to "server-side SSL-related" https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:26: CERT_CHAIN_WRONG_ROOT, Document what these certificates are. (It might be worth pulling the enum and some of the lookup functions into their file somewhere in net/test, shared between the two servers. Then we only document once.) https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:30: // (Use the SSL_PROTOCOL_VERSION_xxx enumerators defined above.) above? :-) https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:33: uint16 version_min; Nit: new code should use uint16_t and friends from <stdint.h> https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:62: // Requires a Client Cert for Client Auth. Nit: Client Cert -> client certificate Client Auth -> client authentication https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:63: bool require_client_cert; Should mention this requires any client certificate but doesn't enforce anything about it. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:66: ServerCertificate server_cert; This enum isn't used by SSLServerSocket, so it should be in the test server's config. I would suggest that either EmbeddedTestServer gets its own config struct that then converts to whatever SSLServerSocket uses, or that you pass the enum into EmbeddedTestServer as a separate parameter. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:94: base_url_ = GURL("http://" + local_endpoint_.ToString()); Nit: I'd probably put this in an else. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:144: LOG(WARNING) << "Request incoming: " << request->relative_url; Probably didn't mean to keep this line. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:276: DCHECK(base::ReadFileToString(key_path, &key_string)); If wrapped in a DCHECK, release builds won't even run this code. Could make this return nullptr, or maybe return int and take a scoped_ptr<StreamSocket>* output pointer. That or maybe a CHECK is fine since this is all test code. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:279: PEMTokenizer pem_tok(key_string, headers); Nit: pem_tokenizer https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:281: std::vector<uint8> key_vector; Nit: uint8_t for new code https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:312: } Nit: No curlies https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:322: } Nit: No curlies https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:331: (SSLServerSocket*)http_connection->socket_.get(); static_cast<SSLServerSocket*>(....) https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:336: OnHandshakeDone(http_connection, rv); Nit: no curlies https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:92: explicit EmbeddedTestServer(bool use_ssl = false); This should be an enum of some sort. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:152: bool UsesSSL() const { return use_ssl_; } Nit: use_ssl() https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:154: void SetSSLConfig(net::SSLServerConfig ssl_config) { Nit: set_ssl_config() https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:156: } The spawned test server makes this a constructor parameter, which avoids weirdness around what happens if you call this after Start or something. Granted, you can add handlers to this API after Start anyway, but any reason you did it that way? https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:174: // Registers the default handlers from request_helpers.h. What is directory here? https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:181: void RegisterDefaultHandler(const HandleRequestCallback& callback); What does RegisterDefaultHandler do? (Document.) At a glance, it looks like the only difference is that they always happen after the normal ones? I wonder if it'd be better to have a way to prepend to the list. Or perhaps if the tests didn't clash on URL patterns. Which tests is this needed for? https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:121: // SSLClientSocket. See https://crbug.com/539520. Nit: If you put this inside the #ifdef, hopefully git cl format won't do something dumb with it. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:104: http_request_->relative_url = url.path(); Nit: curlies (if not having curlies and else having it looks really confusing) https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:154: LOG(WARNING) << "Malformed Content-Length header's value."; Why this change? https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_request.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_request.h:30: METHOD_CONNECT, CONNECT is actually super-magical since you turn the underlying socket into a stream. Are there tests that require this? Edit: I see. URLRequestTestHTTP.ProxyTunnerRedirectTest. I wonder if this is worth a "Note: Although request handlers may respond to CONNECT requests, tunneling is not implemented. This may only be used to test behavior against failed CONNECTs." https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_response.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.cc:37: if (content_type_.size() != 0 || content_.size() != 0) { This is to avoid sending empty headers in some cases I'm guessing? Do our existing tests do this or was there some other reason? (Commit message doesn't say anything about this.) https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:21: typedef base::Callback<void(void)> SendDoneCallback; Nit: using SendDoneCallback = base::Callback<void(void)>; for new code https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:34: // it should start with "HTTP/x.x" line, followed by response headers. Should add "May call |send| reentrantly." or something. Perhaps also add "If the HttpResponse is destroyed before the operation completes, the callback will not be called." https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:35: virtual void SendResponse(SendCallback send, SendDoneCallback done) = 0; Why do you need both callbacks? It looks like SendDoneCallback is only ever passed into SendCallback, right? So you could just bind it when calling SendResponse. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:35: virtual void SendResponse(SendCallback send, SendDoneCallback done) = 0; Nit: I'd probably call this GetResponse or something about putting the response to a string. As far as the HttpResponse is concerned, it's just asynchronously providing a string. It's the EmbeddedTestServer and stuff that decides to send it. https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED scoped_ptr<HttpResponse>() Just return nullptr. nullptr has a distinct type (nullptr_t) so scoped_ptr (and std::unique_ptr) can handle it.
https://codereview.chromium.org/1376593007/diff/320001/content/public/test/co... File content/public/test/content_browser_test.cc (left): https://codereview.chromium.org/1376593007/diff/320001/content/public/test/co... content/public/test/content_browser_test.cc:54: embedded_test_server()->ServeFilesFromDirectory(content_test_data_absolute); On 2015/10/13 19:43:47, David Benjamin wrote: > This is slightly different behavior than before. I think we do actually prefer > giving it an absolute path? Not sure. I had people complain at me before that > prerender browsertests were sensitive to cwd. > > (The spawned test server is weird and mangles paths internally which I think is > why it wants a relative one.) Hrm....I don't think we want every test calling PathService::Get(base::DIR_SOURCE_ROOT, &content_test_data_absolute), so we should probably stick with the old behavior. https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED scoped_ptr<HttpResponse>() I'm not sure all of these really belong in this CL - maybe just switch over some simpler tests that don't use all the fanciness to the embedded test server with SSL, to make sure it works? Also wondering if we can break up landing this file into chunks. 1000-line reviews often don't go well. Bugs missed, relanding/re-review is a pain, etc.
I was planning on updating the CL description near the end since I'm still moving a few changes from the massive CL over here, though since that's basically complete now, I can update it shortly. https://codereview.chromium.org/1376593007/diff/320001/content/public/test/co... File content/public/test/content_browser_test.cc (left): https://codereview.chromium.org/1376593007/diff/320001/content/public/test/co... content/public/test/content_browser_test.cc:54: embedded_test_server()->ServeFilesFromDirectory(content_test_data_absolute); On 2015/10/13 19:43:47, David Benjamin wrote: > This is slightly different behavior than before. I think we do actually prefer > giving it an absolute path? Not sure. I had people complain at me before that > prerender browsertests were sensitive to cwd. > > (The spawned test server is weird and mangles paths internally which I think is > why it wants a relative one.) The default handler serves files based on DIR_SOURCE_ROOT so it should be the same behavior. https://codereview.chromium.org/1376593007/diff/320001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:619: SSL_CTX_set_verify(ssl_ctx.get(), SSL_VERIFY_PEER, NULL); On 2015/10/13 19:43:47, David Benjamin wrote: > So, I don't see any tests that currently use this. Are you sure this does what > you want? I believe SSL_VERIFY_PEER, without also using > SSL_CTX_set_cert_verify_callback will make verification failures fatal. It also > still allows no certificates without SSL_VERIFY_FAIL_IF_NO_PEER_CERT. > > See > https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#C... > Although that documentation is just from me reading the source. > > Cast will also eventually want to actually verify the certificate, so we'll > probably need to route in some ClientCertVerifier interface eventually. But for > now it can just blindly accept all certs. This is for the tests that check to see if the server requests a client cert. The mass ETS migration hasn't migrated the tests that actually complete the Client Cert procedures, since that deals with enough SSL internals that I decided to put it off until the main migration has been completed. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:14: // A collection of SSLServer-related configuration settings. On 2015/10/13 19:43:47, David Benjamin wrote: > Maybe change "SSLServer-related" to "server-side SSL-related" Done. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:26: CERT_CHAIN_WRONG_ROOT, On 2015/10/13 19:43:47, David Benjamin wrote: > Document what these certificates are. (It might be worth pulling the enum and > some of the lookup functions into their file somewhere in net/test, shared > between the two servers. Then we only document once.) Done. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:30: // (Use the SSL_PROTOCOL_VERSION_xxx enumerators defined above.) On 2015/10/13 19:43:47, David Benjamin wrote: > above? :-) Done. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:33: uint16 version_min; On 2015/10/13 19:43:47, David Benjamin wrote: > Nit: new code should use uint16_t and friends from <stdint.h> Done. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:62: // Requires a Client Cert for Client Auth. On 2015/10/13 19:43:47, David Benjamin wrote: > Nit: Client Cert -> client certificate > Client Auth -> client authentication Done. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:63: bool require_client_cert; On 2015/10/13 19:43:47, David Benjamin wrote: > Should mention this requires any client certificate but doesn't enforce anything > about it. Done. https://codereview.chromium.org/1376593007/diff/320001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:66: ServerCertificate server_cert; On 2015/10/13 19:43:47, David Benjamin wrote: > This enum isn't used by SSLServerSocket, so it should be in the test server's > config. I would suggest that either EmbeddedTestServer gets its own config > struct that then converts to whatever SSLServerSocket uses, or that you pass the > enum into EmbeddedTestServer as a separate parameter. This seems to be fairly closely connected to the SSL configuration, and will end up needing to be here when we modify the server socket to deal with OCSP. It also seems like having an additional configuration structure for ETS is redundant, especially since we want to eventually reach a point where SpawnedTestServer can also share this structure. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:94: base_url_ = GURL("http://" + local_endpoint_.ToString()); On 2015/10/13 19:43:47, David Benjamin wrote: > Nit: I'd probably put this in an else. Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:144: LOG(WARNING) << "Request incoming: " << request->relative_url; On 2015/10/13 19:43:48, David Benjamin wrote: > Probably didn't mean to keep this line. Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:276: DCHECK(base::ReadFileToString(key_path, &key_string)); On 2015/10/13 19:43:48, David Benjamin wrote: > If wrapped in a DCHECK, release builds won't even run this code. Could make this > return nullptr, or maybe return int and take a scoped_ptr<StreamSocket>* output > pointer. That or maybe a CHECK is fine since this is all test code. Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:279: PEMTokenizer pem_tok(key_string, headers); On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: pem_tokenizer Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:281: std::vector<uint8> key_vector; On 2015/10/13 19:43:47, David Benjamin wrote: > Nit: uint8_t for new code Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:312: } On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: No curlies Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:322: } On 2015/10/13 19:43:47, David Benjamin wrote: > Nit: No curlies Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:331: (SSLServerSocket*)http_connection->socket_.get(); On 2015/10/13 19:43:48, David Benjamin wrote: > static_cast<SSLServerSocket*>(....) Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:336: OnHandshakeDone(http_connection, rv); On 2015/10/13 19:43:47, David Benjamin wrote: > Nit: no curlies Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:92: explicit EmbeddedTestServer(bool use_ssl = false); On 2015/10/13 19:43:48, David Benjamin wrote: > This should be an enum of some sort. Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:152: bool UsesSSL() const { return use_ssl_; } On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: use_ssl() Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:154: void SetSSLConfig(net::SSLServerConfig ssl_config) { On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: set_ssl_config() Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:156: } On 2015/10/13 19:43:48, David Benjamin wrote: > The spawned test server makes this a constructor parameter, which avoids > weirdness around what happens if you call this after Start or something. > Granted, you can add handlers to this API after Start anyway, but any reason you > did it that way? > > https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... It seemed neater. This also allows you to reuse a server in some cases by changing the SSLServerConfig params, since only server_cert is used pre-accept. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:174: // Registers the default handlers from request_helpers.h. On 2015/10/13 19:43:48, David Benjamin wrote: > What is directory here? Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:181: void RegisterDefaultHandler(const HandleRequestCallback& callback); On 2015/10/13 19:43:48, David Benjamin wrote: > What does RegisterDefaultHandler do? (Document.) > > At a glance, it looks like the only difference is that they always happen after > the normal ones? I wonder if it'd be better to have a way to prepend to the > list. Or perhaps if the tests didn't clash on URL patterns. Which tests is this > needed for? Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:121: // SSLClientSocket. See https://crbug.com/539520. On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: If you put this inside the #ifdef, hopefully git cl format won't do > something dumb with it. Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:104: http_request_->relative_url = url.path(); On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: curlies (if not having curlies and else having it looks really confusing) Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:154: LOG(WARNING) << "Malformed Content-Length header's value."; On 2015/10/13 19:43:48, David Benjamin wrote: > Why this change? "Content-Length: " turns out to be a valid header in some cases... https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_request.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_request.h:30: METHOD_CONNECT, On 2015/10/13 19:43:48, David Benjamin wrote: > CONNECT is actually super-magical since you turn the underlying socket into a > stream. Are there tests that require this? > > Edit: I see. URLRequestTestHTTP.ProxyTunnerRedirectTest. I wonder if this is > worth a "Note: Although request handlers may respond to CONNECT requests, > tunneling is not implemented. This may only be used to test behavior against > failed CONNECTs." Don't know if a note is necessary, we already have a bunch of other partially implemented things in ETS making it definitely not compatible with general use. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_response.cc (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.cc:37: if (content_type_.size() != 0 || content_.size() != 0) { On 2015/10/13 19:43:48, David Benjamin wrote: > This is to avoid sending empty headers in some cases I'm guessing? Do our > existing tests do this or was there some other reason? (Commit message doesn't > say anything about this.) This is for cases where we don't actually have a body (HEAD) and the tests expect that we don't return a Content-Length. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:21: typedef base::Callback<void(void)> SendDoneCallback; On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: using SendDoneCallback = base::Callback<void(void)>; for new code Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:34: // it should start with "HTTP/x.x" line, followed by response headers. On 2015/10/13 19:43:48, David Benjamin wrote: > Should add "May call |send| reentrantly." or something. Perhaps also add "If the > HttpResponse is destroyed before the operation completes, the callback will not > be called." Done. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:35: virtual void SendResponse(SendCallback send, SendDoneCallback done) = 0; On 2015/10/13 19:43:48, David Benjamin wrote: > Why do you need both callbacks? It looks like SendDoneCallback is only ever > passed into SendCallback, right? So you could just bind it when calling > SendResponse. Some subclasses (not implemented yet) will need to be able to do SendCallback multiple times, without SendDoneCallback being called until the final one. Notably chunking will need this. https://codereview.chromium.org/1376593007/diff/320001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:35: virtual void SendResponse(SendCallback send, SendDoneCallback done) = 0; On 2015/10/13 19:43:48, David Benjamin wrote: > Nit: I'd probably call this GetResponse or something about putting the response > to a string. As far as the HttpResponse is concerned, it's just asynchronously > providing a string. It's the EmbeddedTestServer and stuff that decides to send > it. I guess that's kind of true in the base case, however we also want to support cases where SendResponse is the thing setting up and running the send task. https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED scoped_ptr<HttpResponse>() On 2015/10/13 19:43:48, David Benjamin wrote: > Just return nullptr. nullptr has a distinct type (nullptr_t) so scoped_ptr (and > std::unique_ptr) can handle it. Done. https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED scoped_ptr<HttpResponse>() On 2015/10/13 19:55:50, mmenke wrote: > I'm not sure all of these really belong in this CL - maybe just switch over some > simpler tests that don't use all the fanciness to the embedded test server with > SSL, to make sure it works? Also wondering if we can break up landing this file > into chunks. 1000-line reviews often don't go well. Bugs missed, > relanding/re-review is a pain, etc. Even if we don't land request_helpers in this CL, we'll probably need to land most of it in one CL, since there's enough overlap in usage of the various handlers. At the very least these handlers are correct in terms of the existing tests that have been migrated to ETS, and I don't think any missed bugs will be too big of an issue to fix once someone creates new tests that use ETS.
https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/340001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED scoped_ptr<HttpResponse>() On 2015/10/13 20:54:44, svaldez wrote: > On 2015/10/13 19:55:50, mmenke wrote: > > I'm not sure all of these really belong in this CL - maybe just switch over > some > > simpler tests that don't use all the fanciness to the embedded test server > with > > SSL, to make sure it works? Also wondering if we can break up landing this > file > > into chunks. 1000-line reviews often don't go well. Bugs missed, > > relanding/re-review is a pain, etc. > Even if we don't land request_helpers in this CL, we'll probably need to land > most of it in one CL, since there's enough overlap in usage of the various > handlers. > > At the very least these handlers are correct in terms of the existing tests that > have been migrated to ETS, and I don't think any missed bugs will be too big of > an issue to fix once someone creates new tests that use ETS. Just because the trybots pass does not mean this whole thing will land smoothly, unfortunately.
svaldez@chromium.org changed reviewers: + kelvinp@chromium.org, lazyboy@chromium.org
svaldez@chromium.org changed required reviewers: + kelvinp@chromium.org, lazyboy@chromium.org, mmenke@chromium.org
Hi, As part of the work on EmbeddedTestServer, I've ended up changing the SendResponse interface. Could you take a look at the files you own and comment on/approve the changes? lazyboy: extensions/browser/guest_view/web_view/web_view_apitest.cc chrome/browser/apps/guest_view/web_view_browsertest.cc kelvinp: remoting/protocol/ssl_hmac_channel_authenticator.cc Thanks, Steven
web_view_apitest.cc and web_view_browsertest.cc changes seem fine to me.
On 2015/10/14 18:31:00, lazyboy wrote: > web_view_apitest.cc and web_view_browsertest.cc changes seem fine to me. Could you LGTM so that I can push this to the CQ when the net/ stuff gets reviewed and finalized? I don't think there will be any changes to this CL outside of the net/ code before then.
Sure, I thought you were going to wait for net/ review first. web_view_apitest.cc and web_view_browsertest.cc LGTM.
https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.cc:7: #include "net/ssl/ssl_server_config.h" This should be first, one line above the others. https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:20: enum ServerCertificate { Suggest putting the enum above the constructor. THere's actually no tight rule about it, but that's where inner classes go, and the syntax for enums is somewhat like an inner class. https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:46: uint16_t version_max; include <stdint.h> https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:69: std::vector<uint16> disabled_cipher_suites; include <vector> https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:69: std::vector<uint16> disabled_cipher_suites; uint16_t (Like you're using above) is the latest coolness https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:42: : use_ssl_(false), connection_listener_(nullptr), port_(0) {} I believe this can just be: EmbeddedTestServer::EmbeddedTestServer() : EmbeddedTestServer(TYPE_HTTP) { } That makes adding fields a bit simpler. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:47: use_ssl_ = (type == TYPE_HTTPS); nit: Put in initializer list, and can make it const. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:155: for (size_t i = 0; i < default_request_handlers_.size(); ++i) { Can use a range loop. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:131: const net::HostPortPair host_port_pair() const { First const not needed. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:132: return net::HostPortPair::FromURL(base_url_); Neither of these net::'s are needed (We're in net::test_server, but there's no such thing as net::test_server::HostPortPair, so don't need the net::) https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:158: bool use_ssl() const { return use_ssl_; } Is this needed? Presumably whoever created us knows, and I don't think anyone else cares? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:160: void SetSSLConfig(net::SSLServerConfig ssl_config); net:: not needed. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:160: void SetSSLConfig(net::SSLServerConfig ssl_config); const SSLServerConfig&? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:233: bool use_ssl_; is_using_ssl_? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:106: if (header_line_tokens[1][0] == '/') I don't think this is needed? Don't we always send requests with a leading slash? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:155: } Why this change? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:243: LOG(WARNING) << "Method not implemented: " << token; NOTIMPLEMENTED()? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:244: return METHOD_GET; Why this change? https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:8: #include <map> While you're here, I don't think this is needed (This uses StringPairs instead of a map for headers) https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:21: using SendDoneCallback = base::Callback<void(void)>; Suggest Done->Complete (Complete's a bit more common in net/) https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // Callback called when the response is ready to be sent. Document done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:25: base::Callback<void(std::string response, SendDoneCallback done)>; Suggest calling this SendBytesCallback to more clearly distinguish it from SendResponse. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:36: // the operation completes, the callback will not be called. Think this documentation should be targeted at implementations, rather than the one consumer. i.e. "|send| will send the specified data to the network socket, and invoke |done| (Not this |done|, the other |done|) when complete. When the entire response is sent, |done| (This |done|, not the other one) must be called. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:37: virtual void SendResponse(SendCallback send, SendDoneCallback done) = 0; I assume you can use SendCallback and your own send done method to simulate slowly receiving bytes - I don't think this is at all clear from the docs, though. If this is the case, should rename this one send_response_complete, and the other one (In the typedef) send_complete. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:41: // response headers such as "Content-Type". Maybe add something along the lines of "Sends the response instantly."
asanka@chromium.org changed reviewers: + asanka@chromium.org
https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:223: base::FilePath::StringType ext = file_path.FinalExtension(); Nit: Use FilePath::MatchesExtension() instead of extracting the extension and comparing. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:903: // TODO(svaldez): HandleDownload Note that we are going to move the download tests to use a mock URLRequestJob instead. Don't try to convert those.
https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.cc:7: #include "net/ssl/ssl_server_config.h" On 2015/10/14 21:59:15, mmenke wrote: > This should be first, one line above the others. Done. https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:20: enum ServerCertificate { On 2015/10/14 21:59:15, mmenke wrote: > Suggest putting the enum above the constructor. THere's actually no tight rule > about it, but that's where inner classes go, and the syntax for enums is > somewhat like an inner class. Done. https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:46: uint16_t version_max; On 2015/10/14 21:59:15, mmenke wrote: > include <stdint.h> Done. https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:69: std::vector<uint16> disabled_cipher_suites; On 2015/10/14 21:59:15, mmenke wrote: > uint16_t (Like you're using above) is the latest coolness Done. https://codereview.chromium.org/1376593007/diff/460001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:69: std::vector<uint16> disabled_cipher_suites; On 2015/10/14 21:59:15, mmenke wrote: > include <vector> Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:42: : use_ssl_(false), connection_listener_(nullptr), port_(0) {} On 2015/10/14 21:59:15, mmenke wrote: > I believe this can just be: > > EmbeddedTestServer::EmbeddedTestServer() : EmbeddedTestServer(TYPE_HTTP) { > } > > That makes adding fields a bit simpler. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:47: use_ssl_ = (type == TYPE_HTTPS); On 2015/10/14 21:59:15, mmenke wrote: > nit: Put in initializer list, and can make it const. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:155: for (size_t i = 0; i < default_request_handlers_.size(); ++i) { On 2015/10/14 21:59:15, mmenke wrote: > Can use a range loop. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:131: const net::HostPortPair host_port_pair() const { On 2015/10/14 21:59:16, mmenke wrote: > First const not needed. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:132: return net::HostPortPair::FromURL(base_url_); On 2015/10/14 21:59:16, mmenke wrote: > Neither of these net::'s are needed (We're in net::test_server, but there's no > such thing as net::test_server::HostPortPair, so don't need the net::) Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:158: bool use_ssl() const { return use_ssl_; } On 2015/10/14 21:59:16, mmenke wrote: > Is this needed? Presumably whoever created us knows, and I don't think anyone > else cares? This is currently used for ETS unittests, and is slightly nicer and more straight forward than having to use GetParams() and checking that everywhere. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:160: void SetSSLConfig(net::SSLServerConfig ssl_config); On 2015/10/14 21:59:15, mmenke wrote: > net:: not needed. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:160: void SetSSLConfig(net::SSLServerConfig ssl_config); On 2015/10/14 21:59:16, mmenke wrote: > const SSLServerConfig&? Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:233: bool use_ssl_; On 2015/10/14 21:59:16, mmenke wrote: > is_using_ssl_? Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:106: if (header_line_tokens[1][0] == '/') On 2015/10/14 21:59:16, mmenke wrote: > I don't think this is needed? Don't we always send requests with a leading > slash? There are unfortunately some tests that don't send leading slashes, and this is legal per RFC. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:155: } On 2015/10/14 21:59:16, mmenke wrote: > Why this change? Some tests end up sending "Content-Length: ". https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:243: LOG(WARNING) << "Method not implemented: " << token; On 2015/10/14 21:59:16, mmenke wrote: > NOTIMPLEMENTED()? See below. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:244: return METHOD_GET; On 2015/10/14 21:59:16, mmenke wrote: > Why this change? To allow us to handle TRACK/TRACE/... used by tests. The tests never actually care what we return, but need to be a server to send the request to in order to verify that extensions policies work correctly. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:8: #include <map> On 2015/10/14 21:59:16, mmenke wrote: > While you're here, I don't think this is needed (This uses StringPairs instead > of a map for headers) Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:21: using SendDoneCallback = base::Callback<void(void)>; On 2015/10/14 21:59:16, mmenke wrote: > Suggest Done->Complete (Complete's a bit more common in net/) Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // Callback called when the response is ready to be sent. On 2015/10/14 21:59:16, mmenke wrote: > Document done. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:25: base::Callback<void(std::string response, SendDoneCallback done)>; On 2015/10/14 21:59:16, mmenke wrote: > Suggest calling this SendBytesCallback to more clearly distinguish it from > SendResponse. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:36: // the operation completes, the callback will not be called. On 2015/10/14 21:59:16, mmenke wrote: > Think this documentation should be targeted at implementations, rather than the > one consumer. > > i.e. "|send| will send the specified data to the network socket, and invoke > |done| (Not this |done|, the other |done|) when complete. When the entire > response is sent, |done| (This |done|, not the other one) must be called. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:37: virtual void SendResponse(SendCallback send, SendDoneCallback done) = 0; On 2015/10/14 21:59:16, mmenke wrote: > I assume you can use SendCallback and your own send done method to simulate > slowly receiving bytes - I don't think this is at all clear from the docs, > though. If this is the case, should rename this one send_response_complete, and > the other one (In the typedef) send_complete. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:41: // response headers such as "Content-Type". On 2015/10/14 21:59:16, mmenke wrote: > Maybe add something along the lines of "Sends the response instantly." Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:223: base::FilePath::StringType ext = file_path.FinalExtension(); On 2015/10/14 22:28:52, asanka wrote: > Nit: Use FilePath::MatchesExtension() instead of extracting the extension and > comparing. Done. https://codereview.chromium.org/1376593007/diff/460001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:903: // TODO(svaldez): HandleDownload On 2015/10/14 22:28:52, asanka wrote: > Note that we are going to move the download tests to use a mock URLRequestJob > instead. Don't try to convert those. Acknowledged.
https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED nullptr This seems kinda weird and not really needed. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:36: net::UnescapeRule::REPLACE_PLUS_WITH_SPACE; Anonymous namespace, and move this down so you don't need the net::'s https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:39: const std::string kDefaultPassword = "password"; const char[] (Only POD types can be globals, and Singletons) https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:52: bool ShouldHandle(const HttpRequest& request, std::string url) { May want to build this into EmbeddedTestServer, as a parameter to RegisterDefaultHandler or make an easy way to wrap a handler in another callback that applies this first. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:52: bool ShouldHandle(const HttpRequest& request, std::string url) { |url| -> |path_prefix| https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:55: std::string endings("/?;#"); Not a fan of manually parsing URLs like this. I suggest using a GURL (We do have ToGURL, after all). And then using: url.path() == relative_url || base::StartsWith(url.path(), relative_url + "/", base::SENSITIVE) https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:60: RequestQuery ParseQuery(const GURL& url) { All this stuff that isn't exposed in the header should be in an anonymous namespace. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:70: void GetFilePathWithReplacements(const std::string& original_file_path, This needs documentation. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:72: std::string* replacement_path) { Should just return a string (It's a little less efficient, but we don't really care) https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:76: it != text_to_replace.end(); ++it) { Can you use a range loop here? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:99: return GURL("http://localhost" + request.relative_url); Suggest making this a member of HttpRequest (And using EmbeddedTestServer::GetURL() to get it). https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:106: base::ThreadRestrictions::ScopedAllowIO allow_io; Is this needed? We're running on our private thread, and I thought threads were allowed to do file I/O by default. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:115: std::string files_prefix("/files/"); I thought you said you weren't doing this? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:131: if (request.content.find(query["expected_body"].front()) == base::StartsWithASCII? I don't think I've ever seen compare used in chrome, except in sorting functions. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:142: it != headers.end(); ++it) { Range loop? (And can then just inline query["expected_headers"]) https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:144: std::string value = it->substr(it->find(":") + 1); Should probably just explicitly fail when there is no colon. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:161: return NOT_HANDLED; Should we return an error if there was a files/ or post/ prefix in this case? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:167: it != replacements.end(); ++it) { Range loop https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:168: std::string find, with; Only declare one variable per line. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:182: return NOT_HANDLED; Should we return an error in this case? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:185: file_contents = ""; Should we be doing this regardless of the presence of the headers file? That's what the old server does. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:197: if (range_header.compare(0, 6, "bytes=") == 0) { HttpUtil::ParseRangeHeader? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:248: http_response->set_content_type("text/html"); Think all this should be in a separate method. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:260: return NOT_HANDLED; nit: Use braces https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:273: return NOT_HANDLED; nit: Braces https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:294: scoped_ptr<HttpResponse> HandleCacheControl(std::string url, All of these should be const std::string & https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:295: std::string value, Should use a clearer name than "value" https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:303: time << std::time(0); Suggest using base::Time https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:305: "</title></head></html>"); Hrm...I wonder about the utility of all of these - with them in an external server. Unless we want to make a binary where you can just run the test server independently of chrome, these HTML responses seem less useful than they were with the test server. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:351: if (!ShouldHandle(request, "/echo")) Think all of the ones based on path should use a wrapper callback which does this - makes it so most of the paths can be put together in one place. Can also make two instances of the file handler (One for "files/", one not). https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:860: base::Bind(&HandleCacheControl, "/nocachetime/maxage", "max-age=0")); Rather than make all of these take a path, suggest a wrapper method.
A few more comments. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:214: break; Can just make these all returns https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:285: scoped_refptr<X509Certificate> server_cert = GetCertificate(); This should probably either go before all the stuff used to create the server_key, or after it all, rather than in the middle. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:324: ReadData(connection); else DidClose(connection); https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:158: bool use_ssl() const { return is_using_ssl_; } use_ssl -> is_using_ssl https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:189: void RegisterDefaultHandler(const HandleRequestCallback& callback); This should be private. A couple ways to do that - pass a callback into the method it calls, move RegisterDefaultHandlers to EmbeddedTestServer, friend the method (Though I'm not a huge fan of friends). https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:248: // List of registered and default request handlers. Lists? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/http_connection.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/http_connection.cc:58: return; Hrm...the fact that we didn't have these before is concerning. Add a test for it? Just make a handler that continuously post tasks to send data, then start a request, wait for headers, and cancel/destroy the request. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:5: #include "net/test/embedded_test_server/request_helpers.h" default_request_handlers?
Running against the tests on the other CL to see if anything breaks, but should have most of the fixes. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:214: break; On 2015/10/15 20:16:14, mmenke wrote: > Can just make these all returns Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:285: scoped_refptr<X509Certificate> server_cert = GetCertificate(); On 2015/10/15 20:16:15, mmenke wrote: > This should probably either go before all the stuff used to create the > server_key, or after it all, rather than in the middle. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:324: ReadData(connection); On 2015/10/15 20:16:15, mmenke wrote: > else > DidClose(connection); Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:158: bool use_ssl() const { return is_using_ssl_; } On 2015/10/15 20:16:15, mmenke wrote: > use_ssl -> is_using_ssl Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:189: void RegisterDefaultHandler(const HandleRequestCallback& callback); On 2015/10/15 20:16:15, mmenke wrote: > This should be private. > > A couple ways to do that - pass a callback into the method it calls, move > RegisterDefaultHandlers to EmbeddedTestServer, friend the method (Though I'm not > a huge fan of friends). I kind of want to keep this public, so that we could have things like ContentBrowserBase or InProcessTest choose to register a set of default handlers shared by those tests, so that its easier to remove global default handlers to other places. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:248: // List of registered and default request handlers. On 2015/10/15 20:16:15, mmenke wrote: > Lists? Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/http_connection.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/http_connection.cc:58: return; On 2015/10/15 20:16:15, mmenke wrote: > Hrm...the fact that we didn't have these before is concerning. Add a test for > it? Just make a handler that continuously post tasks to send data, then start a > request, wait for headers, and cancel/destroy the request. Should this be an ETS test, or should we have separate http_connection tests? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:5: #include "net/test/embedded_test_server/request_helpers.h" On 2015/10/15 20:16:15, mmenke wrote: > default_request_handlers? Since the main purpose of this file when used by other files is as a central place to have helper functions when creating handlers, seems request_helpers might be a better long-term name? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:31: #define NOT_HANDLED nullptr On 2015/10/15 19:38:18, mmenke wrote: > This seems kinda weird and not really needed. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:36: net::UnescapeRule::REPLACE_PLUS_WITH_SPACE; On 2015/10/15 19:38:17, mmenke wrote: > Anonymous namespace, and move this down so you don't need the net::'s Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:39: const std::string kDefaultPassword = "password"; On 2015/10/15 19:38:17, mmenke wrote: > const char[] (Only POD types can be globals, and Singletons) Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:52: bool ShouldHandle(const HttpRequest& request, std::string url) { On 2015/10/15 19:38:18, mmenke wrote: > |url| -> |path_prefix| Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:52: bool ShouldHandle(const HttpRequest& request, std::string url) { On 2015/10/15 19:38:17, mmenke wrote: > May want to build this into EmbeddedTestServer, as a parameter to > RegisterDefaultHandler or make an easy way to wrap a handler in another callback > that applies this first. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:55: std::string endings("/?;#"); On 2015/10/15 19:38:17, mmenke wrote: > Not a fan of manually parsing URLs like this. > > I suggest using a GURL (We do have ToGURL, after all). > > And then using: > > url.path() == relative_url || base::StartsWith(url.path(), relative_url + "/", > base::SENSITIVE) Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:60: RequestQuery ParseQuery(const GURL& url) { On 2015/10/15 19:38:18, mmenke wrote: > All this stuff that isn't exposed in the header should be in an anonymous > namespace. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:70: void GetFilePathWithReplacements(const std::string& original_file_path, On 2015/10/15 19:38:18, mmenke wrote: > This needs documentation. In the header. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:72: std::string* replacement_path) { On 2015/10/15 19:38:17, mmenke wrote: > Should just return a string (It's a little less efficient, but we don't really > care) Maintaining compatibility with the STS version of this method to make migration easier. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:76: it != text_to_replace.end(); ++it) { On 2015/10/15 19:38:18, mmenke wrote: > Can you use a range loop here? Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:99: return GURL("http://localhost" + request.relative_url); On 2015/10/15 19:38:17, mmenke wrote: > Suggest making this a member of HttpRequest (And using > EmbeddedTestServer::GetURL() to get it). Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:106: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2015/10/15 19:38:18, mmenke wrote: > Is this needed? We're running on our private thread, and I thought threads were > allowed to do file I/O by default. Still required since the restriction is only on the pure IO thread. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:115: std::string files_prefix("/files/"); On 2015/10/15 19:38:17, mmenke wrote: > I thought you said you weren't doing this? Yeah, I was using it to find any remaining old-style failures, but I forgot to remove this when I finished conversion. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:131: if (request.content.find(query["expected_body"].front()) == On 2015/10/15 19:38:18, mmenke wrote: > base::StartsWithASCII? I don't think I've ever seen compare used in chrome, > except in sorting functions. Is there a version for "containsASCII"? https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:142: it != headers.end(); ++it) { On 2015/10/15 19:38:17, mmenke wrote: > Range loop? (And can then just inline query["expected_headers"]) Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:144: std::string value = it->substr(it->find(":") + 1); On 2015/10/15 19:38:17, mmenke wrote: > Should probably just explicitly fail when there is no colon. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:161: return NOT_HANDLED; On 2015/10/15 19:38:17, mmenke wrote: > Should we return an error if there was a files/ or post/ prefix in this case? Could be that there is another File handler that might catch it. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:167: it != replacements.end(); ++it) { On 2015/10/15 19:38:17, mmenke wrote: > Range loop Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:168: std::string find, with; On 2015/10/15 19:38:18, mmenke wrote: > Only declare one variable per line. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:182: return NOT_HANDLED; On 2015/10/15 19:38:17, mmenke wrote: > Should we return an error in this case? The way its currently implemented in STS only returns an error if the expected_ are not there, otherwise its possible other handlers exist to catch the error. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:185: file_contents = ""; On 2015/10/15 19:38:17, mmenke wrote: > Should we be doing this regardless of the presence of the headers file? That's > what the old server does. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:197: if (range_header.compare(0, 6, "bytes=") == 0) { On 2015/10/15 19:38:18, mmenke wrote: > HttpUtil::ParseRangeHeader? Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:248: http_response->set_content_type("text/html"); On 2015/10/15 19:38:17, mmenke wrote: > Think all this should be in a separate method. Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:260: return NOT_HANDLED; On 2015/10/15 19:38:18, mmenke wrote: > nit: Use braces Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:273: return NOT_HANDLED; On 2015/10/15 19:38:18, mmenke wrote: > nit: Braces Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:294: scoped_ptr<HttpResponse> HandleCacheControl(std::string url, On 2015/10/15 19:38:17, mmenke wrote: > All of these should be const std::string & Acknowledged. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:295: std::string value, On 2015/10/15 19:38:18, mmenke wrote: > Should use a clearer name than "value" Acknowledged. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:303: time << std::time(0); On 2015/10/15 19:38:17, mmenke wrote: > Suggest using base::Time Acknowledged. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:305: "</title></head></html>"); On 2015/10/15 19:38:17, mmenke wrote: > Hrm...I wonder about the utility of all of these - with them in an external > server. Unless we want to make a binary where you can just run the test server > independently of chrome, these HTML responses seem less useful than they were > with the test server. Acknowledged. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:351: if (!ShouldHandle(request, "/echo")) On 2015/10/15 19:38:17, mmenke wrote: > Think all of the ones based on path should use a wrapper callback which does > this - makes it so most of the paths can be put together in one place. Can also > make two instances of the file handler (One for "files/", one not). Done. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:860: base::Bind(&HandleCacheControl, "/nocachetime/maxage", "max-age=0")); On 2015/10/15 19:38:17, mmenke wrote: > Rather than make all of these take a path, suggest a wrapper method. Done.
quick responses. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/http_connection.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/http_connection.cc:58: return; On 2015/10/15 21:04:32, svaldez wrote: > On 2015/10/15 20:16:15, mmenke wrote: > > Hrm...the fact that we didn't have these before is concerning. Add a test for > > it? Just make a handler that continuously post tasks to send data, then start > a > > request, wait for headers, and cancel/destroy the request. > > Should this be an ETS test, or should we have separate http_connection tests? I'm thinking an ETS test. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:5: #include "net/test/embedded_test_server/request_helpers.h" On 2015/10/15 21:04:32, svaldez wrote: > On 2015/10/15 20:16:15, mmenke wrote: > > default_request_handlers? > > Since the main purpose of this file when used by other files is as a central > place to have helper functions when creating handlers, seems request_helpers > might be a better long-term name? If you want shareable helper methods for consumers, I'd separate them out from the default handlers. https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:131: if (request.content.find(query["expected_body"].front()) == On 2015/10/15 21:04:33, svaldez wrote: > On 2015/10/15 19:38:18, mmenke wrote: > > base::StartsWithASCII? I don't think I've ever seen compare used in chrome, > > except in sorting functions. > > Is there a version for "containsASCII"? Oops...That comment was meant to go below the "/post/" (And I don't think so) https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:161: return NOT_HANDLED; On 2015/10/15 21:04:33, svaldez wrote: > On 2015/10/15 19:38:17, mmenke wrote: > > Should we return an error if there was a files/ or post/ prefix in this case? > > Could be that there is another File handler that might catch it. But there isn't...files/ was removed anyways, now, and don't care much about post/
On 2015/10/15 21:13:54, mmenke wrote: > quick responses. > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > File net/test/embedded_test_server/http_connection.cc (right): > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > net/test/embedded_test_server/http_connection.cc:58: return; > On 2015/10/15 21:04:32, svaldez wrote: > > On 2015/10/15 20:16:15, mmenke wrote: > > > Hrm...the fact that we didn't have these before is concerning. Add a test > for > > > it? Just make a handler that continuously post tasks to send data, then > start > > a > > > request, wait for headers, and cancel/destroy the request. > > > > Should this be an ETS test, or should we have separate http_connection tests? > > I'm thinking an ETS test. > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > File net/test/embedded_test_server/request_helpers.cc (right): > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > net/test/embedded_test_server/request_helpers.cc:5: #include > "net/test/embedded_test_server/request_helpers.h" > On 2015/10/15 21:04:32, svaldez wrote: > > On 2015/10/15 20:16:15, mmenke wrote: > > > default_request_handlers? > > > > Since the main purpose of this file when used by other files is as a central > > place to have helper functions when creating handlers, seems request_helpers > > might be a better long-term name? > > If you want shareable helper methods for consumers, I'd separate them out from > the default handlers. > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > net/test/embedded_test_server/request_helpers.cc:131: if > (request.content.find(query["expected_body"].front()) == > On 2015/10/15 21:04:33, svaldez wrote: > > On 2015/10/15 19:38:18, mmenke wrote: > > > base::StartsWithASCII? I don't think I've ever seen compare used in chrome, > > > except in sorting functions. > > > > Is there a version for "containsASCII"? > > Oops...That comment was meant to go below the "/post/" (And I don't think so) Done > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > net/test/embedded_test_server/request_helpers.cc:161: return NOT_HANDLED; > On 2015/10/15 21:04:33, svaldez wrote: > > On 2015/10/15 19:38:17, mmenke wrote: > > > Should we return an error if there was a files/ or post/ prefix in this > case? > > > > Could be that there is another File handler that might catch it. > > But there isn't...files/ was removed anyways, now, and don't care much about > post/ I think to some extent we shouldn't be consuming a request (returning an error page) unless the handler is actively supposed to rely on that behavior, the 'expected_' checks. This also seems to be the behavior some of the existing tests (Fake GAIA) seem to be using as well, with fallthrough in case of errors.
On 2015/10/15 21:20:43, svaldez wrote: > On 2015/10/15 21:13:54, mmenke wrote: > > quick responses. > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > File net/test/embedded_test_server/http_connection.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/http_connection.cc:58: return; > > On 2015/10/15 21:04:32, svaldez wrote: > > > On 2015/10/15 20:16:15, mmenke wrote: > > > > Hrm...the fact that we didn't have these before is concerning. Add a test > > for > > > > it? Just make a handler that continuously post tasks to send data, then > > start > > > a > > > > request, wait for headers, and cancel/destroy the request. > > > > > > Should this be an ETS test, or should we have separate http_connection > tests? > > > > I'm thinking an ETS test. > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > File net/test/embedded_test_server/request_helpers.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/request_helpers.cc:5: #include > > "net/test/embedded_test_server/request_helpers.h" > > On 2015/10/15 21:04:32, svaldez wrote: > > > On 2015/10/15 20:16:15, mmenke wrote: > > > > default_request_handlers? > > > > > > Since the main purpose of this file when used by other files is as a central > > > place to have helper functions when creating handlers, seems request_helpers > > > might be a better long-term name? > > > > If you want shareable helper methods for consumers, I'd separate them out from > > the default handlers. > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/request_helpers.cc:131: if > > (request.content.find(query["expected_body"].front()) == > > On 2015/10/15 21:04:33, svaldez wrote: > > > On 2015/10/15 19:38:18, mmenke wrote: > > > > base::StartsWithASCII? I don't think I've ever seen compare used in > chrome, > > > > except in sorting functions. > > > > > > Is there a version for "containsASCII"? > > > > Oops...That comment was meant to go below the "/post/" (And I don't think so) > Done > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/request_helpers.cc:161: return NOT_HANDLED; > > On 2015/10/15 21:04:33, svaldez wrote: > > > On 2015/10/15 19:38:17, mmenke wrote: > > > > Should we return an error if there was a files/ or post/ prefix in this > > case? > > > > > > Could be that there is another File handler that might catch it. > > > > But there isn't...files/ was removed anyways, now, and don't care much about > > post/ > I think to some extent we shouldn't be consuming a request (returning an error > page) unless the handler is actively supposed to rely on that behavior, the > 'expected_' checks. This also seems to be the behavior some of the existing > tests (Fake GAIA) seem to be using as well, with fallthrough in case of errors. I feel like the default handlers should have exclusive control over their paths (Modulo the files one), but don't feel strongly enough about it to argue about. :)
On 2015/10/15 21:20:43, svaldez wrote: > On 2015/10/15 21:13:54, mmenke wrote: > > quick responses. > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > File net/test/embedded_test_server/http_connection.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/http_connection.cc:58: return; > > On 2015/10/15 21:04:32, svaldez wrote: > > > On 2015/10/15 20:16:15, mmenke wrote: > > > > Hrm...the fact that we didn't have these before is concerning. Add a test > > for > > > > it? Just make a handler that continuously post tasks to send data, then > > start > > > a > > > > request, wait for headers, and cancel/destroy the request. > > > > > > Should this be an ETS test, or should we have separate http_connection > tests? > > > > I'm thinking an ETS test. Hmm, is there a good way to do this without using RunLoops triggered from the handler? URLFetcher doesn't have a nice interface to check if its received part of a response. > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > File net/test/embedded_test_server/request_helpers.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/request_helpers.cc:5: #include > > "net/test/embedded_test_server/request_helpers.h" > > On 2015/10/15 21:04:32, svaldez wrote: > > > On 2015/10/15 20:16:15, mmenke wrote: > > > > default_request_handlers? > > > > > > Since the main purpose of this file when used by other files is as a central > > > place to have helper functions when creating handlers, seems request_helpers > > > might be a better long-term name? > > > > If you want shareable helper methods for consumers, I'd separate them out from > > the default handlers. > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/request_helpers.cc:131: if > > (request.content.find(query["expected_body"].front()) == > > On 2015/10/15 21:04:33, svaldez wrote: > > > On 2015/10/15 19:38:18, mmenke wrote: > > > > base::StartsWithASCII? I don't think I've ever seen compare used in > chrome, > > > > except in sorting functions. > > > > > > Is there a version for "containsASCII"? > > > > Oops...That comment was meant to go below the "/post/" (And I don't think so) > Done > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > net/test/embedded_test_server/request_helpers.cc:161: return NOT_HANDLED; > > On 2015/10/15 21:04:33, svaldez wrote: > > > On 2015/10/15 19:38:17, mmenke wrote: > > > > Should we return an error if there was a files/ or post/ prefix in this > > case? > > > > > > Could be that there is another File handler that might catch it. > > > > But there isn't...files/ was removed anyways, now, and don't care much about > > post/ > I think to some extent we shouldn't be consuming a request (returning an error > page) unless the handler is actively supposed to rely on that behavior, the > 'expected_' checks. This also seems to be the behavior some of the existing > tests (Fake GAIA) seem to be using as well, with fallthrough in case of errors.
Going to suggest once again that we don't do all this in one huge CL. We can add the handlers first or second, but there's just no need to add 800 lines of miscellaneous handlers in the same CL we add SSL support in. I'd also like to do the handlers in smaller chunks (For example, add the infrastructure, the file, and the auth ones in one CL, and switch a couple tests over to use them). Staring a 800 lines of miscellaneous functions is just mind numbing, and stuff is much more likely to be missed.
On 2015/10/16 17:59:39, mmenke wrote: > Going to suggest once again that we don't do all this in one huge CL. We can > add the handlers first or second, but there's just no need to add 800 lines of > miscellaneous handlers in the same CL we add SSL support in. > > I'd also like to do the handlers in smaller chunks (For example, add the > infrastructure, the file, and the auth ones in one CL, and switch a couple tests > over to use them). Staring a 800 lines of miscellaneous functions is just mind > numbing, and stuff is much more likely to be missed. I've separated out most of the default handlers to a separate CL so that we can land the SSL part sooner. For separating the handlers, while we can possibly land the handlers in smaller CLs, we'd still have to land them all before we can really easily migrate tests over, since there's enough overlap with the currently implemented handlers that either we wouldn't be able to chunk the test migration CLs or we'd have to add additional test files/test server instances while we are in the state of only having a subset of the frequently used handlers migrated.
I think this is looking pretty good. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:203: std::string EmbeddedTestServer::GetCertificateName() const { DCHECK(is_using_ssl_);? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:245: void EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { Maybe put DCHECK(!io_thread_.get()); in all of these handler registration methods? Or could use the stronger DCHECK(!Started()); https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:260: void EmbeddedTestServer::LoadTestSSLRoot() { Make this method static, or put into an anonymous namespace at the top of this file. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:263: return; Should we fail in some way when this happens? DCHECK? Return false from InitializeAndListen, either by recording a bool or be calling the method there (Though that may be too late...What are the thread safety requirements of this code?) We should also document the threading requirements with the method declaration, too. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:99: explicit EmbeddedTestServer(const Type type); is_using_ssl_ should be const, rather than the type argument. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:111: bool Start(); WARN_UNUSED_RESULT? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:164: std::string GetCertificateName() const; Does this need to be public? I don't see anything using it. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:167: scoped_refptr<X509Certificate> GetCertificate() const; Again, I don't see this being called anywhere, and it's not exposed by the spawned test server. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:173: void ServeFilesFromDirectory(const base::FilePath& directory); Add a TODO to remove this method? I don't think we want two different ways of specifying directories, and I think the difference between the two ways of specifying paths is far from clear. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:176: void ServeFilesFromSourceDirectory(const std::string relative); const std::string& https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:189: // have been tried. Should also specify order relative to those added by AddDefaultHandlers. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:200: // Handles async callback when we've finished performing the SSL handshake. nit: Don't use "we" in comments. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:123: // SSLClientSocket. See https://crbug.com/539520. That TODO is a bit weird - this isn't deep in SSLClientSocket. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:144: ASSERT_TRUE(server_->ShutdownAndWaitUntilComplete()); Shouldn't we shut down the server before tearing down NSS, which the server may be using? In general, shutdown order should be the opposite of startup order. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:380: // SSLClientSocket. See https://crbug.com/539520. Put this below #if, like you do at the start of this file? The TODO here is also weird. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_connection.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_connection.h:42: void SendResponse(std::string response_string, SendCompleteCallback callback); const std::string& https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_connection.h:42: void SendResponse(std::string response_string, SendCompleteCallback callback); Maybe SendResponse -> SendBytes or SendResponseBytes, to indicate it may not be a full response? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:40: return GURL("http://localhost" + relative_url); Could you add a TODO to consider making this return the real URL, from the embedded test server (Not suggesting you do it here because getting the EmbeddedTestServer and the relative_url at the same time is a little ugly). https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:107: if (url.is_valid()) { If we have a full URL, surely HttpRequest::ToURL should return it? (And I'd suggest renaming ToURL to just GetURL or URL()). https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:110: if (header_line_tokens[1][0] == '/') else if { } else { } https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:158: LOG(WARNING) << "Malformed Content-Length header's value."; We actually have tests that send invalid content-length headers? ...Why? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_response.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.cc:39: content_.size()); Why don't we send a content-length when when the content size is 0? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // |response| we are sending along with 1the callback |done| that is called nit: --we https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // |response| we are sending along with 1the callback |done| that is called 1the -> the https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // |response| we are sending along with 1the callback |done| that is called |done| -> |write_done| https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:37: SendCompleteCallback done) = 0; cosnt SendBytesCallback&, const SendCompleteCallback& https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:40: else if (path.MatchesExtension(FILE_PATH_LITERAL(".exe"))) Don't need all these else's, since earlier branches return early. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:46: return "image/jpeg"; nit: Use braces when condition takes more than one line. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:61: return "text/html"; nit: braces https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:65: } } // namespace https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:70: if (headers_.size() != 0 || contents_.size() != 0) prefer !headers_.empty() (It may be faster / smaller, depending on implementation) https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:75: bool ShouldHandle(const HttpRequest& request, std::string path_prefix) { std::string -> const std::string& https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:130: // This is a test-only server. Ignore I/O thread restrictions. Could you add a TODO about figuring out why this is needed? Since we're creating the thread this is running on, it shouldn't have any restrictions. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:137: GURL request_gurl = request.ToURL(); nit: request_url https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:138: relative_url = request_gurl.path(); Suggest calling this relative_path (Since this is different from request.relative_url) https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:29: : headers_(headers), contents_(contents) {} nit: De-inline the constructor. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:29: : headers_(headers), contents_(contents) {} Child classes should have a destructor with override (Know it was like this before) https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:35: std::string contents_; Make these const? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:45: std::string prefix, const std::string& https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:46: EmbeddedTestServer::HandleRequestCallback handler, const EmbeddedTestServer::HandleRequestCallback& https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:49: // Parses |data| as the query of a URL and places it into a map. This comment needs to be updated (There is no |data|) https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:57: std::string* replacement_path); Maybe make this a method on the test server? GURL GetURLWithReplacements(original_path, text_to_replace);? Also think the description could be clearer.
message: On 2015/10/15 22:07:33, svaldez wrote: > On 2015/10/15 21:20:43, svaldez wrote: > > On 2015/10/15 21:13:54, mmenke wrote: > > > quick responses. > > > > > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > > File net/test/embedded_test_server/http_connection.cc (right): > > > > > > > > > https://codereview.chromium.org/1376593007/diff/500001/net/test/embedded_test... > > > net/test/embedded_test_server/http_connection.cc:58: return; > > > On 2015/10/15 21:04:32, svaldez wrote: > > > > On 2015/10/15 20:16:15, mmenke wrote: > > > > > Hrm...the fact that we didn't have these before is concerning. Add a > test > > > for > > > > > it? Just make a handler that continuously post tasks to send data, then > > > start > > > > a > > > > > request, wait for headers, and cancel/destroy the request. > > > > > > > > Should this be an ETS test, or should we have separate http_connection > > tests? > > > > > > I'm thinking an ETS test. > Hmm, is there a good way to do this without using RunLoops triggered from the > handler? URLFetcher doesn't have a nice interface to check if its received part > of a response. Sorry, forgot to respond to this. I see two options: 1) Use URLRequest instead of URLFetcher. Plenty of examples in url_request_unittests to copy how to do this from, without much extra code. I believe TestDelegate can just be set to quit the message loop after receiving headers. If not, can make a simple URLRequestDelegate that can do that. 2) Make the handler or an EmbeddedTestServerConnectionListener exit a run loop once we start sending a response / a connection is established. Suppose you could also set a ResponseWriter on a URLFetcher to do the same basic thing. Open to other ideas.
https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:203: std::string EmbeddedTestServer::GetCertificateName() const { On 2015/10/19 18:07:40, mmenke wrote: > DCHECK(is_using_ssl_);? Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:245: void EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { On 2015/10/19 18:07:40, mmenke wrote: > Maybe put DCHECK(!io_thread_.get()); in all of these handler registration > methods? Or could use the stronger DCHECK(!Started()); Its safe to call this after we've started the server. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:260: void EmbeddedTestServer::LoadTestSSLRoot() { On 2015/10/19 18:07:40, mmenke wrote: > Make this method static, or put into an anonymous namespace at the top of this > file. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:263: return; On 2015/10/19 18:07:40, mmenke wrote: > Should we fail in some way when this happens? DCHECK? Return false from > InitializeAndListen, either by recording a bool or be calling the method there > (Though that may be too late...What are the thread safety requirements of this > code?) > > We should also document the threading requirements with the method declaration, > too. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:99: explicit EmbeddedTestServer(const Type type); On 2015/10/19 18:07:40, mmenke wrote: > is_using_ssl_ should be const, rather than the type argument. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:111: bool Start(); On 2015/10/19 18:07:40, mmenke wrote: > WARN_UNUSED_RESULT? As part of using Start instead of InitializeAndWaitUntilReady, I'm moving the tests to ASSERT_TRUE/CHECK the return value of start. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:164: std::string GetCertificateName() const; On 2015/10/19 18:07:40, mmenke wrote: > Does this need to be public? I don't see anything using it. Some of the STS tests call GetCertificateName. See below. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:167: scoped_refptr<X509Certificate> GetCertificate() const; On 2015/10/19 18:07:40, mmenke wrote: > Again, I don't see this being called anywhere, and it's not exposed by the > spawned test server. Its exposed through the BaseTestServer interface for STS. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:173: void ServeFilesFromDirectory(const base::FilePath& directory); On 2015/10/19 18:07:40, mmenke wrote: > Add a TODO to remove this method? I don't think we want two different ways of > specifying directories, and I think the difference between the two ways of > specifying paths is far from clear. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:176: void ServeFilesFromSourceDirectory(const std::string relative); On 2015/10/19 18:07:40, mmenke wrote: > const std::string& Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:189: // have been tried. On 2015/10/19 18:07:40, mmenke wrote: > Should also specify order relative to those added by AddDefaultHandlers. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.h:200: // Handles async callback when we've finished performing the SSL handshake. On 2015/10/19 18:07:40, mmenke wrote: > nit: Don't use "we" in comments. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:123: // SSLClientSocket. See https://crbug.com/539520. On 2015/10/19 18:07:40, mmenke wrote: > That TODO is a bit weird - this isn't deep in SSLClientSocket. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:144: ASSERT_TRUE(server_->ShutdownAndWaitUntilComplete()); On 2015/10/19 18:07:40, mmenke wrote: > Shouldn't we shut down the server before tearing down NSS, which the server may > be using? In general, shutdown order should be the opposite of startup order. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:380: // SSLClientSocket. See https://crbug.com/539520. On 2015/10/19 18:07:40, mmenke wrote: > Put this below #if, like you do at the start of this file? The TODO here is > also weird. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_connection.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_connection.h:42: void SendResponse(std::string response_string, SendCompleteCallback callback); On 2015/10/19 18:07:40, mmenke wrote: > const std::string& Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_connection.h:42: void SendResponse(std::string response_string, SendCompleteCallback callback); On 2015/10/19 18:07:40, mmenke wrote: > Maybe SendResponse -> SendBytes or SendResponseBytes, to indicate it may not be > a full response? Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:40: return GURL("http://localhost" + relative_url); On 2015/10/19 18:07:40, mmenke wrote: > Could you add a TODO to consider making this return the real URL, from the > embedded test server (Not suggesting you do it here because getting the > EmbeddedTestServer and the relative_url at the same time is a little ugly). Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:107: if (url.is_valid()) { On 2015/10/19 18:07:40, mmenke wrote: > If we have a full URL, surely HttpRequest::ToURL should return it? (And I'd > suggest renaming ToURL to just GetURL or URL()). There are cases where the "full URL" is actually not a valid URL pointing to the ETS. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:110: if (header_line_tokens[1][0] == '/') On 2015/10/19 18:07:40, mmenke wrote: > else if { > } else { > } Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:158: LOG(WARNING) << "Malformed Content-Length header's value."; On 2015/10/19 18:07:40, mmenke wrote: > We actually have tests that send invalid content-length headers? ...Why? Some tests send Content-Length even if there isn't content or if its a body-less request. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_response.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.cc:39: content_.size()); On 2015/10/19 18:07:41, mmenke wrote: > Why don't we send a content-length when when the content size is 0? For responses that are only sending headers back, having a Content-Length causes the test to attempt to parse a body and fail when one isn't expected. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // |response| we are sending along with 1the callback |done| that is called On 2015/10/19 18:07:41, mmenke wrote: > |done| -> |write_done| Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // |response| we are sending along with 1the callback |done| that is called On 2015/10/19 18:07:41, mmenke wrote: > nit: --we Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:23: // |response| we are sending along with 1the callback |done| that is called On 2015/10/19 18:07:41, mmenke wrote: > 1the -> the Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:37: SendCompleteCallback done) = 0; On 2015/10/19 18:07:41, mmenke wrote: > cosnt SendBytesCallback&, const SendCompleteCallback& Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:40: else if (path.MatchesExtension(FILE_PATH_LITERAL(".exe"))) On 2015/10/19 18:07:41, mmenke wrote: > Don't need all these else's, since earlier branches return early. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:46: return "image/jpeg"; On 2015/10/19 18:07:41, mmenke wrote: > nit: Use braces when condition takes more than one line. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:61: return "text/html"; On 2015/10/19 18:07:41, mmenke wrote: > nit: braces Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:65: } On 2015/10/19 18:07:41, mmenke wrote: > } // namespace Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:70: if (headers_.size() != 0 || contents_.size() != 0) On 2015/10/19 18:07:41, mmenke wrote: > prefer !headers_.empty() (It may be faster / smaller, depending on > implementation) Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:75: bool ShouldHandle(const HttpRequest& request, std::string path_prefix) { On 2015/10/19 18:07:41, mmenke wrote: > std::string -> const std::string& Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:130: // This is a test-only server. Ignore I/O thread restrictions. On 2015/10/19 18:07:41, mmenke wrote: > Could you add a TODO about figuring out why this is needed? Since we're > creating the thread this is running on, it shouldn't have any restrictions. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:137: GURL request_gurl = request.ToURL(); On 2015/10/19 18:07:41, mmenke wrote: > nit: request_url Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:138: relative_url = request_gurl.path(); On 2015/10/19 18:07:41, mmenke wrote: > Suggest calling this relative_path (Since this is different from > request.relative_url) Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.h (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:29: : headers_(headers), contents_(contents) {} On 2015/10/19 18:07:41, mmenke wrote: > Child classes should have a destructor with override (Know it was like this > before) Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:29: : headers_(headers), contents_(contents) {} On 2015/10/19 18:07:41, mmenke wrote: > nit: De-inline the constructor. Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:35: std::string contents_; On 2015/10/19 18:07:41, mmenke wrote: > Make these const? Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:45: std::string prefix, On 2015/10/19 18:07:41, mmenke wrote: > const std::string& Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:46: EmbeddedTestServer::HandleRequestCallback handler, On 2015/10/19 18:07:41, mmenke wrote: > const EmbeddedTestServer::HandleRequestCallback& Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:49: // Parses |data| as the query of a URL and places it into a map. On 2015/10/19 18:07:41, mmenke wrote: > This comment needs to be updated (There is no |data|) Done. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:57: std::string* replacement_path); On 2015/10/19 18:07:41, mmenke wrote: > Maybe make this a method on the test server? > > GURL GetURLWithReplacements(original_path, text_to_replace);? > > Also think the description could be clearer. This should probably be bound to HandleFileRequest, since its only applicable to that handler, so it seems better to keep it here.
https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:245: void EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { On 2015/10/19 21:56:14, svaldez wrote: > On 2015/10/19 18:07:40, mmenke wrote: > > Maybe put DCHECK(!io_thread_.get()); in all of these handler registration > > methods? Or could use the stronger DCHECK(!Started()); > > Its safe to call this after we've started the server. But it's not safe not after we've started the IOThread.
On 2015/10/19 22:14:41, mmenke wrote: > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > File net/test/embedded_test_server/embedded_test_server.cc (right): > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > net/test/embedded_test_server/embedded_test_server.cc:245: void > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { > On 2015/10/19 21:56:14, svaldez wrote: > > On 2015/10/19 18:07:40, mmenke wrote: > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler registration > > > methods? Or could use the stronger DCHECK(!Started()); > > > > Its safe to call this after we've started the server. > > But it's not safe not after we've started the IOThread. Hmm, so while its not technically Thread-safe in general, its safe to call as long as we aren't currently serving any requests. If we require that the IO Thread hasn't been started yet, this means that a lot of tests won't be able to start the ETS as part of the harness, since they setup their own request handlers inside the test bodies. For example the EmbeddedTestServer unittests do this.
On 2015/10/20 13:57:33, svaldez wrote: > On 2015/10/19 22:14:41, mmenke wrote: > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { > > On 2015/10/19 21:56:14, svaldez wrote: > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler registration > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > Its safe to call this after we've started the server. > > > > But it's not safe not after we've started the IOThread. > > Hmm, so while its not technically Thread-safe in general, its safe to call as > long as we aren't currently serving any requests. If we require that the IO > Thread hasn't been started yet, this means that a lot of tests won't be able to > start the ETS as part of the harness, since they setup their own request > handlers inside the test bodies. For example the EmbeddedTestServer unittests do > this. I think we probably still want to support this use case, since it simplfies the amount of code that needs to be repeated for each test using EmbeddedTestServer.
On 2015/10/20 13:57:33, svaldez wrote: > On 2015/10/19 22:14:41, mmenke wrote: > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { > > On 2015/10/19 21:56:14, svaldez wrote: > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler registration > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > Its safe to call this after we've started the server. > > > > But it's not safe not after we've started the IOThread. > > Hmm, so while its not technically Thread-safe in general, its safe to call as > long as we aren't currently serving any requests. If we require that the IO > Thread hasn't been started yet, this means that a lot of tests won't be able to > start the ETS as part of the harness, since they setup their own request > handlers inside the test bodies. For example the EmbeddedTestServer unittests do > this. Sorry for the latency, took yesterday off. While there presumably is a memory barrier somewhere between when we install the hooks when when it starts receiving data, it's not something I want to rely on. Also, if we're running tests in parallel on the system, who's to say some throwaway request won't end up trying to talk to us. I'm fine with adding mutexes to do this safely (I also don't think PostTaskAndReply with a nested message loop is something we really want to do here).
On 2015/10/21 16:39:01, mmenke wrote: > On 2015/10/20 13:57:33, svaldez wrote: > > On 2015/10/19 22:14:41, mmenke wrote: > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { > > > On 2015/10/19 21:56:14, svaldez wrote: > > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler > registration > > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > > > Its safe to call this after we've started the server. > > > > > > But it's not safe not after we've started the IOThread. > > > > Hmm, so while its not technically Thread-safe in general, its safe to call as > > long as we aren't currently serving any requests. If we require that the IO > > Thread hasn't been started yet, this means that a lot of tests won't be able > to > > start the ETS as part of the harness, since they setup their own request > > handlers inside the test bodies. For example the EmbeddedTestServer unittests > do > > this. > > Sorry for the latency, took yesterday off. > > While there presumably is a memory barrier somewhere between when we install the > hooks when when it starts receiving data, it's not something I want to rely on. > Also, if we're running tests in parallel on the system, who's to say some > throwaway request won't end up trying to talk to us. > > I'm fine with adding mutexes to do this safely (I also don't think > PostTaskAndReply with a nested message loop is something we really want to do > here). As I think about this...I'm not really sure we want to support this usage. It seems to encourage writing flaky tests. Test fixtures can still start the test, as long as they install all needed hooks in advance, anyways.
On 2015/10/21 16:43:26, mmenke wrote: > On 2015/10/21 16:39:01, mmenke wrote: > > On 2015/10/20 13:57:33, svaldez wrote: > > > On 2015/10/19 22:14:41, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) { > > > > On 2015/10/19 21:56:14, svaldez wrote: > > > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler > > registration > > > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > > > > > Its safe to call this after we've started the server. > > > > > > > > But it's not safe not after we've started the IOThread. > > > > > > Hmm, so while its not technically Thread-safe in general, its safe to call > as > > > long as we aren't currently serving any requests. If we require that the IO > > > Thread hasn't been started yet, this means that a lot of tests won't be able > > to > > > start the ETS as part of the harness, since they setup their own request > > > handlers inside the test bodies. For example the EmbeddedTestServer > unittests > > do > > > this. > > > > Sorry for the latency, took yesterday off. > > > > While there presumably is a memory barrier somewhere between when we install > the > > hooks when when it starts receiving data, it's not something I want to rely > on. > > Also, if we're running tests in parallel on the system, who's to say some > > throwaway request won't end up trying to talk to us. > > > > I'm fine with adding mutexes to do this safely (I also don't think > > PostTaskAndReply with a nested message loop is something we really want to do > > here). > > As I think about this...I'm not really sure we want to support this usage. It > seems to encourage writing flaky tests. Test fixtures can still start the test, > as long as they install all needed hooks in advance, anyways. Hmm, looking at the crashes that result, I think we'll probably want to do this change as a separate CL since it'll involve changing a lot of the existing tests.
On 2015/10/21 17:09:55, svaldez wrote: > On 2015/10/21 16:43:26, mmenke wrote: > > On 2015/10/21 16:39:01, mmenke wrote: > > > On 2015/10/20 13:57:33, svaldez wrote: > > > > On 2015/10/19 22:14:41, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > > > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& directory) > { > > > > > On 2015/10/19 21:56:14, svaldez wrote: > > > > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler > > > registration > > > > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > > > > > > > Its safe to call this after we've started the server. > > > > > > > > > > But it's not safe not after we've started the IOThread. > > > > > > > > Hmm, so while its not technically Thread-safe in general, its safe to call > > as > > > > long as we aren't currently serving any requests. If we require that the > IO > > > > Thread hasn't been started yet, this means that a lot of tests won't be > able > > > to > > > > start the ETS as part of the harness, since they setup their own request > > > > handlers inside the test bodies. For example the EmbeddedTestServer > > unittests > > > do > > > > this. > > > > > > Sorry for the latency, took yesterday off. > > > > > > While there presumably is a memory barrier somewhere between when we install > > the > > > hooks when when it starts receiving data, it's not something I want to rely > > on. > > > Also, if we're running tests in parallel on the system, who's to say some > > > throwaway request won't end up trying to talk to us. > > > > > > I'm fine with adding mutexes to do this safely (I also don't think > > > PostTaskAndReply with a nested message loop is something we really want to > do > > > here). > > > > As I think about this...I'm not really sure we want to support this usage. It > > seems to encourage writing flaky tests. Test fixtures can still start the > test, > > as long as they install all needed hooks in advance, anyways. > > Hmm, looking at the crashes that result, I think we'll probably want to do this > change as a separate CL since it'll involve changing a lot of the existing > tests. SGTM. I'll file a fixit bug for that.
On 2015/10/21 17:22:57, mmenke wrote: > On 2015/10/21 17:09:55, svaldez wrote: > > On 2015/10/21 16:43:26, mmenke wrote: > > > On 2015/10/21 16:39:01, mmenke wrote: > > > > On 2015/10/20 13:57:33, svaldez wrote: > > > > > On 2015/10/19 22:14:41, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > > > > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& > directory) > > { > > > > > > On 2015/10/19 21:56:14, svaldez wrote: > > > > > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler > > > > registration > > > > > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > > > > > > > > > Its safe to call this after we've started the server. > > > > > > > > > > > > But it's not safe not after we've started the IOThread. > > > > > > > > > > Hmm, so while its not technically Thread-safe in general, its safe to > call > > > as > > > > > long as we aren't currently serving any requests. If we require that the > > IO > > > > > Thread hasn't been started yet, this means that a lot of tests won't be > > able > > > > to > > > > > start the ETS as part of the harness, since they setup their own request > > > > > handlers inside the test bodies. For example the EmbeddedTestServer > > > unittests > > > > do > > > > > this. > > > > > > > > Sorry for the latency, took yesterday off. > > > > > > > > While there presumably is a memory barrier somewhere between when we > install > > > the > > > > hooks when when it starts receiving data, it's not something I want to > rely > > > on. > > > > Also, if we're running tests in parallel on the system, who's to say some > > > > throwaway request won't end up trying to talk to us. > > > > > > > > I'm fine with adding mutexes to do this safely (I also don't think > > > > PostTaskAndReply with a nested message loop is something we really want to > > do > > > > here). > > > > > > As I think about this...I'm not really sure we want to support this usage. > It > > > seems to encourage writing flaky tests. Test fixtures can still start the > > test, > > > as long as they install all needed hooks in advance, anyways. > > > > Hmm, looking at the crashes that result, I think we'll probably want to do > this > > change as a separate CL since it'll involve changing a lot of the existing > > tests. > > SGTM. I'll file a fixit bug for that. https://crbug.com/546060 filed.
On 2015/10/21 17:27:20, mmenke wrote: > On 2015/10/21 17:22:57, mmenke wrote: > > On 2015/10/21 17:09:55, svaldez wrote: > > > On 2015/10/21 16:43:26, mmenke wrote: > > > > On 2015/10/21 16:39:01, mmenke wrote: > > > > > On 2015/10/20 13:57:33, svaldez wrote: > > > > > > On 2015/10/19 22:14:41, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > > > > File net/test/embedded_test_server/embedded_test_server.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > > > > > > net/test/embedded_test_server/embedded_test_server.cc:245: void > > > > > > > EmbeddedTestServer::AddDefaultHandlers(const base::FilePath& > > directory) > > > { > > > > > > > On 2015/10/19 21:56:14, svaldez wrote: > > > > > > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > > > > > > Maybe put DCHECK(!io_thread_.get()); in all of these handler > > > > > registration > > > > > > > > > methods? Or could use the stronger DCHECK(!Started()); > > > > > > > > > > > > > > > > Its safe to call this after we've started the server. > > > > > > > > > > > > > > But it's not safe not after we've started the IOThread. > > > > > > > > > > > > Hmm, so while its not technically Thread-safe in general, its safe to > > call > > > > as > > > > > > long as we aren't currently serving any requests. If we require that > the > > > IO > > > > > > Thread hasn't been started yet, this means that a lot of tests won't > be > > > able > > > > > to > > > > > > start the ETS as part of the harness, since they setup their own > request > > > > > > handlers inside the test bodies. For example the EmbeddedTestServer > > > > unittests > > > > > do > > > > > > this. > > > > > > > > > > Sorry for the latency, took yesterday off. > > > > > > > > > > While there presumably is a memory barrier somewhere between when we > > install > > > > the > > > > > hooks when when it starts receiving data, it's not something I want to > > rely > > > > on. > > > > > Also, if we're running tests in parallel on the system, who's to say > some > > > > > throwaway request won't end up trying to talk to us. > > > > > > > > > > I'm fine with adding mutexes to do this safely (I also don't think > > > > > PostTaskAndReply with a nested message loop is something we really want > to > > > do > > > > > here). > > > > > > > > As I think about this...I'm not really sure we want to support this usage. > > > It > > > > seems to encourage writing flaky tests. Test fixtures can still start the > > > test, > > > > as long as they install all needed hooks in advance, anyways. > > > > > > Hmm, looking at the crashes that result, I think we'll probably want to do > > this > > > change as a separate CL since it'll involve changing a lot of the existing > > > tests. > > > > SGTM. I'll file a fixit bug for that. > > https://crbug.com/546060 filed. Added a comment with the bug.
Two more responses. Doing another pass now. https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_request.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_request.cc:107: if (url.is_valid()) { On 2015/10/19 21:56:15, svaldez wrote: > On 2015/10/19 18:07:40, mmenke wrote: > > If we have a full URL, surely HttpRequest::ToURL should return it? (And I'd > > suggest renaming ToURL to just GetURL or URL()). > > There are cases where the "full URL" is actually not a valid URL pointing to the > ETS. Such as? https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... File net/test/embedded_test_server/http_response.cc (right): https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... net/test/embedded_test_server/http_response.cc:39: content_.size()); On 2015/10/19 21:56:15, svaldez wrote: > On 2015/10/19 18:07:41, mmenke wrote: > > Why don't we send a content-length when when the content size is 0? > > For responses that are only sending headers back, having a Content-Length causes > the test to attempt to parse a body and fail when one isn't expected. I'm not following. Our network stack handles Content-Lengths of 0.
I'm re-running the mass test change CL to see if I can track down the actual tests that hit both of these cases. On 2015/10/21 18:45:23, mmenke wrote: > Two more responses. Doing another pass now. > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > File net/test/embedded_test_server/http_request.cc (right): > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > net/test/embedded_test_server/http_request.cc:107: if (url.is_valid()) { > On 2015/10/19 21:56:15, svaldez wrote: > > On 2015/10/19 18:07:40, mmenke wrote: > > > If we have a full URL, surely HttpRequest::ToURL should return it? (And I'd > > > suggest renaming ToURL to just GetURL or URL()). > > > > There are cases where the "full URL" is actually not a valid URL pointing to > the > > ETS. > > Such as? "GET http://test.com/echo HTTP/1.1" I don't remember which test used something similar to this, but I believe it was regarding testing protocol line vs Host header behavior. > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > File net/test/embedded_test_server/http_response.cc (right): > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > net/test/embedded_test_server/http_response.cc:39: content_.size()); > On 2015/10/19 21:56:15, svaldez wrote: > > On 2015/10/19 18:07:41, mmenke wrote: > > > Why don't we send a content-length when when the content size is 0? > > > > For responses that are only sending headers back, having a Content-Length > causes > > the test to attempt to parse a body and fail when one isn't expected. > > I'm not following. Our network stack handles Content-Lengths of 0. I'll take a look around, but there were a few tests that failed because of this.
On 2015/10/21 19:37:09, svaldez wrote: > I'm re-running the mass test change CL to see if I can track down the actual > tests that hit both of these cases. > > On 2015/10/21 18:45:23, mmenke wrote: > > Two more responses. Doing another pass now. > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > File net/test/embedded_test_server/http_request.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > net/test/embedded_test_server/http_request.cc:107: if (url.is_valid()) { > > On 2015/10/19 21:56:15, svaldez wrote: > > > On 2015/10/19 18:07:40, mmenke wrote: > > > > If we have a full URL, surely HttpRequest::ToURL should return it? (And > I'd > > > > suggest renaming ToURL to just GetURL or URL()). > > > > > > There are cases where the "full URL" is actually not a valid URL pointing to > > the > > > ETS. > > > > Such as? > "GET http://test.com/echo HTTP/1.1" > I don't remember which test used something similar to this, but I believe it was > regarding testing protocol line vs Host header behavior. > > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > File net/test/embedded_test_server/http_response.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/580001/net/test/embedded_test... > > net/test/embedded_test_server/http_response.cc:39: content_.size()); > > On 2015/10/19 21:56:15, svaldez wrote: > > > On 2015/10/19 18:07:41, mmenke wrote: > > > > Why don't we send a content-length when when the content size is 0? > > > > > > For responses that are only sending headers back, having a Content-Length > > causes > > > the test to attempt to parse a body and fail when one isn't expected. > > > > I'm not following. Our network stack handles Content-Lengths of 0. > I'll take a look around, but there were a few tests that failed because of this. Thanks! Once we figure out what breaks, can file more bugs, if appropriate.
kelvinp@chromium.org changed reviewers: + sergeyu@chromium.org
+sergeyu for remoting review
Think this is looking really good - we're just about there. I'll plan to do a pass on the other part of this tomorrow, and hopefully a final pass on this as well. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:391: private: https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:407: base::WeakPtrFactory<InfiniteResponse> weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:407: base::WeakPtrFactory<InfiniteResponse> weak_ptr_factory_; include weak_ptr. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:410: scoped_ptr<HttpResponse> HandleInfiniteRequest(const HttpRequest& request) { Just use HandlePrefixedRequest instead? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:416: TEST_P(EmbeddedTestServerTest, CancelDuringRead) { Cancel -> Close, Read -> Write? (From the server's standpoint, the socket is closed in the middle of a write) https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:416: TEST_P(EmbeddedTestServerTest, CancelDuringRead) { Think this test is worth a comment. Something along the lines of "Tests the case the connection is closed while the server is sending a response. May non-deterministically end up at one of three paths (Discover the close event synchronously, asynchronously, or server shutting down before it is discovered)." Unfortunate that we can't be more deterministic here, but I don't see a great way to do that. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:423: server_->GetURL("/"), DEFAULT_PRIORITY, &cancel_delegate); /infinite https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:425: base::RunLoop().RunUntilIdle(); This doesn't run long enough. Use a TestDelegate and call set_cancel_in_response_started(test) on it. Then use base::RunLoop().Run(); Can also check the response code the request received, out of paranoia. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/http_request.h (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/http_request.h:41: GURL ToURL() const; Should mention that the returned URL is currently bogus, and only intended as a convenience function for parsing the path and query string. With a TODO to fix that. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/http_request.h:41: GURL ToURL() const; Nit: ToURL->GetURL (The request can't be uniquely converted to a URL, so the "To" seems a bit misleading). https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:11: #include <string> map, string should be in the header, per comment in other file. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:14: #include "base/files/file_path.h" Already in header https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:77: response = headers_ + "\r\n" + contents_; This seems kinda weird, if headers_ is empty but contents_ is not. Then we'd be returning an HTTP/0.9 response, with a bonus CRLF as the first characters of the body. Maybe: if (!headers_.empty()) response = headers_ + "\r\n" + contents_; else response = contents_; That gives us HTTP/0.9 support and sane handling of requests with contents but no headers. Or could just DCHECK in the constructor that if contents_ is non-null, neither is headers_. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:111: for (auto replacement : text_to_replace) { const auto& is preferred, I believe. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:118: if (first_query_parameter) { nit: Maybe get rid of this variable and use new_file_path.empty()? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:137: // TODO(svaldez): Figure out why restriction is necessary. Think "why restriction is necessary" -> "why thread is I/O restricted in the first place" is clearer. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:140: std::string relative_path(request.relative_url); Not needed - can you overwrite it unconditionally a couple lines down. Can just get rid of this line and declare / initialize this there. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:152: return nullptr; Suggest moving this above setting relative_path. Doesn't really matter, just seems weird to calculate the path and then bail. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:168: for (auto header : query["expected_headers"]) { const auto& https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:172: std::string value = header.substr(header.find(":") + 1); Hrm...Thought I commented on this. If header.find(":") is npos, what does this do? Use the same value as key and value? That seems not great. Same for the base::Base64Decode logic. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:180: // Trim the first byte ('/'). DCHECK that relative_path starts with "/"? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:194: for (auto replacement : query["replace_text"]) { const auto& https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:214: return http_response.Pass(); optional nit: Could just do: return make_scoped_ptr( new CustomHttpResponse(headers_contents, file_contents)); https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:223: if (ranges.size() == 1) { Can we fail if size() is not 1, or if ParseRangeHeader returns false? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.h (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:6: #define NET_TEST_EMBEDDED_TEST_SERVER_REQUEST_HELPERS_H_ nit: "_util.h" is a more common suffix for a files containing a bunch of related useful methods. Maybe call this request_handler_util.h, since these are all related to request handlers? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:8: #include <utility> What's this needed for? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:8: #include <utility> <string>, <map>, and <vector> all seem to be needed by this file. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:11: #include "base/macros.h" base/memory/scoped_ptr https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:16: #include "url/gurl.h" HttpRequest and GURL can be forward declared, and the includes moved to the cc file, I believe. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:30: ~CustomHttpResponse() override{}; nit: de-inline this. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:60: std::string* replacement_path); On 2015/10/19 21:56:15, svaldez wrote: > On 2015/10/19 18:07:41, mmenke wrote: > > Maybe make this a method on the test server? > > > > GURL GetURLWithReplacements(original_path, text_to_replace);? > > > > Also think the description could be clearer. > > This should probably be bound to HandleFileRequest, since its only applicable to > that handler, so it seems better to keep it here. There are two problems here: 1) It's not clear that this is only for use for files served by HandleFileRequest. 2) It's not clear that the files served by the embedded test server default server *are* served the HandleFileRequest. Putting this on the test server solves 2), but not 1). A stronger comment making it clear that this is only for files handled by HandleFileRequest solves 1), but leaves you hanging on 2. Most consumers won't be using HandleFileRequest directly, so I think the association with the embedded server's default handlers and discoverability are the bigger issues. Regardless, at least need a clearer statement saying this is only for use with HandleFileRequest. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:60: std::string* replacement_path); Suggest just returning a string - we're doing a final copy on the result, currently, anyways, so don't think we really gain anything by using a pointer. It also removes the concern about whether original_path can be the same value as replacement_path.
https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:26: SendCompleteCallback write_done)>; nit: const SendCompleteCallback&
remoting lgtm
svaldez@chromium.org changed required reviewers: - kelvinp@chromium.org
Updated. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:391: On 2015/10/21 20:21:36, mmenke wrote: > private: Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:407: base::WeakPtrFactory<InfiniteResponse> weak_ptr_factory_; On 2015/10/21 20:21:36, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:407: base::WeakPtrFactory<InfiniteResponse> weak_ptr_factory_; On 2015/10/21 20:21:37, mmenke wrote: > include weak_ptr. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:410: scoped_ptr<HttpResponse> HandleInfiniteRequest(const HttpRequest& request) { On 2015/10/21 20:21:37, mmenke wrote: > Just use HandlePrefixedRequest instead? Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:416: TEST_P(EmbeddedTestServerTest, CancelDuringRead) { On 2015/10/21 20:21:36, mmenke wrote: > Cancel -> Close, Read -> Write? (From the server's standpoint, the socket is > closed in the middle of a write) Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:416: TEST_P(EmbeddedTestServerTest, CancelDuringRead) { On 2015/10/21 20:21:37, mmenke wrote: > Think this test is worth a comment. Something along the lines of "Tests the > case the connection is closed while the server is sending a response. May > non-deterministically end up at one of three paths (Discover the close event > synchronously, asynchronously, or server shutting down before it is > discovered)." Unfortunate that we can't be more deterministic here, but I don't > see a great way to do that. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:423: server_->GetURL("/"), DEFAULT_PRIORITY, &cancel_delegate); On 2015/10/21 20:21:36, mmenke wrote: > /infinite Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:425: base::RunLoop().RunUntilIdle(); On 2015/10/21 20:21:36, mmenke wrote: > This doesn't run long enough. > > Use a TestDelegate and call set_cancel_in_response_started(test) on it. Then > use base::RunLoop().Run(); > > Can also check the response code the request received, out of paranoia. Not sure I understand. We still need to have some artificial delay using PostDelayedTask, since otherwise the ETS might shutdown right away? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/http_request.h (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/http_request.h:41: GURL ToURL() const; On 2015/10/21 20:21:37, mmenke wrote: > Nit: ToURL->GetURL (The request can't be uniquely converted to a URL, so the > "To" seems a bit misleading). Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/http_request.h:41: GURL ToURL() const; On 2015/10/21 20:21:37, mmenke wrote: > Should mention that the returned URL is currently bogus, and only intended as a > convenience function for parsing the path and query string. With a TODO to fix > that. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/http_response.h (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/http_response.h:26: SendCompleteCallback write_done)>; On 2015/10/21 20:30:39, mmenke wrote: > nit: const SendCompleteCallback& Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:11: #include <string> On 2015/10/21 20:21:37, mmenke wrote: > map, string should be in the header, per comment in other file. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:14: #include "base/files/file_path.h" On 2015/10/21 20:21:37, mmenke wrote: > Already in header Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:77: response = headers_ + "\r\n" + contents_; On 2015/10/21 20:21:37, mmenke wrote: > This seems kinda weird, if headers_ is empty but contents_ is not. Then we'd be > returning an HTTP/0.9 response, with a bonus CRLF as the first characters of the > body. > > Maybe: > > if (!headers_.empty()) > response = headers_ + "\r\n" + contents_; > else > response = contents_; > > That gives us HTTP/0.9 support and sane handling of requests with contents but > no headers. > > Or could just DCHECK in the constructor that if contents_ is non-null, neither > is headers_. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:111: for (auto replacement : text_to_replace) { On 2015/10/21 20:21:37, mmenke wrote: > const auto& is preferred, I believe. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:118: if (first_query_parameter) { On 2015/10/21 20:21:37, mmenke wrote: > nit: Maybe get rid of this variable and use new_file_path.empty()? Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:137: // TODO(svaldez): Figure out why restriction is necessary. On 2015/10/21 20:21:37, mmenke wrote: > Think "why restriction is necessary" -> "why thread is I/O restricted in the > first place" is clearer. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:140: std::string relative_path(request.relative_url); On 2015/10/21 20:21:37, mmenke wrote: > Not needed - can you overwrite it unconditionally a couple lines down. Can just > get rid of this line and declare / initialize this there. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:152: return nullptr; On 2015/10/21 20:21:37, mmenke wrote: > Suggest moving this above setting relative_path. Doesn't really matter, just > seems weird to calculate the path and then bail. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:168: for (auto header : query["expected_headers"]) { On 2015/10/21 20:21:37, mmenke wrote: > const auto& Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:172: std::string value = header.substr(header.find(":") + 1); On 2015/10/21 20:21:37, mmenke wrote: > Hrm...Thought I commented on this. If header.find(":") is npos, what does this > do? Use the same value as key and value? That seems not great. > > Same for the base::Base64Decode logic. But it won't be since we return a failure a few lines above? https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:180: // Trim the first byte ('/'). On 2015/10/21 20:21:37, mmenke wrote: > DCHECK that relative_path starts with "/"? Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:194: for (auto replacement : query["replace_text"]) { On 2015/10/21 20:21:37, mmenke wrote: > const auto& Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:214: return http_response.Pass(); On 2015/10/21 20:21:37, mmenke wrote: > optional nit: Could just do: > > return make_scoped_ptr( > new CustomHttpResponse(headers_contents, file_contents)); Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:223: if (ranges.size() == 1) { On 2015/10/21 20:21:37, mmenke wrote: > Can we fail if size() is not 1, or if ParseRangeHeader returns false? I think we should be ignoring Range we don't support and returning the full body, as per spec. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.h (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:6: #define NET_TEST_EMBEDDED_TEST_SERVER_REQUEST_HELPERS_H_ On 2015/10/21 20:21:37, mmenke wrote: > nit: "_util.h" is a more common suffix for a files containing a bunch of > related useful methods. Maybe call this request_handler_util.h, since these are > all related to request handlers? Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:8: #include <utility> On 2015/10/21 20:21:37, mmenke wrote: > <string>, <map>, and <vector> all seem to be needed by this file. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:8: #include <utility> On 2015/10/21 20:21:37, mmenke wrote: > What's this needed for? Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:11: #include "base/macros.h" On 2015/10/21 20:21:37, mmenke wrote: > base/memory/scoped_ptr Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:16: #include "url/gurl.h" On 2015/10/21 20:21:37, mmenke wrote: > HttpRequest and GURL can be forward declared, and the includes moved to the cc > file, I believe. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:30: ~CustomHttpResponse() override{}; On 2015/10/21 20:21:37, mmenke wrote: > nit: de-inline this. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:60: std::string* replacement_path); On 2015/10/21 20:21:37, mmenke wrote: > On 2015/10/19 21:56:15, svaldez wrote: > > On 2015/10/19 18:07:41, mmenke wrote: > > > Maybe make this a method on the test server? > > > > > > GURL GetURLWithReplacements(original_path, text_to_replace);? > > > > > > Also think the description could be clearer. > > > > This should probably be bound to HandleFileRequest, since its only applicable > to > > that handler, so it seems better to keep it here. > > There are two problems here: > > 1) It's not clear that this is only for use for files served by > HandleFileRequest. > 2) It's not clear that the files served by the embedded test server default > server *are* served the HandleFileRequest. > > Putting this on the test server solves 2), but not 1). A stronger comment > making it clear that this is only for files handled by HandleFileRequest solves > 1), but leaves you hanging on 2. Most consumers won't be using > HandleFileRequest directly, so I think the association with the embedded > server's default handlers and discoverability are the bigger issues. > Regardless, at least need a clearer statement saying this is only for use with > HandleFileRequest. Done. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.h:60: std::string* replacement_path); On 2015/10/21 20:21:37, mmenke wrote: > Suggest just returning a string - we're doing a final copy on the result, > currently, anyways, so don't think we really gain anything by using a pointer. > It also removes the concern about whether original_path can be the same value as > replacement_path. Adding a TODO, since this is currently trying to maintain compatibility with the SpawnedTestServer version.
Quick responses. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:425: base::RunLoop().RunUntilIdle(); On 2015/10/21 21:22:28, svaldez wrote: > On 2015/10/21 20:21:36, mmenke wrote: > > This doesn't run long enough. > > > > Use a TestDelegate and call set_cancel_in_response_started(test) on it. Then > > use base::RunLoop().Run(); > > > > Can also check the response code the request received, out of paranoia. > > Not sure I understand. We still need to have some artificial delay using > PostDelayedTask, since otherwise the ETS might shutdown right away? You aren't even waiting for the response to be received - Depending on the platform, we may not even be establishing a connection, since you're just calling RunUntilIdle. https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... File net/test/embedded_test_server/request_helpers.cc (right): https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... net/test/embedded_test_server/request_helpers.cc:223: if (ranges.size() == 1) { On 2015/10/21 21:22:29, svaldez wrote: > On 2015/10/21 20:21:37, mmenke wrote: > > Can we fail if size() is not 1, or if ParseRangeHeader returns false? > > I think we should be ignoring Range we don't support and returning the full > body, as per spec. Good point - you should then include the result of HttpUtil::ParseRangeHeader in your if statement. If we can only parse part of the range, and the rest fails, should return the full result, too, no? HttpUtil::ParseRangeHeader has no guarantees of not modifying ranges on failure.
On 2015/10/21 21:36:38, mmenke wrote: > Quick responses. > > https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... > File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): > > https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... > net/test/embedded_test_server/embedded_test_server_unittest.cc:425: > base::RunLoop().RunUntilIdle(); > On 2015/10/21 21:22:28, svaldez wrote: > > On 2015/10/21 20:21:36, mmenke wrote: > > > This doesn't run long enough. > > > > > > Use a TestDelegate and call set_cancel_in_response_started(test) on it. > Then > > > use base::RunLoop().Run(); > > > > > > Can also check the response code the request received, out of paranoia. > > > > Not sure I understand. We still need to have some artificial delay using > > PostDelayedTask, since otherwise the ETS might shutdown right away? > > You aren't even waiting for the response to be received - Depending on the > platform, we may not even be establishing a connection, since you're just > calling RunUntilIdle. Is the change to Run and keeping the PostDelayedTask sufficient for this? > > https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... > File net/test/embedded_test_server/request_helpers.cc (right): > > https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... > net/test/embedded_test_server/request_helpers.cc:223: if (ranges.size() == 1) { > On 2015/10/21 21:22:29, svaldez wrote: > > On 2015/10/21 20:21:37, mmenke wrote: > > > Can we fail if size() is not 1, or if ParseRangeHeader returns false? > > > > I think we should be ignoring Range we don't support and returning the full > > body, as per spec. > > Good point - you should then include the result of HttpUtil::ParseRangeHeader in > your if statement. If we can only parse part of the range, and the rest fails, > should return the full result, too, no? HttpUtil::ParseRangeHeader has no > guarantees of not modifying ranges on failure. Fixed.
On 2015/10/21 21:53:07, svaldez wrote: > On 2015/10/21 21:36:38, mmenke wrote: > > Quick responses. > > > > > https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... > > File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/680001/net/test/embedded_test... > > net/test/embedded_test_server/embedded_test_server_unittest.cc:425: > > base::RunLoop().RunUntilIdle(); > > On 2015/10/21 21:22:28, svaldez wrote: > > > On 2015/10/21 20:21:36, mmenke wrote: > > > > This doesn't run long enough. > > > > > > > > Use a TestDelegate and call set_cancel_in_response_started(test) on it. > > Then > > > > use base::RunLoop().Run(); > > > > > > > > Can also check the response code the request received, out of paranoia. > > > > > > Not sure I understand. We still need to have some artificial delay using > > > PostDelayedTask, since otherwise the ETS might shutdown right away? > > > > You aren't even waiting for the response to be received - Depending on the > > platform, we may not even be establishing a connection, since you're just > > calling RunUntilIdle. > Is the change to Run and keeping the PostDelayedTask sufficient for this? That should be sufficient. You should use the closure from the same RunLoop you're calling Run() on, though.
Think this is my last pass. https://codereview.chromium.org/1376593007/diff/730001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1376593007/diff/730001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:124: }; optional: Maybe just use CustomHttpResponse("", "");? And also rename the class to sound more general, and move it to http_response.h? RawHttpResponse or somesuch? Same goes for the other files that do this exact same thing. Don't think we want everyone to roll their own code to return an empty response. https://codereview.chromium.org/1376593007/diff/730001/components/cronet/andr... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/730001/components/cronet/andr... components/cronet/android/test/native_test_server.cc:54: class CustomHttpResponse : public net::test_server::HttpResponse { Suggest renaming this...There's no collision, but since you're adding a public class with this name, I think that's confusing. Alternatively, rename the other, per suggestion in other file. https://codereview.chromium.org/1376593007/diff/730001/content/public/test/br... File content/public/test/browser_test_base.h (right): https://codereview.chromium.org/1376593007/diff/730001/content/public/test/br... content/public/test/browser_test_base.h:11: #include "net/test/embedded_test_server/embedded_test_server.h" Why this change? https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; This only makes sense when used with the embedded test server, right? Doesn't seem like it belongs in code outside of net/test/. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:152: for (auto handler : request_handlers_) { const auto& handler, here and below? https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:258: // after the server has started. crbug.com/546060 give full https:// URLs (Allows clicking from UIs that support that, and avoids http) https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:265: // after the server has started. crbug.com/546060 https:// https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:378: CancelRequestDelegate() {} classes with virtual methods should have virtual desctructors. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:390: private: https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:391: base::RunLoop run_loop_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:396: InfiniteResponse() : weak_ptr_factory_(this){}; Remove colon, add space before {} https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:531: EmbeddedTestServer::Type type_; nit: const (Looks like can be added to the other two as well). https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:555: Should we be testing the certs with failures here?
https://codereview.chromium.org/1376593007/diff/730001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1376593007/diff/730001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:124: }; On 2015/10/22 19:28:27, mmenke wrote: > optional: Maybe just use CustomHttpResponse("", "");? And also rename the > class to sound more general, and move it to http_response.h? RawHttpResponse or > somesuch? Same goes for the other files that do this exact same thing. > > Don't think we want everyone to roll their own code to return an empty response. Done. https://codereview.chromium.org/1376593007/diff/730001/components/cronet/andr... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/730001/components/cronet/andr... components/cronet/android/test/native_test_server.cc:54: class CustomHttpResponse : public net::test_server::HttpResponse { On 2015/10/22 19:28:27, mmenke wrote: > Suggest renaming this...There's no collision, but since you're adding a public > class with this name, I think that's confusing. Alternatively, rename the > other, per suggestion in other file. Removing and implementing as RawHttpResponse. https://codereview.chromium.org/1376593007/diff/730001/content/public/test/br... File content/public/test/browser_test_base.h (right): https://codereview.chromium.org/1376593007/diff/730001/content/public/test/br... content/public/test/browser_test_base.h:11: #include "net/test/embedded_test_server/embedded_test_server.h" On 2015/10/22 19:28:27, mmenke wrote: > Why this change? Since we can't forward declare EmbeddedTestServer until we fully move it out of net::test_server. https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; On 2015/10/22 19:28:27, mmenke wrote: > This only makes sense when used with the embedded test server, right? Doesn't > seem like it belongs in code outside of net/test/. SSLServerSocket will need to know about the ServerCertificate it should be using for more complicated values of ServerCertificate (CERT_AUTO where it generates a new cert for OCSP testing) https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:152: for (auto handler : request_handlers_) { On 2015/10/22 19:28:27, mmenke wrote: > const auto& handler, here and below? Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:258: // after the server has started. crbug.com/546060 On 2015/10/22 19:28:27, mmenke wrote: > give full https:// URLs (Allows clicking from UIs that support that, and avoids > http) Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:265: // after the server has started. crbug.com/546060 On 2015/10/22 19:28:27, mmenke wrote: > https:// Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:378: CancelRequestDelegate() {} On 2015/10/22 19:28:27, mmenke wrote: > classes with virtual methods should have virtual desctructors. Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:390: On 2015/10/22 19:28:28, mmenke wrote: > private: Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:391: base::RunLoop run_loop_; On 2015/10/22 19:28:27, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:396: InfiniteResponse() : weak_ptr_factory_(this){}; On 2015/10/22 19:28:27, mmenke wrote: > Remove colon, add space before {} Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:531: EmbeddedTestServer::Type type_; On 2015/10/22 19:28:27, mmenke wrote: > nit: const (Looks like can be added to the other two as well). Done. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:555: On 2015/10/22 19:28:28, mmenke wrote: > Should we be testing the certs with failures here? We probably don't want to mix SSLServerSocket testing too closely with EmbeddedTestServer testing.
https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; On 2015/10/22 20:00:17, svaldez wrote: > On 2015/10/22 19:28:27, mmenke wrote: > > This only makes sense when used with the embedded test server, right? Doesn't > > seem like it belongs in code outside of net/test/. > > SSLServerSocket will need to know about the ServerCertificate it should be using > for more complicated values of ServerCertificate (CERT_AUTO where it generates a > new cert for OCSP testing) So our production code will have a way of generating bad certs? Happy to defer to David on this part of the review - I'm not at all familiar with the SSL code, or where it's going. https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:555: On 2015/10/22 20:00:17, svaldez wrote: > On 2015/10/22 19:28:28, mmenke wrote: > > Should we be testing the certs with failures here? > > We probably don't want to mix SSLServerSocket testing too closely with > EmbeddedTestServer testing. Currently EmbeddedTestServer has a bunch of hard-coded certificate paths, though, which presumably should be tested, so it's clear if they're broken or other tests are.
On 2015/10/22 20:04:34, mmenke wrote: > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > File net/ssl/ssl_server_config.h (right): > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; > On 2015/10/22 20:00:17, svaldez wrote: > > On 2015/10/22 19:28:27, mmenke wrote: > > > This only makes sense when used with the embedded test server, right? > Doesn't > > > seem like it belongs in code outside of net/test/. > > > > SSLServerSocket will need to know about the ServerCertificate it should be > using > > for more complicated values of ServerCertificate (CERT_AUTO where it generates > a > > new cert for OCSP testing) > > So our production code will have a way of generating bad certs? Happy to defer > to David on this part of the review - I'm not at all familiar with the SSL code, > or where it's going. The uses for SSLServerSocket is currently just the chromoting protocol and EmbeddedTestServer, with an eventual goal of moving the few uses of net/server to also support this. I think we'll eventually want SSLServerSocket to directly take the ServerCertificate enum and load the appropriate cert instead of having that handled by the user of SSLServerSocket, though I think that should probably wait until the refactoring of SSLServerSocket to wire up more options for the SSL Server. (Eventually matching the options available through SpawnedTestServer) > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > net/test/embedded_test_server/embedded_test_server_unittest.cc:555: > On 2015/10/22 20:00:17, svaldez wrote: > > On 2015/10/22 19:28:28, mmenke wrote: > > > Should we be testing the certs with failures here? > > > > We probably don't want to mix SSLServerSocket testing too closely with > > EmbeddedTestServer testing. > > Currently EmbeddedTestServer has a bunch of hard-coded certificate paths, > though, which presumably should be tested, so it's clear if they're broken or > other tests are. On the server side, we should be handling all the "broken" certificates identically, the only thing we can really check that differs is how Chromium handles bad certs on the client side, and we already have tests that check this through URLRequest and SSLClientSocket unittests. We could repeat those tests in this file, but it seems redundant and doesn't actually show us problems with EmbeddedTestServer's implementation compared to Chromium's handling of invalid certs.
On 2015/10/22 20:31:57, svaldez wrote: > On 2015/10/22 20:04:34, mmenke wrote: > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > File net/ssl/ssl_server_config.h (right): > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; > > On 2015/10/22 20:00:17, svaldez wrote: > > > On 2015/10/22 19:28:27, mmenke wrote: > > > > This only makes sense when used with the embedded test server, right? > > Doesn't > > > > seem like it belongs in code outside of net/test/. > > > > > > SSLServerSocket will need to know about the ServerCertificate it should be > > using > > > for more complicated values of ServerCertificate (CERT_AUTO where it > generates > > a > > > new cert for OCSP testing) > > > > So our production code will have a way of generating bad certs? Happy to > defer > > to David on this part of the review - I'm not at all familiar with the SSL > code, > > or where it's going. > The uses for SSLServerSocket is currently just the chromoting protocol and > EmbeddedTestServer, with an eventual goal of moving the few uses of net/server > to also support this. I think we'll eventually want SSLServerSocket to directly > take the ServerCertificate enum and load the appropriate cert instead of having > that handled by the user of SSLServerSocket, though I think that should probably > wait until the refactoring of SSLServerSocket to wire up more options for the > SSL Server. (Eventually matching the options available through > SpawnedTestServer) > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > net/test/embedded_test_server/embedded_test_server_unittest.cc:555: > > On 2015/10/22 20:00:17, svaldez wrote: > > > On 2015/10/22 19:28:28, mmenke wrote: > > > > Should we be testing the certs with failures here? > > > > > > We probably don't want to mix SSLServerSocket testing too closely with > > > EmbeddedTestServer testing. > > > > Currently EmbeddedTestServer has a bunch of hard-coded certificate paths, > > though, which presumably should be tested, so it's clear if they're broken or > > other tests are. > On the server side, we should be handling all the "broken" certificates > identically, the only thing we can really check that differs is how Chromium > handles bad certs on the client side, and we already have tests that check this > through URLRequest and SSLClientSocket unittests. We could repeat those tests in > this file, but it seems redundant and doesn't actually show us problems with > EmbeddedTestServer's implementation compared to Chromium's handling of invalid > certs. It's not the EmbeddedTestServer's implementation of them that matters, but that they're hooked up to it correctly, it's using the correct paths, etc. When a Chromium test fails, it's unclear if it's the feature being tested that's failing, or the test server. Tests here would make it clear when it's a test server (Like if one of the test server's certs expires, or isn't being copied to the device, etc).
On 2015/10/22 20:38:05, mmenke wrote: > On 2015/10/22 20:31:57, svaldez wrote: > > On 2015/10/22 20:04:34, mmenke wrote: > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > > File net/ssl/ssl_server_config.h (right): > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > > net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; > > > On 2015/10/22 20:00:17, svaldez wrote: > > > > On 2015/10/22 19:28:27, mmenke wrote: > > > > > This only makes sense when used with the embedded test server, right? > > > Doesn't > > > > > seem like it belongs in code outside of net/test/. > > > > > > > > SSLServerSocket will need to know about the ServerCertificate it should be > > > using > > > > for more complicated values of ServerCertificate (CERT_AUTO where it > > generates > > > a > > > > new cert for OCSP testing) > > > > > > So our production code will have a way of generating bad certs? Happy to > > defer > > > to David on this part of the review - I'm not at all familiar with the SSL > > code, > > > or where it's going. > > The uses for SSLServerSocket is currently just the chromoting protocol and > > EmbeddedTestServer, with an eventual goal of moving the few uses of net/server > > to also support this. I think we'll eventually want SSLServerSocket to > directly > > take the ServerCertificate enum and load the appropriate cert instead of > having > > that handled by the user of SSLServerSocket, though I think that should > probably > > wait until the refactoring of SSLServerSocket to wire up more options for the > > SSL Server. (Eventually matching the options available through > > SpawnedTestServer) > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > > File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > > net/test/embedded_test_server/embedded_test_server_unittest.cc:555: > > > On 2015/10/22 20:00:17, svaldez wrote: > > > > On 2015/10/22 19:28:28, mmenke wrote: > > > > > Should we be testing the certs with failures here? > > > > > > > > We probably don't want to mix SSLServerSocket testing too closely with > > > > EmbeddedTestServer testing. > > > > > > Currently EmbeddedTestServer has a bunch of hard-coded certificate paths, > > > though, which presumably should be tested, so it's clear if they're broken > or > > > other tests are. > > On the server side, we should be handling all the "broken" certificates > > identically, the only thing we can really check that differs is how Chromium > > handles bad certs on the client side, and we already have tests that check > this > > through URLRequest and SSLClientSocket unittests. We could repeat those tests > in > > this file, but it seems redundant and doesn't actually show us problems with > > EmbeddedTestServer's implementation compared to Chromium's handling of invalid > > certs. > > It's not the EmbeddedTestServer's implementation of them that matters, but that > they're hooked up to it correctly, it's using the correct paths, etc. When a > Chromium test fails, it's unclear if it's the feature being tested that's > failing, or the test server. Tests here would make it clear when it's a test > server (Like if one of the test server's certs expires, or isn't being copied to > the device, etc). Is it sufficient to check that the certificates have values we expect in the X509Certificate that we are installing?
On 2015/10/22 20:45:41, svaldez wrote: > On 2015/10/22 20:38:05, mmenke wrote: > > On 2015/10/22 20:31:57, svaldez wrote: > > > On 2015/10/22 20:04:34, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > > > File net/ssl/ssl_server_config.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > > > net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; > > > > On 2015/10/22 20:00:17, svaldez wrote: > > > > > On 2015/10/22 19:28:27, mmenke wrote: > > > > > > This only makes sense when used with the embedded test server, right? > > > > Doesn't > > > > > > seem like it belongs in code outside of net/test/. > > > > > > > > > > SSLServerSocket will need to know about the ServerCertificate it should > be > > > > using > > > > > for more complicated values of ServerCertificate (CERT_AUTO where it > > > generates > > > > a > > > > > new cert for OCSP testing) > > > > > > > > So our production code will have a way of generating bad certs? Happy to > > > defer > > > > to David on this part of the review - I'm not at all familiar with the SSL > > > code, > > > > or where it's going. > > > The uses for SSLServerSocket is currently just the chromoting protocol and > > > EmbeddedTestServer, with an eventual goal of moving the few uses of > net/server > > > to also support this. I think we'll eventually want SSLServerSocket to > > directly > > > take the ServerCertificate enum and load the appropriate cert instead of > > having > > > that handled by the user of SSLServerSocket, though I think that should > > probably > > > wait until the refactoring of SSLServerSocket to wire up more options for > the > > > SSL Server. (Eventually matching the options available through > > > SpawnedTestServer) > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > > > File net/test/embedded_test_server/embedded_test_server_unittest.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > > > net/test/embedded_test_server/embedded_test_server_unittest.cc:555: > > > > On 2015/10/22 20:00:17, svaldez wrote: > > > > > On 2015/10/22 19:28:28, mmenke wrote: > > > > > > Should we be testing the certs with failures here? > > > > > > > > > > We probably don't want to mix SSLServerSocket testing too closely with > > > > > EmbeddedTestServer testing. > > > > > > > > Currently EmbeddedTestServer has a bunch of hard-coded certificate paths, > > > > though, which presumably should be tested, so it's clear if they're broken > > or > > > > other tests are. > > > On the server side, we should be handling all the "broken" certificates > > > identically, the only thing we can really check that differs is how Chromium > > > handles bad certs on the client side, and we already have tests that check > > this > > > through URLRequest and SSLClientSocket unittests. We could repeat those > tests > > in > > > this file, but it seems redundant and doesn't actually show us problems with > > > EmbeddedTestServer's implementation compared to Chromium's handling of > invalid > > > certs. > > > > It's not the EmbeddedTestServer's implementation of them that matters, but > that > > they're hooked up to it correctly, it's using the correct paths, etc. When a > > Chromium test fails, it's unclear if it's the feature being tested that's > > failing, or the test server. Tests here would make it clear when it's a test > > server (Like if one of the test server's certs expires, or isn't being copied > to > > the device, etc). > > Is it sufficient to check that the certificates have values we expect in the > X509Certificate that we are installing? Sure, I think checking the values returned by GetCertificate() would be fine.
On 2015/10/22 20:48:50, mmenke wrote: > On 2015/10/22 20:45:41, svaldez wrote: > > On 2015/10/22 20:38:05, mmenke wrote: > > > On 2015/10/22 20:31:57, svaldez wrote: > > > > On 2015/10/22 20:04:34, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > > > > File net/ssl/ssl_server_config.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/ssl/ssl_server_con... > > > > > net/ssl/ssl_server_config.h:82: ServerCertificate server_cert; > > > > > On 2015/10/22 20:00:17, svaldez wrote: > > > > > > On 2015/10/22 19:28:27, mmenke wrote: > > > > > > > This only makes sense when used with the embedded test server, > right? > > > > > Doesn't > > > > > > > seem like it belongs in code outside of net/test/. > > > > > > > > > > > > SSLServerSocket will need to know about the ServerCertificate it > should > > be > > > > > using > > > > > > for more complicated values of ServerCertificate (CERT_AUTO where it > > > > generates > > > > > a > > > > > > new cert for OCSP testing) > > > > > > > > > > So our production code will have a way of generating bad certs? Happy > to > > > > defer > > > > > to David on this part of the review - I'm not at all familiar with the > SSL > > > > code, > > > > > or where it's going. > > > > The uses for SSLServerSocket is currently just the chromoting protocol and > > > > EmbeddedTestServer, with an eventual goal of moving the few uses of > > net/server > > > > to also support this. I think we'll eventually want SSLServerSocket to > > > directly > > > > take the ServerCertificate enum and load the appropriate cert instead of > > > having > > > > that handled by the user of SSLServerSocket, though I think that should > > > probably > > > > wait until the refactoring of SSLServerSocket to wire up more options for > > the > > > > SSL Server. (Eventually matching the options available through > > > > SpawnedTestServer) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > > > > File net/test/embedded_test_server/embedded_test_server_unittest.cc > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1376593007/diff/730001/net/test/embedded_test... > > > > > net/test/embedded_test_server/embedded_test_server_unittest.cc:555: > > > > > On 2015/10/22 20:00:17, svaldez wrote: > > > > > > On 2015/10/22 19:28:28, mmenke wrote: > > > > > > > Should we be testing the certs with failures here? > > > > > > > > > > > > We probably don't want to mix SSLServerSocket testing too closely with > > > > > > EmbeddedTestServer testing. > > > > > > > > > > Currently EmbeddedTestServer has a bunch of hard-coded certificate > paths, > > > > > though, which presumably should be tested, so it's clear if they're > broken > > > or > > > > > other tests are. > > > > On the server side, we should be handling all the "broken" certificates > > > > identically, the only thing we can really check that differs is how > Chromium > > > > handles bad certs on the client side, and we already have tests that check > > > this > > > > through URLRequest and SSLClientSocket unittests. We could repeat those > > tests > > > in > > > > this file, but it seems redundant and doesn't actually show us problems > with > > > > EmbeddedTestServer's implementation compared to Chromium's handling of > > invalid > > > > certs. > > > > > > It's not the EmbeddedTestServer's implementation of them that matters, but > > that > > > they're hooked up to it correctly, it's using the correct paths, etc. When > a > > > Chromium test fails, it's unclear if it's the feature being tested that's > > > failing, or the test server. Tests here would make it clear when it's a > test > > > server (Like if one of the test server's certs expires, or isn't being > copied > > to > > > the device, etc). > > > > Is it sufficient to check that the certificates have values we expect in the > > X509Certificate that we are installing? > > Sure, I think checking the values returned by GetCertificate() would be fine. Done.
LGTM! But do wait for David to sign off on the SSL stuff. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:377: class CancelRequestDelegate : public TestDelegate { All this stuff should be in an anonymous namespace. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:443: nit: Remove blank line. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:452: {SSLServerConfig::CERT_OK, false, "127.0.0.1", "Test Root CA"}, Can't have non-POD types in global scope - simplest just to switch from strings to char*. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:457: {SSLServerConfig::CERT_BAD_VALIDITY, true, "Leaf Certificate", "Test Root CA"}, Some of these liens are too long. Did you run git CL format?
https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:377: class CancelRequestDelegate : public TestDelegate { On 2015/10/22 21:26:58, mmenke wrote: > All this stuff should be in an anonymous namespace. Done. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:443: On 2015/10/22 21:26:58, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:452: {SSLServerConfig::CERT_OK, false, "127.0.0.1", "Test Root CA"}, On 2015/10/22 21:26:58, mmenke wrote: > Can't have non-POD types in global scope - simplest just to switch from strings > to char*. Done. https://codereview.chromium.org/1376593007/diff/790001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server_unittest.cc:457: {SSLServerConfig::CERT_BAD_VALIDITY, true, "Leaf Certificate", "Test Root CA"}, On 2015/10/22 21:26:58, mmenke wrote: > Some of these liens are too long. Did you run git CL format? Done.
lgtm with some small comments. I only looked at //net and mostly skimmed it for SSL-related bits, deferring the rest of Matt. Let me know if you'd like me to take a more thorough look instead. https://codereview.chromium.org/1376593007/diff/1030001/net/ssl/ssl_server_co... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/1030001/net/ssl/ssl_server_co... net/ssl/ssl_server_config.h:24: // (Use the SSL_PROTOCOL_VERSION_xxx enumerators defined in ssl_config.) Nit: ssl_config -> ssl_config.h https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.cc:245: ServeFilesFromDirectory(test_data_dir.AppendASCII(relative)); Should this CHECK() that PathService::Get succeeded? https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.cc:252: ServeFilesFromDirectory(test_data_dir.Append(relative)); Ditto. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.h:117: // |type| indicates the protocol type of the server. (HTTP/HTTPS) Nit: |type| indicates the protocol type of the server (HTTP/HTTPS). https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.h:184: // Returns the file name of the certificate the server is using. Nit: I'd maybe mention that the certificate may be found in <whatever>. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server_unittest.cc:198: if (server_->is_using_ssl()) { Nit: I think it's marginally better to condition GetParam() just so that your test notices if, for some reason, EmbeddedTestServer interprets TYPE_HTTPS wrong.
https://codereview.chromium.org/1376593007/diff/1030001/net/ssl/ssl_server_co... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1376593007/diff/1030001/net/ssl/ssl_server_co... net/ssl/ssl_server_config.h:24: // (Use the SSL_PROTOCOL_VERSION_xxx enumerators defined in ssl_config.) On 2015/10/26 21:30:08, davidben wrote: > Nit: ssl_config -> ssl_config.h Done. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.cc:245: ServeFilesFromDirectory(test_data_dir.AppendASCII(relative)); On 2015/10/26 21:30:08, davidben wrote: > Should this CHECK() that PathService::Get succeeded? Done. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.cc:252: ServeFilesFromDirectory(test_data_dir.Append(relative)); On 2015/10/26 21:30:08, davidben wrote: > Ditto. Done. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.h:117: // |type| indicates the protocol type of the server. (HTTP/HTTPS) On 2015/10/26 21:30:08, davidben wrote: > Nit: |type| indicates the protocol type of the server (HTTP/HTTPS). Done. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server.h:184: // Returns the file name of the certificate the server is using. On 2015/10/26 21:30:08, davidben wrote: > Nit: I'd maybe mention that the certificate may be found in <whatever>. Done. https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... File net/test/embedded_test_server/embedded_test_server_unittest.cc (right): https://codereview.chromium.org/1376593007/diff/1030001/net/test/embedded_tes... net/test/embedded_test_server/embedded_test_server_unittest.cc:198: if (server_->is_using_ssl()) { On 2015/10/26 21:30:08, davidben wrote: > Nit: I think it's marginally better to condition GetParam() just so that your > test notices if, for some reason, EmbeddedTestServer interprets TYPE_HTTPS > wrong. Done.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, sergeyu@chromium.org, mmenke@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1376593007/#ps1050001 (title: "Last few nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376593007/1050001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376593007/1050001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Description was changed from ========== SSL and add handlers in EmbeddedTestServer As part of the migration of tests to EmbeddedTestServer, this CL modifies EmbeddedTestServer to support SSLServerSocket and to add shared default handlers that can be used by tests to mirror the SpawnedTestServer handlers. The major changes are: - SSL support through SSLServerSocket - Adding SSLServerConfig to configure SSLServerSockets. - Setting up default handlers for BrowserTestBase tests. - Asynchronous HttpResponse. - Moving EmbeddedTestServer to the net:: namespace. BUG=496825 ========== to ========== SSL and add handlers in EmbeddedTestServer As part of the migration of tests to EmbeddedTestServer, this CL modifies EmbeddedTestServer to support SSLServerSocket and to add shared default handlers that can be used by tests to mirror the SpawnedTestServer handlers. The major changes are: - SSL support through SSLServerSocket - Adding SSLServerConfig to configure SSLServerSockets. - Setting up default handlers for BrowserTestBase tests. - Asynchronous HttpResponse. - Moving EmbeddedTestServer to the net:: namespace. BUG=496825 ==========
Driveby: wrapping description to 80 characters to make it easier to read in browser.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, davidben@chromium.org, lazyboy@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1376593007/#ps1070001 (title: "Typo fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376593007/1070001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376593007/1070001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== SSL and add handlers in EmbeddedTestServer As part of the migration of tests to EmbeddedTestServer, this CL modifies EmbeddedTestServer to support SSLServerSocket and to add shared default handlers that can be used by tests to mirror the SpawnedTestServer handlers. The major changes are: - SSL support through SSLServerSocket - Adding SSLServerConfig to configure SSLServerSockets. - Setting up default handlers for BrowserTestBase tests. - Asynchronous HttpResponse. - Moving EmbeddedTestServer to the net:: namespace. BUG=496825 ========== to ========== SSL and add handlers in EmbeddedTestServer As part of the migration of tests to EmbeddedTestServer, this CL modifies EmbeddedTestServer to support SSLServerSocket and to add shared default handlers that can be used by tests to mirror the SpawnedTestServer handlers. The major changes are: - SSL support through SSLServerSocket - Adding SSLServerConfig to configure SSLServerSockets. - Setting up default handlers for BrowserTestBase tests. - Asynchronous HttpResponse. - Moving EmbeddedTestServer to the net:: namespace. BUG=496825 NOPRESUBMIT=TRUE ==========
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/1376593007/1070001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376593007/1070001
Message was sent while issue was closed.
Committed patchset #55 (id:1070001)
Message was sent while issue was closed.
Patchset 55 (id:??) landed as https://crrev.com/5c265faa70626b7754e8b62300f65771dae2adbf Cr-Commit-Position: refs/heads/master@{#356305}
Message was sent while issue was closed.
A revert of this CL (patchset #55 id:1070001) has been created in https://codereview.chromium.org/1421903008/ by stevenjb@chromium.org. The reason for reverting is: This breaks cros_trunk, failure started here: http://master.chrome.corp.google.com:8011/builders/cros%20trunk/builds/40350 crbug.com/548358. |
