Removing webrtc::PortAllocatorFactoryInterface.

Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator,
making PortAllocatorFactoryInterface redundant. This CL also
moves SetNetworkIgnoreMask to PortAllocator.

R=phoglund@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/0c7e9f540b282d60b94081f601a1694054d8646e
Cr-Commit-Position: refs/heads/master@{#11139}

[Taylor Brandstetter, 2015-12-14 20:50:48]

Description was changed from

==========
Removing PortAllocatorFactoryInterface.
==========

to

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator, making
PortAllocatorFactoryInterface redundant. This CL also moves SetNetworkIgnoreMask
to PortAllocator.
==========

[Taylor Brandstetter, 2015-12-14 20:51:22]

Description was changed from

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator, making
PortAllocatorFactoryInterface redundant. This CL also moves SetNetworkIgnoreMask
to PortAllocator.
==========

to

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator, 
making PortAllocatorFactoryInterface redundant. This CL also
moves SetNetworkIgnoreMask to PortAllocator.
==========

[Taylor Brandstetter, 2015-12-14 20:54:33]

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

[Taylor Brandstetter, 2015-12-14 20:54:34]

pthatcher: PTAL

phoglund: Are you ok with what I did with SetNetworkIgnoreMask? It's going away
with PortAllocatorFactoryInterface, but I added it as a virtual method to
PortAllocator and NetworkManager (see portallocator.h and network.h).

[pthatcher1, 2015-12-15 07:58:45]

This is a really nice cleanup.

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

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:542: }
Can you leave a comment explaining why this is important (that candidates for
different turn servers need different priorities)?

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectionfactory.cc:267:
allocator->SetNetworkIgnoreMask(options_.network_ignore_mask);
So is this is really just a way to set the network ignore mask of the underlying
network manager, which will then be used to filter out networks when used by the
allocator in BasicPortAllocatorSession::DoAllocate amd
BasicPortAllocatorSession::OnNetworksChanged()?

If so, I have a few suggestions:

1.  Would it make sense to just skip setting the network mask in the
NetworkManager and instead apply filtering in the PortAllocator?  We could do
inside of DoAllocate and OnNetworksChanged quite easily.

2.  If that's infeasible, would it make sense to add a network_manager() getter
on the allocator, so the above line of code would be
allocator->newtork_manager()->SetNetworkIgnoreMask(...).  That would perhaps be
cleaner and make it more clear what the scope of the change is.

3.  If neither of those are feasible, then could we at least document what is
really going on here at this point (that the NetworkManager of the PortAllocator
is being modified, not really the PortAllocator)?

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectionfactory_unittest.cc (right):

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectionfactory_unittest.cc:109: raw_port_allocator_ =
port_allocator_.get();
Why not just have a getter of port_allocator() that returns a raw pointer
instead of copying the pointer?

[phoglund, 2015-12-15 12:41:31]

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectionfactory.cc:267:
allocator->SetNetworkIgnoreMask(options_.network_ignore_mask);
On 2015/12/15 07:58:44, pthatcher1 wrote:
> So is this is really just a way to set the network ignore mask of the
underlying
> network manager, which will then be used to filter out networks when used by
the
> allocator in BasicPortAllocatorSession::DoAllocate amd
> BasicPortAllocatorSession::OnNetworksChanged()?
> 
> If so, I have a few suggestions:
> 
> 1.  Would it make sense to just skip setting the network mask in the
> NetworkManager and instead apply filtering in the PortAllocator?  We could do
> inside of DoAllocate and OnNetworksChanged quite easily.
> 
> 2.  If that's infeasible, would it make sense to add a network_manager()
getter
> on the allocator, so the above line of code would be
> allocator->newtork_manager()->SetNetworkIgnoreMask(...).  That would perhaps
be
> cleaner and make it more clear what the scope of the change is.
> 
> 3.  If neither of those are feasible, then could we at least document what is
> really going on here at this point (that the NetworkManager of the
PortAllocator
> is being modified, not really the PortAllocator)?

Yeah. When I made the network mask change, I had to go through the port
allocator to get the information in options_ to the network manager. I don't
fully understand the role of the port allocator: does it make sense to tell it
to apply a ignore mask? If it does, 1) sounds great. The network manager already
classifies networks as loopback or not, so one could move all whole ignore logic
to the port allocator and make it do the filtering. Just watch out if there are
other users of the network manager, that doesn't go through the port allocator.

Also, please be careful so that the mask is set before the networks are
enumerated. Otherwise the list of networks will be incorrect.

[Taylor Brandstetter, 2015-12-15 19:30:47]

On 2015/12/15 12:41:31, phoglund wrote:
>
https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
> File talk/app/webrtc/peerconnectionfactory.cc (right):
> 
>
https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
> talk/app/webrtc/peerconnectionfactory.cc:267:
> allocator->SetNetworkIgnoreMask(options_.network_ignore_mask);
> On 2015/12/15 07:58:44, pthatcher1 wrote:
> > So is this is really just a way to set the network ignore mask of the
> underlying
> > network manager, which will then be used to filter out networks when used by
> the
> > allocator in BasicPortAllocatorSession::DoAllocate amd
> > BasicPortAllocatorSession::OnNetworksChanged()?
> > 
> > If so, I have a few suggestions:
> > 
> > 1.  Would it make sense to just skip setting the network mask in the
> > NetworkManager and instead apply filtering in the PortAllocator?  We could
do
> > inside of DoAllocate and OnNetworksChanged quite easily.
> > 
> > 2.  If that's infeasible, would it make sense to add a network_manager()
> getter
> > on the allocator, so the above line of code would be
> > allocator->newtork_manager()->SetNetworkIgnoreMask(...).  That would perhaps
> be
> > cleaner and make it more clear what the scope of the change is.
> > 
> > 3.  If neither of those are feasible, then could we at least document what
is
> > really going on here at this point (that the NetworkManager of the
> PortAllocator
> > is being modified, not really the PortAllocator)?
> 
> Yeah. When I made the network mask change, I had to go through the port
> allocator to get the information in options_ to the network manager. I don't
> fully understand the role of the port allocator: does it make sense to tell it
> to apply a ignore mask? If it does, 1) sounds great. The network manager
already
> classifies networks as loopback or not, so one could move all whole ignore
logic
> to the port allocator and make it do the filtering. Just watch out if there
are
> other users of the network manager, that doesn't go through the port
allocator.
> 
> Also, please be careful so that the mask is set before the networks are
> enumerated. Otherwise the list of networks will be incorrect.

I think it does make sense to move the ignore mask to the port allocator. If an
application uses the default network manager (Chrome doesn't, but native apps
do), then multiple PCs *will* end up sharing the same network manager. Which
gives more incentive to do filtering at the PortAllocator level (specific to one
PC).

[Taylor Brandstetter, 2015-12-15 21:31:54]

Ok, I moved the network ignore mask logic to BasicPortAllocator. One advantage
of this approach is that there's no more "TODO: implement in Chromium", since
it's all handled in WebRTC land.

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

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:542: }
On 2015/12/15 07:58:44, pthatcher1 wrote:
> Can you leave a comment explaining why this is important (that candidates for
> different turn servers need different priorities)?

Done.

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectionfactory_unittest.cc (right):

https://codereview.webrtc.org/1520963002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectionfactory_unittest.cc:109: raw_port_allocator_ =
port_allocator_.get();
On 2015/12/15 07:58:44, pthatcher1 wrote:
> Why not just have a getter of port_allocator() that returns a raw pointer
> instead of copying the pointer?

I tried this route, but at the point where I was writing
"static_cast<cricket::FakePortAllocator*>(static_cast<PeerConnection*>(pc_.get())->port_allocator());",
it started feeling like more trouble than it was worth. Plus, I don't like
adding public functions only used for unit tests.

[pthatcher1, 2015-12-15 23:53:18]

lgtm



I like it.

https://codereview.webrtc.org/1520963002/diff/80001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1520963002/diff/80001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:333: if
(!(allocator_->network_ignore_mask() & network->type())) {
Might be slightly more readable as:

bool ignored = (allocator_->network_ignore_mask() & network->type())
if (!ignored) {
  networks->push_back(network);
}

[phoglund, 2015-12-16 09:15:16]

lgtm

[pthatcher1, 2015-12-17 22:02:03]

lgtm

https://codereview.webrtc.org/1520963002/diff/160001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1520963002/diff/160001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:337: networks->end());
Could be slightly more readable as:

auto filtered = [this](rtc::Network* network) {
  return allocator_->network_ignore_mask() & network->type();
};
...

[Taylor Brandstetter, 2015-12-17 23:16:24]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-12-17 23:16:25]

The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1520963002/#ps160001 (title:
"Fixing patch conflicts")

[commit-bot: I haz the power, 2015-12-17 23:16:37]

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

[commit-bot: I haz the power, 2015-12-17 23:21:27]

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

[commit-bot: I haz the power, 2015-12-17 23:21:28]

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

[Taylor Brandstetter, 2015-12-17 23:52:58]

deadbeef@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[Taylor Brandstetter, 2015-12-17 23:52:59]

Zeke: Could you take a look at conductor.cc?

[tkchin_webrtc, 2015-12-21 12:41:36]

On 2015/12/17 23:52:59, Taylor Brandstetter wrote:
> Zeke: Could you take a look at conductor.cc?

lgtm

[Taylor Brandstetter, 2015-12-29 18:46:03]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-12-29 18:46:16]

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

[commit-bot: I haz the power, 2015-12-29 18:52:54]

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

[commit-bot: I haz the power, 2015-12-29 18:52:54]

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

[Taylor Brandstetter, 2015-12-29 22:15:07]

Description was changed from

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator, 
making PortAllocatorFactoryInterface redundant. This CL also
moves SetNetworkIgnoreMask to PortAllocator.
==========

to

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator,
making PortAllocatorFactoryInterface redundant. This CL also
moves SetNetworkIgnoreMask to PortAllocator.

R=phoglund@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

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

[Taylor Brandstetter, 2015-12-29 22:15:11]

Committed patchset #10 (id:180001) manually as
0c7e9f540b282d60b94081f601a1694054d8646e (presubmit successful).

[commit-bot: I haz the power, 2015-12-29 22:15:19]

Description was changed from

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator,
making PortAllocatorFactoryInterface redundant. This CL also
moves SetNetworkIgnoreMask to PortAllocator.

R=phoglund@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

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

to

==========
Removing webrtc::PortAllocatorFactoryInterface.

ICE servers are now passed directly into PortAllocator,
making PortAllocatorFactoryInterface redundant. This CL also
moves SetNetworkIgnoreMask to PortAllocator.

R=phoglund@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/0c7e9f540b282d60b94081f601a1694054d8646e
Cr-Commit-Position: refs/heads/master@{#11139}
==========

[commit-bot: I haz the power, 2015-12-29 22:15:20]

Patchset 10 (id:??) landed as
https://crrev.com/0c7e9f540b282d60b94081f601a1694054d8646e
Cr-Commit-Position: refs/heads/master@{#11139}