Introduce Http{Request,Response} mojom structs.

Introduce Http{Request,Response} mojom structs.

The reason to stop using URL{Request,Response} for HTTP server interfaces: some
fields of those structs belong to a higher level, e.g., |auto_follow_redirects|.
It seems cleaner to define Http{Request,Response} which match the HTTP
request/response definitions.

BUG=478249
TEST=None

Committed: https://crrev.com/9e02055bbbf56c87f3bd27fb9c57607f8b9f515e
Cr-Commit-Position: refs/heads/master@{#330238}

[yzshen1, 2015-05-14 21:59:43]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org

[yzshen1, 2015-05-14 21:59:44]

Hi, John.

Would you please take a look? Thanks!

https://codereview.chromium.org/1129143005/diff/1/mojo/services/network/publi...
File mojo/services/network/public/interfaces/http_message.mojom (right):

https://codereview.chromium.org/1129143005/diff/1/mojo/services/network/publi...
mojo/services/network/public/interfaces/http_message.mojom:7: struct HTTPHeader
{
One issue that I haven't resolved is the naming inconsistency between
*HTTP*Header and *Http*Request/etc.

HTTPHeader was used by url_loader.mojom so changing it will affect existing
code.
However, it seems the convention of chrome repo is to use "Http" in class/method
names.

What do you think? Thanks!

[darin (slow to review), 2015-05-14 22:15:18]

We should either go with HTTP or Http universally. Mixing the two would be
unfortunate. We don't have UrlRequest, so it is perhaps odd to have
HttpRequest.

-Darin

On Thu, May 14, 2015 at 2:59 PM, <yzshen@chromium.org> wrote:

> Reviewers: jam,
>
> Message:
> Hi, John.
>
> Would you please take a look? Thanks!
>
>
>
>
https://codereview.chromium.org/1129143005/diff/1/mojo/services/network/publi...
> File mojo/services/network/public/interfaces/http_message.mojom (right):
>
>
>
https://codereview.chromium.org/1129143005/diff/1/mojo/services/network/publi...
> mojo/services/network/public/interfaces/http_message.mojom:7: struct
> HTTPHeader {
> One issue that I haven't resolved is the naming inconsistency between
> *HTTP*Header and *Http*Request/etc.
>
> HTTPHeader was used by url_loader.mojom so changing it will affect
> existing code.
> However, it seems the convention of chrome repo is to use "Http" in
> class/method names.
>
> What do you think? Thanks!
>
> Description:
> Introduce Http{Request,Response} mojom structs.
>
> The reason to stop using URL{Request,Response} for HTTP server interfaces:
> some
> fields of those structs belong to a higher level, e.g.,
> |auto_follow_redirects|.
> It seems cleaner to define Http{Request,Response} which match the HTTP
> request/response definitions.
>
> BUG=478249
> TEST=None
>
> Please review this at https://codereview.chromium.org/1129143005/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+47, -27 lines):
>   M mojo/services/network/http_connection_impl.h
>   M mojo/services/network/http_connection_impl.cc
>   M mojo/services/network/http_server_apptest.cc
>   M mojo/services/network/public/interfaces/BUILD.gn
>   M mojo/services/network/public/interfaces/http_connection.mojom
>   A mojo/services/network/public/interfaces/http_message.mojom
>   M mojo/services/network/public/interfaces/url_loader.mojom
>
>
> Index: mojo/services/network/http_connection_impl.cc
> diff --git a/mojo/services/network/http_connection_impl.cc
> b/mojo/services/network/http_connection_impl.cc
> index
>
24fc7c5de90c326d88650910fba7aa99854bb8ae..8fece71a5f2ec326ff592e747f775eb333129223
> 100644
> --- a/mojo/services/network/http_connection_impl.cc
> +++ b/mojo/services/network/http_connection_impl.cc
> @@ -82,11 +82,11 @@ class HttpConnectionImpl::SimpleDataPipeReader {
>  };
>
>  template <>
> -struct TypeConverter<URLRequestPtr, net::HttpServerRequestInfo> {
> -  static URLRequestPtr Convert(const net::HttpServerRequestInfo& obj) {
> -    URLRequestPtr request(URLRequest::New());
> -    request->url = obj.path;
> +struct TypeConverter<HttpRequestPtr, net::HttpServerRequestInfo> {
> +  static HttpRequestPtr Convert(const net::HttpServerRequestInfo& obj) {
> +    HttpRequestPtr request(HttpRequest::New());
>      request->method = obj.method;
> +    request->url = obj.path;
>      request->headers.resize(obj.headers.size());
>      size_t index = 0;
>      for (const auto& item : obj.headers) {
> @@ -103,7 +103,7 @@ struct TypeConverter<URLRequestPtr,
> net::HttpServerRequestInfo> {
>        options.element_num_bytes = 1;
>        options.capacity_num_bytes = num_bytes;
>        DataPipe data_pipe(options);
> -      request->body.push_back(data_pipe.consumer_handle.Pass());
> +      request->body = data_pipe.consumer_handle.Pass();
>        MojoResult result =
>            WriteDataRaw(data_pipe.producer_handle.get(), obj.data.data(),
>                         &num_bytes, MOJO_WRITE_DATA_FLAG_ALL_OR_NONE);
> @@ -138,7 +138,7 @@ void HttpConnectionImpl::OnReceivedHttpRequest(
>      return;
>
>    delegate_->OnReceivedRequest(
> -      URLRequest::From(info), [this](URLResponsePtr response) {
> +      HttpRequest::From(info), [this](HttpResponsePtr response) {
>          if (response->body.is_valid()) {
>            SimpleDataPipeReader* reader = new SimpleDataPipeReader;
>            response_body_readers_.insert(reader);
> @@ -198,7 +198,7 @@ void HttpConnectionImpl::OnConnectionError() {
>  }
>
>  void HttpConnectionImpl::OnFinishedReadingResponseBody(
> -    URLResponsePtr response,
> +    HttpResponsePtr response,
>      SimpleDataPipeReader* reader,
>      scoped_ptr<std::string> body) {
>    if (reader) {
> Index: mojo/services/network/http_connection_impl.h
> diff --git a/mojo/services/network/http_connection_impl.h
> b/mojo/services/network/http_connection_impl.h
> index
>
ec23fc0b292cbfacefa075a2babbc6219f42e5f7..f9d59c9ed769cdde745de2ad264a939169ba1203
> 100644
> --- a/mojo/services/network/http_connection_impl.h
> +++ b/mojo/services/network/http_connection_impl.h
> @@ -11,7 +11,7 @@
>  #include "base/macros.h"
>  #include "base/memory/scoped_ptr.h"
>  #include "mojo/services/network/public/interfaces/http_connection.mojom.h"
> -#include "mojo/services/network/public/interfaces/url_loader.mojom.h"
> +#include "mojo/services/network/public/interfaces/http_message.mojom.h"
>  #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h"
>  #include "third_party/mojo/src/mojo/public/cpp/bindings/error_handler.h"
>
> @@ -51,7 +51,7 @@ class HttpConnectionImpl : public HttpConnection,
>    // ErrorHandler implementation.
>    void OnConnectionError() override;
>
> -  void OnFinishedReadingResponseBody(URLResponsePtr response_ptr,
> +  void OnFinishedReadingResponseBody(HttpResponsePtr response_ptr,
>                                       SimpleDataPipeReader* reader,
>                                       scoped_ptr<std::string> body);
>
> Index: mojo/services/network/http_server_apptest.cc
> diff --git a/mojo/services/network/http_server_apptest.cc
> b/mojo/services/network/http_server_apptest.cc
> index
>
f6d35e0ab8a8ee5431ef2a6f569b17434484da75..9251eee9d7dbb67444459af986c18a363bdec2e7
> 100644
> --- a/mojo/services/network/http_server_apptest.cc
> +++ b/mojo/services/network/http_server_apptest.cc
> @@ -68,8 +68,8 @@ std::string MakeRequestMessage(const TestRequest& data) {
>    return message;
>  }
>
> -URLResponsePtr MakeResponseStruct(const TestResponse& data) {
> -  URLResponsePtr response(URLResponse::New());
> +HttpResponsePtr MakeResponseStruct(const TestResponse& data) {
> +  HttpResponsePtr response(HttpResponse::New());
>    response->status_code = data.status_code;
>    response->headers.resize(data.headers.size());
>    size_t index = 0;
> @@ -116,17 +116,17 @@ void CheckHeaders(const TestHeaders& expected,
>    }
>  }
>
> -void CheckRequest(const TestRequest& expected, URLRequestPtr request) {
> +void CheckRequest(const TestRequest& expected, HttpRequestPtr request) {
>    EXPECT_EQ(expected.method, request->method);
>    EXPECT_EQ(expected.url, request->url);
>    CheckHeaders(expected.headers, request->headers);
>    if (expected.body) {
> -    EXPECT_EQ(1u, request->body.size());
> +    EXPECT_TRUE(request->body.is_valid());
>      std::string body;
> -    common::BlockingCopyToString(request->body[0].Pass(), &body);
> +    common::BlockingCopyToString(request->body.Pass(), &body);
>      EXPECT_EQ(*expected.body, body);
>    } else {
> -    EXPECT_EQ(0u, request->body.size());
> +    EXPECT_FALSE(request->body.is_valid());
>    }
>  }
>
> @@ -264,7 +264,7 @@ class TestHttpClient {
>  class HttpConnectionDelegateImpl : public HttpConnectionDelegate {
>   public:
>    struct PendingRequest {
> -    URLRequestPtr request;
> +    HttpRequestPtr request;
>      OnReceivedRequestCallback callback;
>    };
>
> @@ -277,7 +277,7 @@ class HttpConnectionDelegateImpl : public
> HttpConnectionDelegate {
>    ~HttpConnectionDelegateImpl() override {}
>
>    // HttpConnectionDelegate implementation:
> -  void OnReceivedRequest(URLRequestPtr request,
> +  void OnReceivedRequest(HttpRequestPtr request,
>                           const OnReceivedRequestCallback& callback)
> override {
>      linked_ptr<PendingRequest> pending_request(new PendingRequest);
>      pending_request->request = request.Pass();
> @@ -290,12 +290,12 @@ class HttpConnectionDelegateImpl : public
> HttpConnectionDelegate {
>    }
>
>    void OnReceivedWebSocketRequest(
> -      URLRequestPtr request,
> +      HttpRequestPtr request,
>        const OnReceivedWebSocketRequestCallback& callback) override {
>      NOTREACHED();
>    }
>
> -  void SendResponse(URLResponsePtr response) {
> +  void SendResponse(HttpResponsePtr response) {
>      ASSERT_FALSE(pending_requests_.empty());
>      linked_ptr<PendingRequest> request = pending_requests_[0];
>      pending_requests_.erase(pending_requests_.begin());
> Index: mojo/services/network/public/interfaces/BUILD.gn
> diff --git a/mojo/services/network/public/interfaces/BUILD.gn
> b/mojo/services/network/public/interfaces/BUILD.gn
> index
>
9b5c50fff1f9e44924aba55bb6ae932c2f7501a1..79ddbdcf2b8334b548a485f72eccf1319053ec59
> 100644
> --- a/mojo/services/network/public/interfaces/BUILD.gn
> +++ b/mojo/services/network/public/interfaces/BUILD.gn
> @@ -9,6 +9,7 @@ mojom("interfaces") {
>    sources = [
>      "cookie_store.mojom",
>      "http_connection.mojom",
> +    "http_message.mojom",
>      "http_server.mojom",
>      "net_address.mojom",
>      "network_error.mojom",
> Index: mojo/services/network/public/interfaces/http_connection.mojom
> diff --git a/mojo/services/network/public/interfaces/http_connection.mojom
> b/mojo/services/network/public/interfaces/http_connection.mojom
> index
>
bf98038fd8a61d6767a86592d63285b4285e30a9..1733ed22d39151f7d1f52b3fc8cfc295af48dd1c
> 100644
> --- a/mojo/services/network/public/interfaces/http_connection.mojom
> +++ b/mojo/services/network/public/interfaces/http_connection.mojom
> @@ -4,7 +4,7 @@
>
>  module mojo;
>
> -import "network/public/interfaces/url_loader.mojom";
> +import "network/public/interfaces/http_message.mojom";
>  import "network/public/interfaces/network_error.mojom";
>  import "network/public/interfaces/web_socket.mojom";
>
> @@ -18,7 +18,7 @@ interface HttpConnection {
>
>  interface HttpConnectionDelegate {
>    // Called when an HTTP request is received.
> -  OnReceivedRequest(URLRequest request) => (URLResponse response);
> +  OnReceivedRequest(HttpRequest request) => (HttpResponse response);
>
>    // Called when an WebSocket request is received.
>    //
> @@ -28,7 +28,7 @@ interface HttpConnectionDelegate {
>    // WebSocket should be written to the producer end of the |send_stream|.
>    // |web_socket| will be already connected. There is no need to call
> Connect()
>    // on it. But |client| will still receive a DidConnect() notification.
> -  OnReceivedWebSocketRequest(URLRequest request)
> +  OnReceivedWebSocketRequest(HttpRequest request)
>        => (WebSocket&? web_socket,
>            handle<data_pipe_consumer>? send_stream,
>            WebSocketClient? client);
> Index: mojo/services/network/public/interfaces/http_message.mojom
> diff --git a/mojo/services/network/public/interfaces/http_message.mojom
> b/mojo/services/network/public/interfaces/http_message.mojom
> new file mode 100644
> index
>
0000000000000000000000000000000000000000..0450e46896090a0d02c4e6594404e12bf54dee50
> --- /dev/null
> +++ b/mojo/services/network/public/interfaces/http_message.mojom
> @@ -0,0 +1,23 @@
> +// 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.
> +
> +module mojo;
> +
> +struct HTTPHeader {
> +  string name;
> +  string value;
> +};
> +
> +struct HttpRequest {
> +  string method = "GET";
> +  string url;
> +  array<HTTPHeader>? headers;
> +  handle<data_pipe_consumer>? body;
> +};
> +
> +struct HttpResponse {
> +  uint32 status_code = 200;
> +  array<HTTPHeader>? headers;
> +  handle<data_pipe_consumer>? body;
> +};
> Index: mojo/services/network/public/interfaces/url_loader.mojom
> diff --git a/mojo/services/network/public/interfaces/url_loader.mojom
> b/mojo/services/network/public/interfaces/url_loader.mojom
> index
>
4ca3f8dde9c65d9fe85e2aaaa8c8cca2a2964298..41248a69586a018b3d21fd80c55c97b420a33117
> 100644
> --- a/mojo/services/network/public/interfaces/url_loader.mojom
> +++ b/mojo/services/network/public/interfaces/url_loader.mojom
> @@ -4,13 +4,9 @@
>
>  module mojo;
>
> +import "network/public/interfaces/http_message.mojom";
>  import "network/public/interfaces/network_error.mojom";
>
> -struct HTTPHeader {
> -  string name;
> -  string value;
> -};
> -
>  struct URLRequest {
>    // The URL to load.
>    string url;
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2015-05-14 22:48:46]

On 2015/05/14 22:15:18, darin (slow to review) wrote:
> We should either go with HTTP or Http universally. Mixing the two would be
> unfortunate. We don't have UrlRequest, so it is perhaps odd to have
> HttpRequest.
> 
> -Darin

Thanks Darin!

I agree that we should pick one way and use it universally. I am just not quite
sure which is preferred. We don't enforce that very well in the existing chrome
code. For example, our net/ code prefers "URL/TCP/UDP/SSL", but also prefers
"Http/Spdy/Quic" in class/method names.

Using "HTTP" in mojom makes our mojo APIs look more consistent. On the other
hand, it causes inconsistency when those mojom definitions are used in the
chrome codebase, unless we want to change the existing "Http" usage.

Also our C++ coding style guide says:
"Prefer to capitalize acronyms as single words (i.e. StartRpc(), not
StartRPC())."

I am torn between the two...

[darin (slow to review), 2015-05-15 05:45:09]

Probably best to follow the style guide then :)
On May 14, 2015 3:49 PM, <yzshen@chromium.org> wrote:

> On 2015/05/14 22:15:18, darin (slow to review) wrote:
>
>> We should either go with HTTP or Http universally. Mixing the two would be
>> unfortunate. We don't have UrlRequest, so it is perhaps odd to have
>> HttpRequest.
>>
>
>  -Darin
>>
>
> Thanks Darin!
>
> I agree that we should pick one way and use it universally. I am just not
> quite
> sure which is preferred. We don't enforce that very well in the existing
> chrome
> code. For example, our net/ code prefers "URL/TCP/UDP/SSL", but also
> prefers
> "Http/Spdy/Quic" in class/method names.
>
> Using "HTTP" in mojom makes our mojo APIs look more consistent. On the
> other
> hand, it causes inconsistency when those mojom definitions are used in the
> chrome codebase, unless we want to change the existing "Http" usage.
>
> Also our C++ coding style guide says:
> "Prefer to capitalize acronyms as single words (i.e. StartRpc(), not
> StartRPC())."
>
> I am torn between the two...
>
> https://codereview.chromium.org/1129143005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2015-05-15 05:56:03]

lgtm, I defer to Darin on the exact naming (seems easy to change whatever we
decide later)

[yzshen1, 2015-05-15 23:31:28]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-05-15 23:31:29]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1129143005/#ps100001
(title: " ")

[yzshen1, 2015-05-15 23:32:38]

On 2015/05/15 05:56:03, jam wrote:
> lgtm, I defer to Darin on the exact naming (seems easy to change whatever we
> decide later)

Thanks Darin and John!

Then I will use "Http" for now. And as John said, I could make changes later if
we decide to pick the other style.

[commit-bot: I haz the power, 2015-05-15 23:33:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129143005/100001

[commit-bot: I haz the power, 2015-05-15 23:54:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-18 11:29:27]

Patchset 6 (id:??) landed as
https://crrev.com/9e02055bbbf56c87f3bd27fb9c57607f8b9f515e
Cr-Commit-Position: refs/heads/master@{#330238}