Emit SignalReadyToSend even for "presumed writable" connections.

Emit SignalReadyToSend even for "presumed writable" connections.

The Connection class will now blindly forward SignalReadyToSend, and
P2PTransportChannel will decide whether to forward it further (which
it was already doing).

BUG=webrtc:6448
Committed: https://crrev.com/dd7fb43f28410e00039660e31e130568c901d03e
Cr-Commit-Position: refs/heads/master@{#14462}

[honghaiz3, 2016-09-29 20:04:51]

honghaiz@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

[honghaiz3, 2016-09-29 20:04:52]

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1814: // Only test one endpoint,
so we can ensure the connection doesn't receive a
If you only test one endpoint, would it be better to create the test based on
P2PTransportChannelPingTest, which has only one endpoint?

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1815: // binding response and
advance beyond being "presumed" writable.
nit: s/advance/advances/ ?

[Taylor Brandstetter, 2016-09-29 21:09:09]

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1814: // Only test one endpoint,
so we can ensure the connection doesn't receive a
On 2016/09/29 20:04:51, honghaiz3 wrote:
> If you only test one endpoint, would it be better to create the test based on
> P2PTransportChannelPingTest, which has only one endpoint?

I could, but then I'd have to add a TURN server to the test, and I thought that
may be introducing bloat. What do you think?

Also, if I move this test, should I move "TurnToTurnPresumedWritable" above,
which also only uses one endpoint?

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1815: // binding response and
advance beyond being "presumed" writable.
On 2016/09/29 20:04:51, honghaiz3 wrote:
> nit: s/advance/advances/ ?

I think "advances" is correct here ("doesn't advance" as opposed to "doesn't
advances")

[honghaiz3, 2016-09-29 21:48:00]

lgtm

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1814: // Only test one endpoint,
so we can ensure the connection doesn't receive a
On 2016/09/29 21:09:09, Taylor Brandstetter wrote:
> On 2016/09/29 20:04:51, honghaiz3 wrote:
> > If you only test one endpoint, would it be better to create the test based
on
> > P2PTransportChannelPingTest, which has only one endpoint?
> 
> I could, but then I'd have to add a TURN server to the test, and I thought
that
> may be introducing bloat. What do you think?
> 
> Also, if I move this test, should I move "TurnToTurnPresumedWritable" above,
> which also only uses one endpoint?

I will leave it for you to decide. If you wait for my CL being landed, that will
be just one-line to Create the Basic port allocator (and maybe a few more lines
for setting up). 
If you move this, probably it is a good idea to move that too (or put a TODO at
least).

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1815: // binding response and
advance beyond being "presumed" writable.
On 2016/09/29 21:09:09, Taylor Brandstetter wrote:
> On 2016/09/29 20:04:51, honghaiz3 wrote:
> > nit: s/advance/advances/ ?
> 
> I think "advances" is correct here ("doesn't advance" as opposed to "doesn't
> advances")

I thought you meant (does not receive) and (advances), right?
If you meant (does not receive) and (does not advance), then it should be
"doesn't receive or advance", I think. 
It is just nit anyway.

[Taylor Brandstetter, 2016-09-29 22:24:11]

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2374183005/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1814: // Only test one endpoint,
so we can ensure the connection doesn't receive a
On 2016/09/29 21:47:59, honghaiz3 wrote:
> On 2016/09/29 21:09:09, Taylor Brandstetter wrote:
> > On 2016/09/29 20:04:51, honghaiz3 wrote:
> > > If you only test one endpoint, would it be better to create the test based
> on
> > > P2PTransportChannelPingTest, which has only one endpoint?
> > 
> > I could, but then I'd have to add a TURN server to the test, and I thought
> that
> > may be introducing bloat. What do you think?
> > 
> > Also, if I move this test, should I move "TurnToTurnPresumedWritable" above,
> > which also only uses one endpoint?
> 
> I will leave it for you to decide. If you wait for my CL being landed, that
will
> be just one-line to Create the Basic port allocator (and maybe a few more
lines
> for setting up). 
> If you move this, probably it is a good idea to move that too (or put a TODO
at
> least). 

I looked into this, but the TURN server and network manager are only created in
P2PTransportChannelMostLikelyToWorkFirst test (subclass of
P2PTransportChannelPingTest), which is where it would make the most sense to put
these tests. But then the name doesn't really match what's being tested.

Maybe we could split this file into just two test classes?
"EndToEndP2PTransportChannelTest" (which uses two P2PTranpsortChannels) and
"SingleP2PTransportChannelTest" (which tests an individual channel, simulating
individual pings being received where necessary). Similar to
"peerconnectionendtoend_unittest" vs "peerconnectioninterface_unittest".

Anyway, until we decide where we're going with these tests, I think I'll land
this with a TODO.

[Taylor Brandstetter, 2016-09-29 22:24:16]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-09-29 22:24:17]

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2374183005/#ps20001 (title:
"Adding TODO.")

[commit-bot: I haz the power, 2016-09-29 22:24:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-29 22:56:45]

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

[commit-bot: I haz the power, 2016-09-29 22:56:46]

Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/11714)

[Taylor Brandstetter, 2016-09-30 22:03:05]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-09-30 22:03:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-30 22:16:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-30 22:17:01]

Description was changed from

==========
Emit SignalReadyToSend even for "presumed writable" connections.

The Connection class will now blindly forward SignalReadyToSend, and
P2PTransportChannel will decide whether to forward it further (which
it was already doing).

BUG=webrtc:6448
==========

to

==========
Emit SignalReadyToSend even for "presumed writable" connections.

The Connection class will now blindly forward SignalReadyToSend, and
P2PTransportChannel will decide whether to forward it further (which
it was already doing).

BUG=webrtc:6448

Committed: https://crrev.com/dd7fb43f28410e00039660e31e130568c901d03e
Cr-Commit-Position: refs/heads/master@{#14462}
==========

[commit-bot: I haz the power, 2016-09-30 22:17:02]

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