Add support for HTTP handlers that are in self-contained apps. Also add an example app.

Add support for HTTP handlers that are in self-contained apps. Also add an example app.

To try this out, run mojo_shell mojo://http_handler

I've also changed the http server so that it uses port 80 by default.

R=yzshen@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/ad9a66daf15d5a8abae7f49c368e8e6c28c03fa4

[jam, 2014-11-12 01:44:23]

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

[yzshen1, 2014-11-12 17:38:34]

https://codereview.chromium.org/715153004/diff/1/examples/http_handler/http_h...
File examples/http_handler/http_handler.cc (right):

https://codereview.chromium.org/715153004/diff/1/examples/http_handler/http_h...
examples/http_handler/http_handler.cc:25: virtual ~HttpHandler() {}
nit: I think the style guide now prefers "~HttpHandler() override".

https://codereview.chromium.org/715153004/diff/1/examples/http_handler/http_h...
examples/http_handler/http_handler.cc:40: const Callback<void(HttpResponsePtr)>&
callback) {
nit: override

https://codereview.chromium.org/715153004/diff/1/services/http_server/http_se...
File services/http_server/http_server.cc (right):

https://codereview.chromium.org/715153004/diff/1/services/http_server/http_se...
services/http_server/http_server.cc:329: for (size_t i = 0; i <
handlers_.size(); ++i) {
nit: using iterator here might be a little more cleaner.

https://codereview.chromium.org/715153004/diff/1/services/http_server/http_se...
services/http_server/http_server.cc:420: net_address->ipv4->port = 80;
This requires that there is nothing occupying that port currently. Are you
planning to make it configurable later?

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
File services/http_server/public/http_server.mojom (right):

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
services/http_server/public/http_server.mojom:19: CreateResponse(uint32
status_code, string body) => (HttpResponse response);
It seems this could be done in a client-side utility, instead of making a
round-trip to do it at the service side. WDYT?

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
services/http_server/public/http_server.mojom:22: [Client=HttpServerService]
Nit: usually you don't need to add this attribute, unless you want to explicitly
request/pass HttpServerClient interface in the mojom definition. (Typically you
only pass/request HttpServerService.)

[jam, 2014-11-12 20:47:54]

Patchset #2 (id:20001) has been deleted

[jam, 2014-11-12 20:47:57]

Patchset #2 (id:40001) has been deleted

[jam, 2014-11-12 20:48:41]

https://codereview.chromium.org/715153004/diff/1/examples/http_handler/http_h...
File examples/http_handler/http_handler.cc (right):

https://codereview.chromium.org/715153004/diff/1/examples/http_handler/http_h...
examples/http_handler/http_handler.cc:25: virtual ~HttpHandler() {}
On 2014/11/12 17:38:33, yzshen1 wrote:
> nit: I think the style guide now prefers "~HttpHandler() override".

Done.

https://codereview.chromium.org/715153004/diff/1/examples/http_handler/http_h...
examples/http_handler/http_handler.cc:40: const Callback<void(HttpResponsePtr)>&
callback) {
On 2014/11/12 17:38:33, yzshen1 wrote:
> nit: override

Done.

https://codereview.chromium.org/715153004/diff/1/services/http_server/http_se...
File services/http_server/http_server.cc (right):

https://codereview.chromium.org/715153004/diff/1/services/http_server/http_se...
services/http_server/http_server.cc:329: for (size_t i = 0; i <
handlers_.size(); ++i) {
On 2014/11/12 17:38:33, yzshen1 wrote:
> nit: using iterator here might be a little more cleaner.

Done.

https://codereview.chromium.org/715153004/diff/1/services/http_server/http_se...
services/http_server/http_server.cc:420: net_address->ipv4->port = 80;
On 2014/11/12 17:38:33, yzshen1 wrote:
> This requires that there is nothing occupying that port currently. Are you
> planning to make it configurable later?

I think right now it's easier if it's 80 so that when we push this to servers it
runs on a predetermined port. if we need it to be configurable later, we can add
that easily.

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
File services/http_server/public/http_server.mojom (right):

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
services/http_server/public/http_server.mojom:19: CreateResponse(uint32
status_code, string body) => (HttpResponse response);
On 2014/11/12 17:38:34, yzshen1 wrote:
> It seems this could be done in a client-side utility, instead of making a
> round-trip to do it at the service side. WDYT?

yeah that's better; done

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
services/http_server/public/http_server.mojom:22: [Client=HttpServerService]
On 2014/11/12 17:38:33, yzshen1 wrote:
> Nit: usually you don't need to add this attribute, unless you want to
explicitly
> request/pass HttpServerClient interface in the mojom definition. (Typically
you
> only pass/request HttpServerService.)

ah, I didn't know that, thanks. i had copied this from viewmanagerservice.
should that be changed as well?

[yzshen1, 2014-11-12 21:24:26]

LGTM with a few more nits.

Thanks!

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
File services/http_server/public/http_server.mojom (right):

https://codereview.chromium.org/715153004/diff/1/services/http_server/public/...
services/http_server/public/http_server.mojom:22: [Client=HttpServerService]
On 2014/11/12 20:48:41, jam wrote:
> On 2014/11/12 17:38:33, yzshen1 wrote:
> > Nit: usually you don't need to add this attribute, unless you want to
> explicitly
> > request/pass HttpServerClient interface in the mojom definition. (Typically
> you
> > only pass/request HttpServerService.)
> 
> ah, I didn't know that, thanks. i had copied this from viewmanagerservice.
> should that be changed as well?

IMO it is nice to change but not a big issue.

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
File services/http_server/public/BUILD.gn (right):

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
services/http_server/public/BUILD.gn:15: source_set("util") {
Do we want to put util in a sub-directory cpp/?

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
File services/http_server/public/http_server_util.cc (right):

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
services/http_server/public/http_server_util.cc:21: MojoResult result =
CreateDataPipe(
nit: Please add corresponding include files.

[jam, 2014-11-12 21:59:05]

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
File services/http_server/public/BUILD.gn (right):

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
services/http_server/public/BUILD.gn:15: source_set("util") {
On 2014/11/12 21:24:26, yzshen1 wrote:
> Do we want to put util in a sub-directory cpp/?

I didn't put interfaces in a subdirectory, so i'm hesitant to make a cpp
subdirectory at this point. if this directory grows, maybe then we can make
subdirs.

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
File services/http_server/public/http_server_util.cc (right):

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
services/http_server/public/http_server_util.cc:21: MojoResult result =
CreateDataPipe(
On 2014/11/12 21:24:26, yzshen1 wrote:
> nit: Please add corresponding include files. 

Done.

[yzshen1, 2014-11-12 22:01:21]

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
File services/http_server/public/BUILD.gn (right):

https://codereview.chromium.org/715153004/diff/60001/services/http_server/pub...
services/http_server/public/BUILD.gn:15: source_set("util") {
On 2014/11/12 21:59:04, jam wrote:
> On 2014/11/12 21:24:26, yzshen1 wrote:
> > Do we want to put util in a sub-directory cpp/?
> 
> I didn't put interfaces in a subdirectory, so i'm hesitant to make a cpp
> subdirectory at this point. if this directory grows, maybe then we can make
> subdirs.

Sounds good. Thanks!

[jam, 2014-11-12 22:03:02]

Committed patchset #3 (id:80001) manually as
ad9a66daf15d5a8abae7f49c368e8e6c28c03fa4.