Reland "Remove GICE (gone forever!) and PORTALLOCATOR_ENABLE_SHARED_UFRAG (enabled forever)." becau…

Reland "Remove GICE (gone forever!) and PORTALLOCATOR_ENABLE_SHARED_UFRAG (enabled forever)." becauese remoting code is using dead constants and breaks the FYI bots."

This reverts commit 475243a134be003aab30bb17294ca6c664d0ef81.

R=guoweis@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/081f34b564e1a26ffbbe9515eba1fef7c736fdde

[pthatcher1, 2015-08-19 20:16:08]

pthatcher@webrtc.org changed reviewers:
+ guoweis@webrtc.org

[guoweis_webrtc, 2015-08-19 20:48:37]

some comments and questions.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:398: bool CheckCandidate1(const
Result& expected) {
Can you add a comment on what this is doing?

This one looks very similar to ExpectCandidate1 and it's not clear to me what
difference they are serving.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:412: remote_type ==
expected.local_type)));
This is a bit subtle. When IsLocalToPrflxOrTheReverse is true, the local_type
can't be the same as expected.local_type. (it should have early return).

Can we put a comment explaining that it must be the reverse case when running to
here?

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:430: bool CheckCandidate2(const
Result& expected) {
comments here too.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:454: void ExpectCandidate2(const
Result& expected) {
Same here.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1419: 2000);
why do we need to change the timeout?

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:74: assert(port->Candidates().size() >= 1);
why do we need to change this?

[pthatcher1, 2015-08-19 21:38:53]

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:398: bool CheckCandidate1(const
Result& expected) {
On 2015/08/19 20:48:37, guoweis wrote:
> Can you add a comment on what this is doing?
> 
> This one looks very similar to ExpectCandidate1 and it's not clear to me what
> difference they are serving.

Added a comment.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:412: remote_type ==
expected.local_type)));
On 2015/08/19 20:48:36, guoweis wrote:
> This is a bit subtle. When IsLocalToPrflxOrTheReverse is true, the local_type
> can't be the same as expected.local_type. (it should have early return).
> 
> Can we put a comment explaining that it must be the reverse case when running
to
> here? 

The logic is that it can be either local == local && remote == remote or local
== remote or remote == local (reversed).  I think that's pretty clear, and
there's already a comment explaining it.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:430: bool CheckCandidate2(const
Result& expected) {
On 2015/08/19 20:48:37, guoweis wrote:
> comments here too.

Added a comment.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:454: void ExpectCandidate2(const
Result& expected) {
On 2015/08/19 20:48:36, guoweis wrote:
> Same here.

Added a comment.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1419: 2000);
On 2015/08/19 20:48:37, guoweis wrote:
> why do we need to change the timeout?

I found it was flaky until I bumped the timeout.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:74: assert(port->Candidates().size() >= 1);
On 2015/08/19 20:48:37, guoweis wrote:
> why do we need to change this?

It's now possible for there to be more than one candidate.  I think it's because
of peer reflex candidates.  And I think we didn't have that problem before
because the tests were GICE, and GICE didn't have the same peer reflex behavior.

[guoweis_webrtc, 2015-08-19 21:56:42]

lgtm.

Spot some misspelling.

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/1291363005/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:74: assert(port->Candidates().size() >= 1);
On 2015/08/19 21:38:53, pthatcher1 wrote:
> On 2015/08/19 20:48:37, guoweis wrote:
> > why do we need to change this?
> 
> It's now possible for there to be more than one candidate.  I think it's
because
> of peer reflex candidates.  And I think we didn't have that problem before
> because the tests were GICE, and GICE didn't have the same peer reflex
behavior.

Is candidate[0] still the right one to return in this case?

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

https://codereview.webrtc.org/1291363005/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:420: // CheckCandidate1, except
that it will provider more detail about
provide.

https://codereview.webrtc.org/1291363005/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:466: // CheckCandidate2, except
that it will provider more detail about
provide.

[pthatcher1, 2015-08-19 22:03:09]

The CQ bit was checked by pthatcher@webrtc.org

[pthatcher1, 2015-08-19 22:03:10]

The patchset sent to the CQ was uploaded after l-g-t-m from guoweis@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1291363005/#ps60001 (title:
"fix spelling")

[commit-bot: I haz the power, 2015-08-19 22:03:21]

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

[commit-bot: I haz the power, 2015-08-19 22:05:12]

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

[commit-bot: I haz the power, 2015-08-19 22:05:13]

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

[pthatcher1, 2015-08-20 03:38:05]

Committed patchset #4 (id:60001) manually as
081f34b564e1a26ffbbe9515eba1fef7c736fdde (presubmit successful).