Remove GICE.

Remove GICE (gone forever!) and PORTALLOCATOR_ENABLE_SHARED_UFRAG (enabled forever).

R=deadbeef@webrtc.org, juberti@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/3449faa553ec94c52ef2d0949867befb60992c88

[pthatcher1, 2015-07-28 23:00:53]

pthatcher@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, juberti@webrtc.org

[pthatcher1, 2015-07-28 23:00:54]

It's time for GICE to disappear.  Please make sure I didn't break anything in
the process of removing it.

[Taylor Brandstetter, 2015-07-28 23:57:03]

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:489: type = PRFLX_PORT_TYPE;
Looks like the "type" variable isn't used any more. Can probably delete this
line and move the comment down to where the Candidate is constructed.

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:932: // We don't want to pick the best
connections if channel is is
Minor typo here.

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/client/httppor...
File webrtc/p2p/client/httpportallocator.cc (right):

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/client/httppor...
webrtc/p2p/client/httpportallocator.cc:171: if (allocator()->flags()) {
If shared ufrag is always enabled, we should just get rid of this "if"
statement, right?

[pthatcher1, 2015-07-29 17:29:26]

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:489: type = PRFLX_PORT_TYPE;
On 2015/07/28 23:57:03, Taylor Brandstetter wrote:
> Looks like the "type" variable isn't used any more. Can probably delete this
> line and move the comment down to where the Candidate is constructed.

Good idea. 

Done.

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:932: // We don't want to pick the best
connections if channel is is
On 2015/07/28 23:57:03, Taylor Brandstetter wrote:
> Minor typo here.

Good catch.

Fixed.

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/client/httppor...
File webrtc/p2p/client/httpportallocator.cc (right):

https://codereview.webrtc.org/1263663002/diff/40001/webrtc/p2p/client/httppor...
webrtc/p2p/client/httpportallocator.cc:171: if (allocator()->flags()) {
On 2015/07/28 23:57:03, Taylor Brandstetter wrote:
> If shared ufrag is always enabled, we should just get rid of this "if"
> statement, right?

Yeah.  Hopefully, that's my "yeah duh" moment for the day :).

Fixed.

[Taylor Brandstetter, 2015-07-29 17:35:17]

lgtm

https://codereview.webrtc.org/1263663002/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1263663002/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:483: std::string type;
Sorry if I'm being overly picky, but there's still "std::string type" which
isn't used

[pthatcher1, 2015-07-31 00:28:53]

OK, Justin, can you take a look?  

I had to change some of the unit tests because for some reason when some of the
timing changed, so did the propensity for ending up with a prflx candidate
instead of a local candidate.  But other than that, everything was pure
deletion.

https://codereview.webrtc.org/1263663002/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1263663002/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:483: std::string type;
On 2015/07/29 17:35:17, Taylor Brandstetter wrote:
> Sorry if I'm being overly picky, but there's still "std::string type" which
> isn't used

Picky is good.  Thank you.

[juberti1, 2015-07-31 01:14:00]

Didn't review unit tests yet. I think there is also cruft in candidate.h (e.g.
preference()) that can now be culled.

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:370: portallocator_flags |=
cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
shared_socket should also be enabled forever

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectionendtoend_unittest.cc (left):

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectionendtoend_unittest.cc:289:
TEST_F(PeerConnectionEndToEndTest, DISABLED_CallWithLegacySdp) {
I think we need to retain this test, just w/o GICE. SDES still needs testing...
and perhaps non-BUNDLE

https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
File talk/session/media/mediasession.cc (left):

https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
talk/session/media/mediasession.cc:1889: bool GetMediaChannelNameFromComponent(
good riddance

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:192: // TODO(pthatcher): Remove this old behavior once
we're sure no one
yes, this has been the source of many bugs

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:618: response.AddMessageIntegrity(password_);
fix indent

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.cc (left):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.cc:24: // If PORTALLOCATOR_ENABLE_SHARED_UFRAG
flag is not enabled, ignore the
Is there code to generate ufrag/pwd in this file that should be removed?

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/rawtransp...
File webrtc/p2p/base/rawtransport.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/rawtransp...
webrtc/p2p/base/rawtransport.cc:20: RawTransport::RawTransport(rtc::Thread*
signaling_thread,
I think we should probably just kill this off at this point, now that we have no
way to negotiate a non-ICE protocol

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h
File webrtc/p2p/base/session.h (left):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h...
webrtc/p2p/base/session.h:448: const std::string transport_type_;
FWIW, think about how we would negotiate a QUIC transport (i.e. non-ICE). Would
we want this? Or maybe we just come back to it later?

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.cc:58: if (offer == nullptr) {
!offer

[pthatcher1, 2015-07-31 04:25:37]

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:370: portallocator_flags |=
cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
On 2015/07/31 01:13:59, juberti1 wrote:
> shared_socket should also be enabled forever

I'll look into it, but I'm guessing this CL is already too big already, and that
should happen in a separate CL.

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectionendtoend_unittest.cc (left):

https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectionendtoend_unittest.cc:289:
TEST_F(PeerConnectionEndToEndTest, DISABLED_CallWithLegacySdp) {
On 2015/07/31 01:13:59, juberti1 wrote:
> I think we need to retain this test, just w/o GICE. SDES still needs
testing...
> and perhaps non-BUNDLE

Done.

https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
File talk/session/media/mediasession.cc (left):

https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
talk/session/media/mediasession.cc:1889: bool GetMediaChannelNameFromComponent(
On 2015/07/31 01:13:59, juberti1 wrote:
> good riddance

Acknowledged.

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:192: // TODO(pthatcher): Remove this old behavior once
we're sure no one
On 2015/07/31 01:13:59, juberti1 wrote:
> yes, this has been the source of many bugs

Acknowledged.

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:618: response.AddMessageIntegrity(password_);
On 2015/07/31 01:13:59, juberti1 wrote:
> fix indent

Done.

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.cc (left):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.cc:24: // If PORTALLOCATOR_ENABLE_SHARED_UFRAG
flag is not enabled, ignore the
On 2015/07/31 01:13:59, juberti1 wrote:
> Is there code to generate ufrag/pwd in this file that should be removed?

No, it's in that other file where you said "source of many bugs", where I added
a TODO to remove it.

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h
File webrtc/p2p/base/session.h (left):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h...
webrtc/p2p/base/session.h:448: const std::string transport_type_;
On 2015/07/31 01:14:00, juberti1 wrote:
> FWIW, think about how we would negotiate a QUIC transport (i.e. non-ICE).
Would
> we want this? Or maybe we just come back to it later?

Come back to it later.

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.cc (right):

https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.cc:58: if (offer == nullptr) {
On 2015/07/31 01:14:00, juberti1 wrote:
> !offer

Done.

https://codereview.webrtc.org/1263663002/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1263663002/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:445:
EXPECT_TRUE(RemoteCandidateTypeEquals(ep1_ch1(), expected.remote_type));
FYI, I'm still a little uneasy about this local/prflx candidate thing being
necessary.  But if I don't do it, the tests with BLOCK_UDP in it fail, and I'm
not sure why, other than it's a timing issue.  They don't always fail, they're
just flaky because sometimes the candidate is local, and sometimes it's prflx. 
I'm not sure what change might have trigger this other than the difference in
the step delay.  But I found increasing the step delay only made it less flaky,
not completely un-flaky.

Advice on how to make these tests better (not flaky, but testing what we want)
are welcome.

[pthatcher1, 2015-07-31 04:49:19]

https://codereview.webrtc.org/1263663002/diff/120001/webrtc/p2p/base/port_uni...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/1263663002/diff/120001/webrtc/p2p/base/port_uni...
webrtc/p2p/base/port_unittest.cc:594:
port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
FYI, 2 of these tests are failing:

TestTcpReconnectOnPing
TestTcpReconnectOnSendPacket

I'm not sure why.  It appears that we always tested these in GICE mode, and
never in ICE mode, so perhaps it never worked in ICE mode.  But I haven't been
able to look into it a lot yet.  Any insights would be helpful.

[pthatcher1, 2015-07-31 04:54:52]

On 2015/07/31 01:14:00, juberti1 wrote:
> Didn't review unit tests yet. I think there is also cruft in candidate.h (e.g.
> preference()) that can now be culled.
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> File talk/app/webrtc/peerconnection.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> talk/app/webrtc/peerconnection.cc:370: portallocator_flags |=
> cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
> shared_socket should also be enabled forever
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> File talk/app/webrtc/peerconnectionendtoend_unittest.cc (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> talk/app/webrtc/peerconnectionendtoend_unittest.cc:289:
> TEST_F(PeerConnectionEndToEndTest, DISABLED_CallWithLegacySdp) {
> I think we need to retain this test, just w/o GICE. SDES still needs
testing...
> and perhaps non-BUNDLE
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
> File talk/session/media/mediasession.cc (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
> talk/session/media/mediasession.cc:1889: bool
GetMediaChannelNameFromComponent(
> good riddance
> 
> https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc
> File webrtc/p2p/base/port.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
> webrtc/p2p/base/port.cc:192: // TODO(pthatcher): Remove this old behavior once
> we're sure no one
> yes, this has been the source of many bugs
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
> webrtc/p2p/base/port.cc:618: response.AddMessageIntegrity(password_);
> fix indent
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
> File webrtc/p2p/base/portallocator.cc (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
> webrtc/p2p/base/portallocator.cc:24: // If PORTALLOCATOR_ENABLE_SHARED_UFRAG
> flag is not enabled, ignore the
> Is there code to generate ufrag/pwd in this file that should be removed?
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/rawtransp...
> File webrtc/p2p/base/rawtransport.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/rawtransp...
> webrtc/p2p/base/rawtransport.cc:20: RawTransport::RawTransport(rtc::Thread*
> signaling_thread,
> I think we should probably just kill this off at this point, now that we have
no
> way to negotiate a non-ICE protocol
> 
> https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h
> File webrtc/p2p/base/session.h (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h...
> webrtc/p2p/base/session.h:448: const std::string transport_type_;
> FWIW, think about how we would negotiate a QUIC transport (i.e. non-ICE).
Would
> we want this? Or maybe we just come back to it later?
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
> File webrtc/p2p/base/transportdescriptionfactory.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
> webrtc/p2p/base/transportdescriptionfactory.cc:58: if (offer == nullptr) {
> !offer

Candidate preference is now gone.

[pthatcher1, 2015-07-31 05:09:41]

On 2015/07/31 01:14:00, juberti1 wrote:
> Didn't review unit tests yet. I think there is also cruft in candidate.h (e.g.
> preference()) that can now be culled.
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> File talk/app/webrtc/peerconnection.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> talk/app/webrtc/peerconnection.cc:370: portallocator_flags |=
> cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
> shared_socket should also be enabled forever
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> File talk/app/webrtc/peerconnectionendtoend_unittest.cc (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/app/webrtc/peerconne...
> talk/app/webrtc/peerconnectionendtoend_unittest.cc:289:
> TEST_F(PeerConnectionEndToEndTest, DISABLED_CallWithLegacySdp) {
> I think we need to retain this test, just w/o GICE. SDES still needs
testing...
> and perhaps non-BUNDLE
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
> File talk/session/media/mediasession.cc (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/talk/session/media/medias...
> talk/session/media/mediasession.cc:1889: bool
GetMediaChannelNameFromComponent(
> good riddance
> 
> https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc
> File webrtc/p2p/base/port.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
> webrtc/p2p/base/port.cc:192: // TODO(pthatcher): Remove this old behavior once
> we're sure no one
> yes, this has been the source of many bugs
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/port.cc#n...
> webrtc/p2p/base/port.cc:618: response.AddMessageIntegrity(password_);
> fix indent
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
> File webrtc/p2p/base/portallocator.cc (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/portalloc...
> webrtc/p2p/base/portallocator.cc:24: // If PORTALLOCATOR_ENABLE_SHARED_UFRAG
> flag is not enabled, ignore the
> Is there code to generate ufrag/pwd in this file that should be removed?
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/rawtransp...
> File webrtc/p2p/base/rawtransport.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/rawtransp...
> webrtc/p2p/base/rawtransport.cc:20: RawTransport::RawTransport(rtc::Thread*
> signaling_thread,
> I think we should probably just kill this off at this point, now that we have
no
> way to negotiate a non-ICE protocol
> 
> https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h
> File webrtc/p2p/base/session.h (left):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/session.h...
> webrtc/p2p/base/session.h:448: const std::string transport_type_;
> FWIW, think about how we would negotiate a QUIC transport (i.e. non-ICE).
Would
> we want this? Or maybe we just come back to it later?
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
> File webrtc/p2p/base/transportdescriptionfactory.cc (right):
> 
>
https://codereview.webrtc.org/1263663002/diff/80001/webrtc/p2p/base/transport...
> webrtc/p2p/base/transportdescriptionfactory.cc:58: if (offer == nullptr) {
> !offer

FYI, Here is a CL with SHARED_SOCKET also removed:

https://codereview.webrtc.org/1263603003


But as you can see, tons of tests need to be updated.  I think that should be in
its own CL.

[pthatcher1, 2015-07-31 09:00:30]

FYI, I cleaned up the p2ptransportchannel_unittest to the point where I like it,
or at least find it tolerable.  It's much more precise in what it allows
(local<->prflx to be flipped around to prflx<->local).

[juberti1, 2015-08-07 07:28:08]

Still haven't looked at the unit tests, but the code lgtm.

Will try to look at unit tests friday, but go ahead and submit if you don't hear
from me by monday

https://codereview.webrtc.org/1263663002/diff/180001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionendtoend_unittest.cc (right):

https://codereview.webrtc.org/1263663002/diff/180001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionendtoend_unittest.cc:237: // Disabled per
b/14899892
that pulse build is now dead, we should re-enable this

[pthatcher1, 2015-08-10 19:23:08]

Committed patchset #13 (id:220001) manually as
3449faa553ec94c52ef2d0949867befb60992c88 (presubmit successful).