Add a chromium based simple QUIC client.

Add a chromium based simple QUIC client.

Committed: https://crrev.com/04730a11c0a58a338bccc006c5cdbccc2607185e
Cr-Commit-Position: refs/heads/master@{#321303}

Committed: https://crrev.com/a9d12ce1b126160de760c263e3761f10deca80c4
Cr-Commit-Position: refs/heads/master@{#321465}

[Ryan Hamilton, 2015-03-18 02:50:56]

rch@chromium.org changed reviewers:
+ rtenneti@chromium.org

[ramant (doing other things), 2015-03-18 05:21:36]

Awesome work.

LGTM.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
File net/tools/quic/quic_simple_client.cc (right):

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:27: } // namespace
nit: an extra space before "//".

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:139: // Close old socket
overly nit: consider punctuation. Period at the end of sentence.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:166: /* is_secure= */ false,
nit: should we consider adding a TODO to support server_id_.is_https().

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:223: return NULL;
nit: consider nullptr to be consistent with rest of the code. It is your call.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:261:
base::Bind(&QuicSimpleClient::OnReadComplete, base::Unretained(this)));
nit: I am not 100% sure what to use: "weak_factory_.GetWeakPtr()" vs
"base::Unretained(this)".

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:293:
session_->connection()->ProcessUdpPacket(
nit: Should we worry about ProcessUdpPacket() deleting |this|? In chromium code
we were exiting if connection() is not connected after ProcessUdpPacket() call.
You are the guru.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:302: if (response_listener_.get() != NULL)
{
nit: NULL -> nullptr?

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
File net/tools/quic/quic_simple_client.h (right):

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.h:33: class QuicConnectionHelper;
nit: Consider moving "class QuicConnectionHelper;" to after line 31.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
File net/tools/quic/quic_simple_client_stream.cc (right):

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client_stream.cc:107: size_t len =
framer.ParseHeaderBlockInBuffer(data, read_buf_len, &headers);
nit: Not sure. Could read_buf_->offset() change after call to
read_buf_->StartOfBuffer()? (in quic_http_stream.cc, we are passing
"read_buf_->offset()"). I think the old code copied that code.

[Ryan Hamilton, 2015-03-18 22:35:58]

PTAL

Thanks for the review. Much of this coded came from an old CL so I'm really glad
you're poking at it!

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
File net/tools/quic/quic_simple_client.cc (right):

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:27: } // namespace
On 2015/03/18 05:21:36, ramant wrote:
> nit: an extra space before "//".

Done.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:139: // Close old socket
On 2015/03/18 05:21:36, ramant wrote:
> overly nit: consider punctuation. Period at the end of sentence. 

Actually, nuking the comment completely.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:166: /* is_secure= */ false,
On 2015/03/18 05:21:36, ramant wrote:
> nit: should we consider adding a TODO to support server_id_.is_https().

Whoop,s I think we already have support for this. Done.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:223: return NULL;
On 2015/03/18 05:21:36, ramant wrote:
> nit: consider nullptr to be consistent with rest of the code. It is your call.

Done.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:261:
base::Bind(&QuicSimpleClient::OnReadComplete, base::Unretained(this)));
On 2015/03/18 05:21:36, ramant wrote:
> nit: I am not 100% sure what to use: "weak_factory_.GetWeakPtr()" vs
> "base::Unretained(this)".

Good catch.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:293:
session_->connection()->ProcessUdpPacket(
On 2015/03/18 05:21:36, ramant wrote:
> nit: Should we worry about ProcessUdpPacket() deleting |this|? In chromium
code
> we were exiting if connection() is not connected after ProcessUdpPacket()
call.
> You are the guru.

No, that's not possible here. That's possible in chrome because the session can
contact the factory which can delete the session. But nothing can delete the
Client in this case.

I think :>

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.cc:302: if (response_listener_.get() != NULL)
{
On 2015/03/18 05:21:36, ramant wrote:
> nit: NULL -> nullptr?

Done.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
File net/tools/quic/quic_simple_client.h (right):

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client.h:33: class QuicConnectionHelper;
On 2015/03/18 05:21:36, ramant wrote:
> nit: Consider moving "class QuicConnectionHelper;" to after line 31.

I think that would be an error, because this is a net::QuicConnectionHelper, not
a net::tool::QuicConnectionHelper.

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
File net/tools/quic/quic_simple_client_stream.cc (right):

https://codereview.chromium.org/995423003/diff/100001/net/tools/quic/quic_sim...
net/tools/quic/quic_simple_client_stream.cc:107: size_t len =
framer.ParseHeaderBlockInBuffer(data, read_buf_len, &headers);
On 2015/03/18 05:21:36, ramant wrote:
> nit: Not sure. Could read_buf_->offset() change after call to
> read_buf_->StartOfBuffer()? (in quic_http_stream.cc, we are passing
> "read_buf_->offset()"). I think the old code copied that code.

I don't think it matter, but happy to do what we're doing in quic_http_stream.cc

[ramant (doing other things), 2015-03-18 22:54:18]

LGTM.

[Ryan Hamilton, 2015-03-18 22:56:44]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2015-03-18 22:57:02]

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

[commit-bot: I haz the power, 2015-03-19 00:13:02]

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

[commit-bot: I haz the power, 2015-03-19 00:13:03]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Ryan Hamilton, 2015-03-19 02:38:47]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2015-03-19 02:38:48]

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

[commit-bot: I haz the power, 2015-03-19 02:39:21]

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

[Ryan Hamilton, 2015-03-19 03:48:23]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2015-03-19 03:48:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org
Link to the patchset: https://codereview.chromium.org/995423003/#ps160001
(title: "fix for win")

[commit-bot: I haz the power, 2015-03-19 03:48:35]

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

[Ryan Hamilton, 2015-03-19 04:15:11]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2015-03-19 04:15:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org
Link to the patchset: https://codereview.chromium.org/995423003/#ps180001
(title: "one more fix")

[commit-bot: I haz the power, 2015-03-19 04:15:37]

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

[commit-bot: I haz the power, 2015-03-19 06:02:19]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-19 06:02:52]

Patchset 10 (id:??) landed as
https://crrev.com/04730a11c0a58a338bccc006c5cdbccc2607185e
Cr-Commit-Position: refs/heads/master@{#321303}

[jackhou1, 2015-03-19 06:17:13]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1015353003/ by jackhou@chromium.org.

The reason for reverting is: Failed compile on IOS:
http://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/273
http://build.chromium.org/p/chromium.mac/builders/iOS_Simulator_%28dbg%29/bui....

[Ryan Hamilton, 2015-03-19 21:37:29]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2015-03-19 21:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org
Link to the patchset: https://codereview.chromium.org/995423003/#ps200001
(title: "fix iOS")

[commit-bot: I haz the power, 2015-03-19 21:37:47]

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

[commit-bot: I haz the power, 2015-03-19 23:07:06]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-19 23:08:12]

Patchset 11 (id:??) landed as
https://crrev.com/a9d12ce1b126160de760c263e3761f10deca80c4
Cr-Commit-Position: refs/heads/master@{#321465}