Add net::HttpServer::Delegate::OnConnect() function and set ChromeDriver buffer sizes to 100 MB

Add net::HttpServer::Delegate::OnConnect() function and set ChromeDriver buffer sizes to 100 MB

BUG=

Committed: https://crrev.com/0b4d9b95e0cb553624b994df899ee4010425985e
Cr-Commit-Position: refs/heads/master@{#296881}

[samuong, 2014-09-23 22:00:14]

samuong@chromium.org changed reviewers:
+ mmenke@chromium.org, stgao@chromium.org

[samuong, 2014-09-23 22:00:14]

The 1 MB buffer size limit introduced in r291784 is too low for ChromeDriver,
and we need the ability to increase the maximum buffer size for our HttpServer.
This CL adds an OnConnect() method which is called when new HttpConnections are
made, so that the buffer size can be set before we read the data into the
buffer.

[mmenke, 2014-09-23 22:29:31]

https://codereview.chromium.org/594393002/diff/1/net/server/http_server.cc
File net/server/http_server.cc (right):

https://codereview.chromium.org/594393002/diff/1/net/server/http_server.cc#ne...
net/server/http_server.cc:156: delegate_->OnConnect(connection->id());
Should allow the embedder to close the connection here, and have a test for that
case (Which conveniently also makes sure it's called in the first place)

[samuong, 2014-09-23 23:45:11]

Patchset #2 (id:20001) has been deleted

[samuong, 2014-09-24 00:07:14]

https://codereview.chromium.org/594393002/diff/1/net/server/http_server.cc
File net/server/http_server.cc (right):

https://codereview.chromium.org/594393002/diff/1/net/server/http_server.cc#ne...
net/server/http_server.cc:156: delegate_->OnConnect(connection->id());
On 2014/09/23 22:29:31, mmenke wrote:
> Should allow the embedder to close the connection here, and have a test for
that
> case (Which conveniently also makes sure it's called in the first place)

Done.

[mmenke, 2014-09-24 15:38:45]

https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
File net/server/http_server_unittest.cc (right):

https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
net/server/http_server_unittest.cc:624: namespace {
nit:  Can you just stick this entire file in the anonymous namespace instead?

https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
net/server/http_server_unittest.cc:637: } // namespace
nit: 2 spaces between code and comment.

[mmenke, 2014-09-24 15:39:01]

Oh, and LGTM

[samuong, 2014-09-24 16:32:51]

https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
File net/server/http_server_unittest.cc (right):

https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
net/server/http_server_unittest.cc:624: namespace {
On 2014/09/24 15:38:45, mmenke wrote:
> nit:  Can you just stick this entire file in the anonymous namespace instead?

I've done this as much as possible, but HttpServerTest is a friend class of
net::HttpServer, so it needs to be outside of the anonymous namespace for the
friend declaration in net::HttpServer to work. But everything else in this file
is now in an anonymous namespace.

https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
net/server/http_server_unittest.cc:637: } // namespace
On 2014/09/24 15:38:45, mmenke wrote:
> nit: 2 spaces between code and comment.

Done.

[mmenke, 2014-09-24 16:39:02]

On 2014/09/24 16:32:51, samuong wrote:
>
https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
> File net/server/http_server_unittest.cc (right):
> 
>
https://codereview.chromium.org/594393002/diff/40001/net/server/http_server_u...
> net/server/http_server_unittest.cc:624: namespace {
> On 2014/09/24 15:38:45, mmenke wrote:
> > nit:  Can you just stick this entire file in the anonymous namespace
instead?
> 
> I've done this as much as possible, but HttpServerTest is a friend class of
> net::HttpServer, so it needs to be outside of the anonymous namespace for the
> friend declaration in net::HttpServer to work. But everything else in this
file
> is now in an anonymous namespace.

Ah, I'd forgotten about that.

Still LGTM

[samuong, 2014-09-24 17:05:25]

The CQ bit was checked by samuong@chromium.org

[commit-bot: I haz the power, 2014-09-24 17:06:30]

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

[commit-bot: I haz the power, 2014-09-24 17:15:46]

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

[commit-bot: I haz the power, 2014-09-24 17:15:47]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[samuong, 2014-09-24 17:39:45]

samuong@chromium.org changed reviewers:
+ aa@chromium.org, gene@chromium.org, pfeldman@chromium.org

[samuong, 2014-09-24 17:39:46]

aa@chromium.org: Please review changes in mojo/spy/websocket_server.h

gene@chromium.org: Please review changes in
cloud_print/gcp20/prototype/privet_http_server.h

pfeldman@chromium.org: Please review changes in
content/browser/devtools/devtools_http_handler_impl.h

[gene, 2014-09-24 17:51:35]

lgtm for cloud_print/

[stgao, 2014-09-24 17:53:34]

lgtm with nits for chromedriver/

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/net/test_http_server.cc (right):

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/net/test_http_server.cc:86:
server_->SetSendBufferSize(connection_id, kBufferSize);
Could these two lines be deleted as they are already set in OnConnect above?

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/net/test_http_server.cc:112:
server_->SetSendBufferSize(connection_id, kBufferSize);
Same here.

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:718: def
testReceivingLotsOfData(self):
How about testChromeDriverSendLargeData?

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:723: def
testSendingAndReceivingLotsOfData(self):
testChromeDriverRecieveAndSendLargeData?

[samuong, 2014-09-24 18:18:36]

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/net/test_http_server.cc (right):

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/net/test_http_server.cc:86:
server_->SetSendBufferSize(connection_id, kBufferSize);
On 2014/09/24 17:53:34, Shuotao wrote:
> Could these two lines be deleted as they are already set in OnConnect above?

Done.

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/net/test_http_server.cc:112:
server_->SetSendBufferSize(connection_id, kBufferSize);
On 2014/09/24 17:53:34, Shuotao wrote:
> Same here.

Done.

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:718: def
testReceivingLotsOfData(self):
On 2014/09/24 17:53:34, Shuotao wrote:
> How about testChromeDriverSendLargeData?

Done.

https://codereview.chromium.org/594393002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:723: def
testSendingAndReceivingLotsOfData(self):
On 2014/09/24 17:53:34, Shuotao wrote:
> testChromeDriverRecieveAndSendLargeData?

Done.

[samuong, 2014-09-25 21:40:09]

On ios_dbg_simulator, the OnConnect() call happens a bit later than on other
platforms. I've updated the unit test to reflect that.

[Aaron Boodman, 2014-09-25 21:43:23]

lgtm

[mmenke, 2014-09-25 21:45:50]

On 2014/09/25 21:43:23, Aaron Boodman wrote:
> lgtm

Updated test LGTM.

[pfeldman, 2014-09-26 02:28:28]

lgtm.

[samuong, 2014-09-26 03:06:19]

The CQ bit was checked by samuong@chromium.org

[commit-bot: I haz the power, 2014-09-26 03:06:57]

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

[commit-bot: I haz the power, 2014-09-26 04:15:36]

Committed patchset #5 (id:100001) as d5bf8dbe7d5c92faab09d60e71cd93b7b9ac6db0

[commit-bot: I haz the power, 2014-09-26 04:16:12]

Patchset 5 (id:??) landed as
https://crrev.com/0b4d9b95e0cb553624b994df899ee4010425985e
Cr-Commit-Position: refs/heads/master@{#296881}