Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Depends on this CL in order to work in Chromium:
https://codereview.chromium.org/1976673002/

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944008f75825971802

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

[Taylor Brandstetter, 2016-05-05 19:56:22]

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

[Taylor Brandstetter, 2016-05-05 19:56:23]

Peter and Honghai, please take a look.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
File webrtc/p2p/base/fakeportallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:108: turn_servers_(turn_servers) {
I started passing the servers into the fake session in order to identify the
session for tests.

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.cc:477: }
You might think "we shouldn't use pooled sessions when doing an ICE restart".
But one use case provided by JSEP is calling SetConfiguration to change the set
of ICE servers (which refreshes the pool), then calling setLocalDescription
later to do an ICE restart. In this case the (new) pooled session *should* be
used.

So, if the application *doesn't* want pooled sessions to be used, it should just
call SetConfiguration with a pool size of 0.

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:331:
channel->MaybeStartGathering();
This needed to be moved because when using a pooled session, the
P2PTransportChannel can signal candidates synchronously inside of
MaybeStartGathering.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator_unittest.cc:9: */
Aside from OnPortReady, OnCandidatesReady, OnCandidatesAllocationDone, and the
new test, all changes in this file are just formatting.

[pthatcher1, 2016-05-05 21:51:26]

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:555: &cricket::PortAllocator::SetConfiguration,
port_allocator_.get(),
I'm a little confused.  Some things on PortAllocator are accessed on the worker
thread (SetConfiguration) and others on the signaling thread (flags, set_flags,
set_step_delay).  It would be really nice to have the port_allocator_ only
accessed on one thread, even if that means the port allocator does its own
thread hopping, or making all calls here do thread hopping.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:296: int ice_candidate_pool_size = 0;
While you're in here, could  you use this style for all the other fields?  It
will make the constructor a lot nicer.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
File webrtc/p2p/base/fakeportallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:127: return ready_ports_;
ready_ports()?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:130: return candidates_;
candidates()?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:132: bool CandidatesAllocationDone() const
override { return allocation_done_; }
allocation_done()?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:160: int transport_info_updates() const {
return transport_info_updates_; }
transport_info_update_count_?

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.cc:290: }
Instead of having a bool passed in, I think it might be cleaner to have two
methods:

AddAllocatorSession(session);   // Adds, but doesn't start
AddAndStartAllocatorSession(session);   // Adds and starts

Or just have AddAllocatorSession(session) and StartAllocatorSession(session) and
have the caller call two methods instead of AddAndStartAllocatorSession.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.cc:291: }
Actually, is StartGettingPorts a no-op if it's already started?  If so, couldn't
we just always call StartGettingPorts and avoid this bool?

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:331:
channel->MaybeStartGathering();
On 2016/05/05 19:56:23, Taylor Brandstetter wrote:
> This needed to be moved because when using a pooled session, the
> P2PTransportChannel can signal candidates synchronously inside of
> MaybeStartGathering.

Should we change that by having MaybeStartGathering do a thread post?

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1607:
allocator_2->turn_servers(), 1);
Can you give this value of 1 a name to make it more clear what it is?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/port.h#newcod...
webrtc/p2p/base/port.h:192: const std::string& password);
Why is the ufrag copied but on the password?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/port.h#newcod...
webrtc/p2p/base/port.h:192: const std::string& password);
I think SetIceParameters might be a better name.  It's almost exactly the same
as the ORTC IceParameters.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:176: virtual void
UpdateTransportInformationInternal() {}
And here, SetIceParametersInternal might be a better name.

Actually, why not just make SetIceParameters protected and let the unit test
override the whole things and call the base SetIceParameters first instead of
having SetIceParameters call this thing last?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:202: friend class PortAllocator;
Why do we need this?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:208: // but they aren't guaranteed to be
synchronized.
As mentioned in another place, I think we should either say "all methods must be
called on the same thread" or "all methods can be called on any thread".  Having
it mixed is too easy to break later.  

So, we either do a thread hop here, or a thread hope for all calls, or use a
lock on the parts that need to be synchronized.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:264: const std::string& ice_pwd);
If it takes ownership, should it return a unique_ptr<PortAllocatorSession> to
make that clear?  And shouldn't it also be called something like
TakePooledSession?  

Finally, it seems weird that it both takes the session and alters it.  Can't the
caller alter it after taking it, if that's what it wants?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:267: const PortAllocatorSession*
PeekPooledSession() const;
It seems like this one should be called GetPooledSession(), since all it does is
get it.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:335: int allocated_pooled_sessions_ = 0;
Can we call these two numbers something similar, like
target_pooled_session_count_ and allocated_pooled_session_count_?  

And how is allocated_pooled_session_count_ different than
pooled_sessions_.size()?  Can you make that more clear in the comment?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
File webrtc/p2p/base/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator_unittest.cc:52: bool HasExactlyNPooledSessions(int
num_sessions) {
Why not call this TakeAllPooledSessions() and have it return the number, and
then use EXPECT_EQ(n, TakeAllPooledSessions())

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator_unittest.cc:112: SetConfigurationWithPoolSize(-1);
Are you going to verify something here?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:207: std::vector<PortInterface*>
BasicPortAllocatorSession::ReadyPorts() const {
Do we need to update the P2pPortAllocator in chromium as well?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:247: }
std::all_of might be handy here and below.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:690: if (content_name().empty()) {
Instead of having an implicit content_name().empty() == pooled(), can we just
have a pooled()?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.h:127: }
Why not just have complete() and then let callers do "ready() || complete()"?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.h:131: }
Same here: "complete() || error()"

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator_unittest.cc:1462: // Set candidate pool
size to 1.
int pool_size = 1;  would be more clear.

[Taylor Brandstetter, 2016-05-06 03:53:35]

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:555: &cricket::PortAllocator::SetConfiguration,
port_allocator_.get(),
On 2016/05/05 21:51:24, pthatcher1 wrote:
> I'm a little confused.  Some things on PortAllocator are accessed on the
worker
> thread (SetConfiguration) and others on the signaling thread (flags,
set_flags,
> set_step_delay).  It would be really nice to have the port_allocator_ only
> accessed on one thread, even if that means the port allocator does its own
> thread hopping, or making all calls here do thread hopping. 

That's the way it's always been. I agree it should be fixed, I just thought it
may be better done in a separate CL. It seems... *relatively* simple though so
I'll do it in this one (I can revert if you think it's too complex).

This makes me realize we need more PeerConnection tests. For example, making
sure the PeerConnection calls PortAllocator::SetConfiguration AFTER applying the
other settings to the port allocator. Don't l-g-t-m the CL until I've done that.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:296: int ice_candidate_pool_size = 0;
On 2016/05/05 21:51:24, pthatcher1 wrote:
> While you're in here, could  you use this style for all the other fields?  It
> will make the constructor a lot nicer.

Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
File webrtc/p2p/base/fakeportallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:127: return ready_ports_;
On 2016/05/05 21:51:24, pthatcher1 wrote:
> ready_ports()?

An underscore name implies it's a trivial method. Since it's a pure virtual
method, it's not guaranteed that the subclass's implementation is trivial. For
BasicPortAllocator, it isn't.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:130: return candidates_;
On 2016/05/05 21:51:24, pthatcher1 wrote:
> candidates()?

See above. I called it ReadyCandidates so that we have
SignalPortReady/ReadyPorts and SignalCandidatesReady/ReadyCandidates. Though I'd
be fine with changing it to just Candidates, since there's a difference between
a Port and a ready Port, but the same isn't true for Candidates.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:132: bool CandidatesAllocationDone() const
override { return allocation_done_; }
On 2016/05/05 21:51:24, pthatcher1 wrote:
> allocation_done()?

See above.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/fakeportalloc...
webrtc/p2p/base/fakeportallocator.h:160: int transport_info_updates() const {
return transport_info_updates_; }
On 2016/05/05 21:51:24, pthatcher1 wrote:
> transport_info_update_count_?

Done.

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.cc:290: }
On 2016/05/05 21:51:24, pthatcher1 wrote:
> Instead of having a bool passed in, I think it might be cleaner to have two
> methods:
> 
> AddAllocatorSession(session);   // Adds, but doesn't start
> AddAndStartAllocatorSession(session);   // Adds and starts
> 
> Or just have AddAllocatorSession(session) and StartAllocatorSession(session)
and
> have the caller call two methods instead of AddAndStartAllocatorSession.

Not much point in making a method (StartAllocatorSession) that just calls one
other method. But moving StartGettingPorts out of here makes sense.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.cc:291: }
On 2016/05/05 21:51:24, pthatcher1 wrote:
> Actually, is StartGettingPorts a no-op if it's already started?  If so,
couldn't
> we just always call StartGettingPorts and avoid this bool?

No, it isn't a no-op.

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:331:
channel->MaybeStartGathering();
On 2016/05/05 21:51:25, pthatcher1 wrote:
> On 2016/05/05 19:56:23, Taylor Brandstetter wrote:
> > This needed to be moved because when using a pooled session, the
> > P2PTransportChannel can signal candidates synchronously inside of
> > MaybeStartGathering.
> 
> Should we change that by having MaybeStartGathering do a thread post?

No; the TransportController already posts candidates asynchronously, so there's
no issue with candidates being emitted before the SLD success callback. This is
only an issue with unit test implementation.

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1607:
allocator_2->turn_servers(), 1);
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Can you give this value of 1 a name to make it more clear what it is?

Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/port.h#newcod...
webrtc/p2p/base/port.h:192: const std::string& password);
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Why is the ufrag copied but on the password?

A typo. :)

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/port.h#newcod...
webrtc/p2p/base/port.h:192: const std::string& password);
On 2016/05/05 21:51:25, pthatcher1 wrote:
> I think SetIceParameters might be a better name.  It's almost exactly the same
> as the ORTC IceParameters.

Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:176: virtual void
UpdateTransportInformationInternal() {}
On 2016/05/05 21:51:25, pthatcher1 wrote:
> And here, SetIceParametersInternal might be a better name.
> 
> Actually, why not just make SetIceParameters protected and let the unit test
> override the whole things and call the base SetIceParameters first instead of
> having SetIceParameters call this thing last?

I don't like saying "if a derived class overrides this, it must call the base
class implementation". It's bad design; it creates unnecessary dependencies
between base and derived classes.

I did rename it though.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:202: friend class PortAllocator;
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Why do we need this?

SetTransportInformation is private. Only PortAllocator should call it.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:208: // but they aren't guaranteed to be
synchronized.
On 2016/05/05 21:51:25, pthatcher1 wrote:
> As mentioned in another place, I think we should either say "all methods must
be
> called on the same thread" or "all methods can be called on any thread". 
Having
> it mixed is too easy to break later.  
> 
> So, we either do a thread hop here, or a thread hope for all calls, or use a
> lock on the parts that need to be synchronized.  

I was just documenting the behavior we previously had, which was undocumented.
But I agree it should be fixed.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:264: const std::string& ice_pwd);
On 2016/05/05 21:51:25, pthatcher1 wrote:
> If it takes ownership, should it return a unique_ptr<PortAllocatorSession> to
> make that clear?  And shouldn't it also be called something like
> TakePooledSession?  
> 
> Finally, it seems weird that it both takes the session and alters it.  Can't
the
> caller alter it after taking it, if that's what it wants?

I was just matching CreateSession (which also returns a raw pointer even though
the caller also takes ownership). I can change that as well though.

I disagree with forcing the caller to alter it. That would impose an additional
requirement on the caller and also force SetTransportInformation to be public.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:267: const PortAllocatorSession*
PeekPooledSession() const;
On 2016/05/05 21:51:25, pthatcher1 wrote:
> It seems like this one should be called GetPooledSession(), since all it does
is
> get it.

"Get/Peek", "Pop/Peek", "Take/Get", I don't care much. "Get/Peek" matches our
current scheme though (for example, messagequeue.h).

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:335: int allocated_pooled_sessions_ = 0;
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Can we call these two numbers something similar, like
> target_pooled_session_count_ and allocated_pooled_session_count_?  
> 
> And how is allocated_pooled_session_count_ different than
> pooled_sessions_.size()?  Can you make that more clear in the comment?

It represents the number of currently pooled sessions + the ones that were given
away. So if GetPooledSession is called, pooled_sessions_.size() goes down but
allocated_pooled_sessions_ doesn't.

This is so that, for example, if you pool 2 sessions, then they both get taken,
then you change the pool size to 3, only 1 additional session gets pooled,
rather than 3.

I don't know how to make the comment any clearer though without making it too
long. What would you suggest?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
File webrtc/p2p/base/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator_unittest.cc:52: bool HasExactlyNPooledSessions(int
num_sessions) {
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Why not call this TakeAllPooledSessions() and have it return the number, and
> then use EXPECT_EQ(n, TakeAllPooledSessions())

Good idea, done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator_unittest.cc:112: SetConfigurationWithPoolSize(-1);
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Are you going to verify something here?

Just that it "DoesntCrash" (see test name).

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:207: std::vector<PortInterface*>
BasicPortAllocatorSession::ReadyPorts() const {
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Do we need to update the P2pPortAllocator in chromium as well?

No, because at this point P2PPortAllocator just derives from BasicPortAllocator
and does a bit of stuff in its constructor.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:247: }
On 2016/05/05 21:51:25, pthatcher1 wrote:
> std::all_of might be handy here and below.

Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:690: if (content_name().empty()) {
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Instead of having an implicit content_name().empty() == pooled(), can we just
> have a pooled()?

Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.h:127: }
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Why not just have complete() and then let callers do "ready() || complete()"?

I was just keeping it the way it was before. I assume it was written this way
because the check occurs so often it was convenient to only call one method. But
I'll change it.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.h:131: }
On 2016/05/05 21:51:25, pthatcher1 wrote:
> Same here: "complete() || error()"

Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator_unittest.cc:1462: // Set candidate pool
size to 1.
On 2016/05/05 21:51:25, pthatcher1 wrote:
> int pool_size = 1;  would be more clear. 

Done.

https://codereview.webrtc.org/1956453003/diff/20001/webrtc/api/webrtcsession_...
File webrtc/api/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1956453003/diff/20001/webrtc/api/webrtcsession_...
webrtc/api/webrtcsession_unittest.cc:1555: 
I think testing the candidate filter should be covered by
basicportallocaotr_unittest.cc. So I added a test in
peerconnectioninterface_unittest.cc that just verifies the IceTransportsType is
converted to a cricket:: candidate filter properly.

[pthatcher1, 2016-05-07 00:21:44]

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

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/p2ptransportc...
webrtc/p2p/base/p2ptransportchannel.cc:290: }
On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> On 2016/05/05 21:51:24, pthatcher1 wrote:
> > Instead of having a bool passed in, I think it might be cleaner to have two
> > methods:
> > 
> > AddAllocatorSession(session);   // Adds, but doesn't start
> > AddAndStartAllocatorSession(session);   // Adds and starts
> > 
> > Or just have AddAllocatorSession(session) and StartAllocatorSession(session)
> and
> > have the caller call two methods instead of AddAndStartAllocatorSession.
> 
> Not much point in making a method (StartAllocatorSession) that just calls one
> other method. But moving StartGettingPorts out of here makes sense.

That was actually the first idea I had.  Now seeing it, I agree it's better.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:202: friend class PortAllocator;
On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> On 2016/05/05 21:51:25, pthatcher1 wrote:
> > Why do we need this?
> 
> SetTransportInformation is private. Only PortAllocator should call it.

Can you put a comment explaining this?

And... is there a good way to avoid this?

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:264: const std::string& ice_pwd);
On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> On 2016/05/05 21:51:25, pthatcher1 wrote:
> > If it takes ownership, should it return a unique_ptr<PortAllocatorSession>
to
> > make that clear?  And shouldn't it also be called something like
> > TakePooledSession?  
> > 
> > Finally, it seems weird that it both takes the session and alters it.  Can't
> the
> > caller alter it after taking it, if that's what it wants?
> 
> I was just matching CreateSession (which also returns a raw pointer even
though
> the caller also takes ownership). I can change that as well though.
> 
> I disagree with forcing the caller to alter it. That would impose an
additional
> requirement on the caller and also force SetTransportInformation to be public.

I can see the merit in "PopPooledSession + SetIceParameters"  in one method. 
But we do need a good name for it.  What's the opposite of "put in a generic
pool"?  Withdraw?   Actually, "Take" might be good for that.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:267: const PortAllocatorSession*
PeekPooledSession() const;
On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> On 2016/05/05 21:51:25, pthatcher1 wrote:
> > It seems like this one should be called GetPooledSession(), since all it
does
> is
> > get it.
> 
> "Get/Peek", "Pop/Peek", "Take/Get", I don't care much. "Get/Peek" matches our
> current scheme though (for example, messagequeue.h).

I think I'd prefer Take/Get

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:335: int allocated_pooled_sessions_ = 0;
On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> On 2016/05/05 21:51:25, pthatcher1 wrote:
> > Can we call these two numbers something similar, like
> > target_pooled_session_count_ and allocated_pooled_session_count_?  
> > 
> > And how is allocated_pooled_session_count_ different than
> > pooled_sessions_.size()?  Can you make that more clear in the comment?
> 
> It represents the number of currently pooled sessions + the ones that were
given
> away. So if GetPooledSession is called, pooled_sessions_.size() goes down but
> allocated_pooled_sessions_ doesn't.
> 
> This is so that, for example, if you pool 2 sessions, then they both get
taken,
> then you change the pool size to 3, only 1 additional session gets pooled,
> rather than 3.
> 
> I don't know how to make the comment any clearer though without making it too
> long. What would you suggest?

OK, I get it now.  Yes, your comment is good already.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
File webrtc/p2p/base/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator_unittest.cc:112: SetConfigurationWithPoolSize(-1);
On 2016/05/06 03:53:35, Taylor Brandstetter wrote:
> On 2016/05/05 21:51:25, pthatcher1 wrote:
> > Are you going to verify something here?
> 
> Just that it "DoesntCrash" (see test name).

Ah.  I missed that.  Perhaps just a comment like so:

// ASSERT(doesn't crash) :)

Might be nice.

https://codereview.webrtc.org/1956453003/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1956453003/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:571: }
I like this.  Thank you.

https://codereview.webrtc.org/1956453003/diff/40001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:333: // both owned by this class and taken by
GetPooledSession.
Ah, so this will read well: taken by TakePooledSession :).

[Taylor Brandstetter, 2016-05-09 22:07:10]

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:202: friend class PortAllocator;
On 2016/05/07 00:21:44, pthatcher1 wrote:
> On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> > On 2016/05/05 21:51:25, pthatcher1 wrote:
> > > Why do we need this?
> > 
> > SetTransportInformation is private. Only PortAllocator should call it.
> 
> Can you put a comment explaining this?
> 
> And... is there a good way to avoid this?

This method can be either an implementation detail (private, as is) or part of
the interface (and called by P2PTransportChannel). I prefer the former since it
makes the interface simpler, and I can't think of why P2PTransportChannel would
get a pooled session and *not* want this method called.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:267: const PortAllocatorSession*
PeekPooledSession() const;
On 2016/05/07 00:21:44, pthatcher1 wrote:
> On 2016/05/06 03:53:34, Taylor Brandstetter wrote:
> > On 2016/05/05 21:51:25, pthatcher1 wrote:
> > > It seems like this one should be called GetPooledSession(), since all it
> does
> > is
> > > get it.
> > 
> > "Get/Peek", "Pop/Peek", "Take/Get", I don't care much. "Get/Peek" matches
our
> > current scheme though (for example, messagequeue.h).
> 
> I think I'd prefer Take/Get

I don't like it much but I'll compromise... Done.

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
File webrtc/p2p/base/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1956453003/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator_unittest.cc:112: SetConfigurationWithPoolSize(-1);
On 2016/05/07 00:21:44, pthatcher1 wrote:
> On 2016/05/06 03:53:35, Taylor Brandstetter wrote:
> > On 2016/05/05 21:51:25, pthatcher1 wrote:
> > > Are you going to verify something here?
> > 
> > Just that it "DoesntCrash" (see test name).
> 
> Ah.  I missed that.  Perhaps just a comment like so:
> 
> // ASSERT(doesn't crash) :)
> 
> Might be nice.

Done.

[pthatcher1, 2016-05-11 05:49:44]

lgtm

[Taylor Brandstetter, 2016-05-12 15:31:40]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-05-12 15:31:45]

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

[commit-bot: I haz the power, 2016-05-12 15:34:18]

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

[commit-bot: I haz the power, 2016-05-12 15:34:19]

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

[commit-bot: I haz the power, 2016-05-12 17:19:46]

Description was changed from

==========
Implement RTCConfiguration.iceCandidatePoolSize.

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.
==========

to

==========
Implement RTCConfiguration.iceCandidatePoolSize.

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed: https://crrev.com/48e9d05f510b1616c81303944008f75825971802
Cr-Commit-Position: refs/heads/master@{#12708}
==========

[commit-bot: I haz the power, 2016-05-12 17:19:47]

Patchset 7 (id:??) landed as
https://crrev.com/48e9d05f510b1616c81303944008f75825971802
Cr-Commit-Position: refs/heads/master@{#12708}

[Taylor Brandstetter, 2016-05-12 17:19:48]

Description was changed from

==========
Implement RTCConfiguration.iceCandidatePoolSize.

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed: https://crrev.com/48e9d05f510b1616c81303944008f75825971802
Cr-Commit-Position: refs/heads/master@{#12708}
==========

to

==========
Implement RTCConfiguration.iceCandidatePoolSize.

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...
==========

[Taylor Brandstetter, 2016-05-12 17:19:49]

Committed patchset #7 (id:120001) manually as
48e9d05f510b1616c81303944008f75825971802 (presubmit successful).

[Taylor Brandstetter, 2016-05-12 19:43:38]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.webrtc.org/1972043004/ by deadbeef@webrtc.org.

The reason for reverting is: Breaks remoting_unittests. They defined their own
operator== which conflicts with this one.

I'll remove the operator== in a roll CL. But until it's approved, I'm reverting
this so the FYI bots will pass..

[Taylor Brandstetter, 2016-05-13 15:14:22]

Description was changed from

==========
Implement RTCConfiguration.iceCandidatePoolSize.

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...
==========

to

==========
Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Depends on this CL in order to work in Chromium: 
https://codereview.chromium.org/1976673002/

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...
==========

[Taylor Brandstetter, 2016-05-13 15:15:23]

Description was changed from

==========
Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Depends on this CL in order to work in Chromium: 
https://codereview.chromium.org/1976673002/

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...
==========

to

==========
Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Depends on this CL in order to work in Chromium:
https://codereview.chromium.org/1976673002/

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...

Committed:
https://chromium.googlesource.com/external/webrtc/+/a1c303535fe7a29b878790479...
==========

[Taylor Brandstetter, 2016-05-13 15:15:25]

Committed patchset #7 (id:120001) manually as
a1c303535fe7a29b87879047996efa2952f9701b (presubmit successful).

[commit-bot: I haz the power, 2016-05-13 15:15:28]

Description was changed from

==========
Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Depends on this CL in order to work in Chromium:
https://codereview.chromium.org/1976673002/

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...

Committed:
https://chromium.googlesource.com/external/webrtc/+/a1c303535fe7a29b878790479...
==========

to

==========
Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

Depends on this CL in order to work in Chromium:
https://codereview.chromium.org/1976673002/

It works by creating pooled PortAllocatorSessions which can be picked up
by a P2PTransportChannel when needed (after a local description is set).

This can optimize candidate gathering time when there is some time between
creating a PeerConnection and setting a local description.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48e9d05f510b1616c81303944...

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

[tommi, 2016-05-14 10:23:46]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.webrtc.org/1982513002/ by tommi@webrtc.org.

The reason for reverting is: Speculative revert to see if this could be
triggering an error on the Memcheck build bot where
EndToEndTest.SendsAndReceivesH264 consistently times out:

https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck

.