Emit SignalReadyToSend even for "presumed writable" connections.

webrtc/p2p/base/p2ptransportchannel_unittest.cc

Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 51ff5fe92306768577857d112ffcca7a44f73450..ec488a1f94f581aea3e5173de60eb95e17e1e0d1 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1806,6 +1806,42 @@ TEST_F(P2PTransportChannelTest, PresumedWritablePreferredOverUnreliable) {
   DestroyChannels();
 }
 
+// Ensure that "SignalReadyToSend" is fired as expected with a "presumed
+// writable" connection. Previously this did not work.
+TEST_F(P2PTransportChannelTest, SignalReadyToSendWithPresumedWritable) {
+  ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
+                     kDefaultPortAllocatorFlags);
+  // Only test one endpoint, so we can ensure the connection doesn't receive a

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

If you only test one endpoint, would it be better to create the test based on
P2PTransportChannelPingTest, which has only one endpoint?

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

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?

[honghaiz3, 2016/09/29 21:47:59]

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). 

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

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.

+  // binding response and advance beyond being "presumed" writable.

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

nit: s/advance/advances/ ?

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

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

[honghaiz3, 2016/09/29 21:47:59]

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. 

+  GetEndpoint(0)->cd1_.ch_.reset(CreateChannel(
+      0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1]));
+  IceConfig config;
+  config.presume_writable_when_fully_relayed = true;
+  ep1_ch1()->SetIceConfig(config);
+  ep1_ch1()->MaybeStartGathering();
+  EXPECT_EQ_WAIT(IceGatheringState::kIceGatheringComplete,
+                 ep1_ch1()->gathering_state(), kDefaultTimeout);
+  ep1_ch1()->AddRemoteCandidate(
+      CreateUdpCandidate(RELAY_PORT_TYPE, "1.1.1.1", 1, 0));
+  // Sanity checking the type of the connection.
+  EXPECT_TRUE(ep1_ch1()->selected_connection() != nullptr);
+  EXPECT_EQ(RELAY_PORT_TYPE, LocalCandidate(ep1_ch1())->type());
+  EXPECT_EQ(RELAY_PORT_TYPE, RemoteCandidate(ep1_ch1())->type());
+
+  // Tell the socket server to block packets (returning EWOULDBLOCK).
+  virtual_socket_server()->SetSendingBlocked(true);
+  const char* data = "test";
+  int len = static_cast<int>(strlen(data));
+  EXPECT_EQ(-1, SendData(ep1_ch1(), data, len));
+
+  // Reset |ready_to_send_| flag, which is set to true if the event fires as it
+  // should.
+  GetEndpoint(0)->ready_to_send_ = false;
+  virtual_socket_server()->SetSendingBlocked(false);
+  EXPECT_TRUE(GetEndpoint(0)->ready_to_send_);
+  EXPECT_EQ(len, SendData(ep1_ch1(), data, len));
+}
+
 // Test what happens when we have 2 users behind the same NAT. This can lead
 // to interesting behavior because the STUN server will only give out the
 // address of the outermost NAT.