SSL in EmbeddedTestServer

net/test/embedded_test_server/request_helpers.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_TEST_EMBEDDED_TEST_SERVER_REQUEST_HELPERS_H_
+#define NET_TEST_EMBEDDED_TEST_SERVER_REQUEST_HELPERS_H_

[mmenke, 2015/10/21 20:21:37]

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?

[svaldez, 2015/10/21 21:22:29]

Done.

+
+#include <utility>

[mmenke, 2015/10/21 20:21:37]

<string>, <map>, and <vector> all seem to be needed by this file.

[mmenke, 2015/10/21 20:21:37]

What's this needed for?

[svaldez, 2015/10/21 21:22:29]

Done.

[svaldez, 2015/10/21 21:22:29]

Done.

+
+#include "base/files/file_path.h"
+#include "base/macros.h"

[mmenke, 2015/10/21 20:21:37]

base/memory/scoped_ptr

[svaldez, 2015/10/21 21:22:29]

Done.

+#include "base/strings/string_split.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "url/gurl.h"

[mmenke, 2015/10/21 20:21:37]

HttpRequest and GURL can be forward declared, and the includes moved to the cc
file, I believe.

[svaldez, 2015/10/21 21:22:29]

Done.

+
+namespace net {
+namespace test_server {
+
+// This file is only meant for compatibility with testserver.py. No
+// additional handlers should be added here that don't affect multiple
+// distinct tests.
+
+using RequestQuery = std::map<std::string, std::vector<std::string>>;
+
+class CustomHttpResponse : public HttpResponse {
+ public:
+  CustomHttpResponse(const std::string& headers, const std::string& contents);
+  ~CustomHttpResponse() override{};

[mmenke, 2015/10/21 20:21:37]

nit:  de-inline this.

[svaldez, 2015/10/21 21:22:29]

Done.

+
+  void SendResponse(const SendBytesCallback& send,
+                    const SendCompleteCallback& done) override;
+
+ private:
+  const std::string headers_;
+  const std::string contents_;
+
+  DISALLOW_COPY_AND_ASSIGN(CustomHttpResponse);
+};
+
+// Return whether |request| starts with a URL path of |url|.
+bool ShouldHandle(const HttpRequest& request, const std::string& prefix_path);
+
+// Calls |handler| if the |request| URL starts with |prefix|.
+scoped_ptr<HttpResponse> HandlePrefixedRequest(
+    const std::string& prefix,
+    const EmbeddedTestServer::HandleRequestCallback& handler,
+    const HttpRequest& request);
+
+// Parses |url| to get the query and places it into a map.
+RequestQuery ParseQuery(const GURL& url);
+
+// Returns a path that serves the contents of the file at |original_path|
+// with all the text matching the elements of |text_to_replace| replaced
+// with the corresponding values. The path is returned in |replacement_path|.
+// HandleFileRequest will handle the resulting path.
+void GetFilePathWithReplacements(const std::string& original_path,
+                                 const base::StringPairs& text_to_replace,
+                                 std::string* replacement_path);

[mmenke, 2015/10/21 20:21:37]

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.

[mmenke, 2015/10/21 20:21:37]

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.

[svaldez, 2015/10/21 21:22:29]

Done.

[svaldez, 2015/10/21 21:22:29]

Adding a TODO, since this is currently trying to maintain compatibility with the
SpawnedTestServer version.

+
+// Handles |request| by serving a file from under |server_root|.
+scoped_ptr<HttpResponse> HandleFileRequest(const base::FilePath& server_root,
+                                           const HttpRequest& request);
+
+}  // namespace test_server
+}  // namespace net
+
+#endif  // NET_TEST_EMBEDDED_TEST_SERVER_REQUEST_HELPERS_H_