Mojo service implementation for HTTP server - part 2

Mojo service implementation for HTTP server - part 2

This CL supports HTTPConnectionDelegate.OnReceivedRequest() and adds corresponding tests.

The following work will be in future CLs:
- support WebSocket.
- more tests.

BUG=478249
TEST=Newly-added tests.

Committed: https://crrev.com/349704cec73c9b5a377827313860d178b062fb8c
Cr-Commit-Position: refs/heads/master@{#330168}

[yzshen1, 2015-05-14 19:08:02]

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

[yzshen1, 2015-05-14 19:08:25]

Hi, John.

Would you please take a look? Thanks!

[jam, 2015-05-15 06:46:43]

lgtm

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
File mojo/services/network/http_connection_impl.cc (right):

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
mojo/services/network/http_connection_impl.cc:110: DCHECK_EQ(MOJO_RESULT_OK,
result);
nit: perhaps CHECK so that we know if we have to hit MOJO_RESULT_SHOULD_WAIT
later?

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
mojo/services/network/http_connection_impl.cc:131: for (const auto& reader :
response_body_readers_)
nit: STLDeleteElements

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
mojo/services/network/http_connection_impl.cc:187: if
(!encountered_connection_error_) {
why is this boolean needed? perhaps document why it would get called twice

nit: change this so that it early returns if the bool is set, that way you have
less indentation

[yzshen1, 2015-05-15 17:22:54]

Thank you, John.

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
File mojo/services/network/http_connection_impl.cc (right):

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
mojo/services/network/http_connection_impl.cc:110: DCHECK_EQ(MOJO_RESULT_OK,
result);
On 2015/05/15 06:46:43, jam wrote:
> nit: perhaps CHECK so that we know if we have to hit MOJO_RESULT_SHOULD_WAIT
> later?

Done.

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
mojo/services/network/http_connection_impl.cc:131: for (const auto& reader :
response_body_readers_)
On 2015/05/15 06:46:43, jam wrote:
> nit: STLDeleteElements

Done.

https://codereview.chromium.org/1128863004/diff/100001/mojo/services/network/...
mojo/services/network/http_connection_impl.cc:187: if
(!encountered_connection_error_) {
On 2015/05/15 06:46:43, jam wrote:
> why is this boolean needed? perhaps document why it would get called twice
> 
> nit: change this so that it early returns if the bool is set, that way you
have
> less indentation
I realized that based on the current code it won't be called twice. Although
this error handler is set for both |binding_| and |delegate_|, we reset the two
when this method is called for the first time.

I replaced the boolean with a method EncounteredConnectionError(), and updated
comments. Thanks!

[yzshen1, 2015-05-15 17:23:37]

The CQ bit was checked by yzshen@chromium.org

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

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/1128863004/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 17:24:01]

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

[commit-bot: I haz the power, 2015-05-15 17:32:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 17:32:34]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2015-05-15 17:47:44]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-05-15 17:47:44]

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/1128863004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 17:50:25]

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

[commit-bot: I haz the power, 2015-05-15 18:06:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 18:06:22]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2015-05-15 19:03:50]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-05-15 19:03:50]

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/1128863004/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 19:07:16]

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

[commit-bot: I haz the power, 2015-05-15 19:37:44]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-05-15 19:38:32]

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