Fixing the behavior of the candidate filter with pooled candidates.

Fixing the behavior of the candidate filter with pooled candidates.

According to JSEP, the candidate filter does not affect pooled
candidates because they can be filtered once they're ready to be
surfaced to the application.

So, pooled port allocator sessions will use a filter of CF_ALL, with a
new filter applied when the session is taken by a P2PTransportChannel.

When the filter is applied:
* Some candidates may no longer be returned by ReadyCandidates()
* Some candidates may no longer have a "related address" (for privacy)
* Some ports may no longer be returned by ReadyPorts()

To simplify this, the candidate filtering logic is now moved up from
the Ports to the BasicPortAllocator, with some helper methods to perform
the filtering and stripping out of data.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/417eebe5dd5b7ee9e9e8da11a48fc8a19df4eca3

[Taylor Brandstetter, 2016-05-19 22:11:44]

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

[Taylor Brandstetter, 2016-05-19 22:11:45]

PTAL

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

https://codereview.webrtc.org/1998813002/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:304: // TODO(mallinath) - Do transition check?
Transition check now done (in BasicPortAllocatorSession)

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

https://codereview.webrtc.org/1998813002/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:752: void
BasicPortAllocatorSession::OnShake() {
Will remove this in a later CL.

[honghaiz3, 2016-05-20 00:11:11]

lgtm

[pthatcher1, 2016-05-20 01:23:54]

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

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:147: // by ReadyPorts().
Perhaps make it clear that it's for already-gathered candidates, perhaps from
pooled gathering.

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.h:151: bool ready_ = false;
I think it would be more clear to call this has_pairable_candidates(), along
with set_has_pairable_candidates().

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator_unittest.cc:979: EXPECT_EQ(4U,
ports_.size());
Can you make it clear that 7 candidates on 4 ports means !CF_RELAY?

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator_unittest.cc:1541: for (size_t i = 0; i <
candidates.size(); ++i) {
c++11 for-loop?

[Taylor Brandstetter, 2016-05-20 21:03:37]

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

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:147: // by ReadyPorts().
On 2016/05/20 01:23:54, pthatcher1 wrote:
> Perhaps make it clear that it's for already-gathered candidates, perhaps from
> pooled gathering.

Done.

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.h:151: bool ready_ = false;
On 2016/05/20 01:23:54, pthatcher1 wrote:
> I think it would be more clear to call this has_pairable_candidates(), along
> with set_has_pairable_candidates().

Done.

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator_unittest.cc:979: EXPECT_EQ(4U,
ports_.size());
On 2016/05/20 01:23:54, pthatcher1 wrote:
> Can you make it clear that 7 candidates on 4 ports means !CF_RELAY?

Done.

https://codereview.webrtc.org/1998813002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator_unittest.cc:1541: for (size_t i = 0; i <
candidates.size(); ++i) {
On 2016/05/20 01:23:54, pthatcher1 wrote:
> c++11 for-loop?

Done.

[pthatcher1, 2016-05-20 21:46:36]

lgtm

[Taylor Brandstetter, 2016-05-23 17:14:15]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-05-23 17:14:16]

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1998813002/#ps80001 (title:
"Merge with master (no more OnShake!)")

[commit-bot: I haz the power, 2016-05-23 17:14:23]

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

[commit-bot: I haz the power, 2016-05-23 17:15:32]

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

[commit-bot: I haz the power, 2016-05-23 17:15:33]

Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7708)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7694)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10029)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15239)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13889)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/15355)
  win_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/9481)

[Taylor Brandstetter, 2016-05-23 17:55:24]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-05-23 17:55:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1998813002/#ps100001 (title:
"Merge with master again")

[commit-bot: I haz the power, 2016-05-23 17:55:34]

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

[commit-bot: I haz the power, 2016-05-23 17:56:51]

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

[commit-bot: I haz the power, 2016-05-23 17:56:52]

Try jobs failed on following builders:
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15242)
  linux_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/11124)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/12407)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11581)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3392)

[Taylor Brandstetter, 2016-05-23 18:21:59]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-05-23 18:22:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1998813002/#ps120001 (title:
"Updating unit tests.")

[commit-bot: I haz the power, 2016-05-23 18:22:08]

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

[commit-bot: I haz the power, 2016-05-23 18:36:02]

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

[commit-bot: I haz the power, 2016-05-23 18:36:03]

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

[commit-bot: I haz the power, 2016-05-23 23:02:32]

Description was changed from

==========
Fixing the behavior of the candidate filter with pooled candidates.

According to JSEP, the candidate filter does not affect pooled
candidates because they can be filtered once they're ready to be
surfaced to the application.

So, pooled port allocator sessions will use a filter of CF_ALL, with a
new filter applied when the session is taken by a P2PTransportChannel.

When the filter is applied:
* Some candidates may no longer be returned by ReadyCandidates()
* Some candidates may no longer have a "related address" (for privacy)
* Some ports may no longer be returned by ReadyPorts()

To simplify this, the candidate filtering logic is now moved up from
the Ports to the BasicPortAllocator, with some helper methods to perform
the filtering and stripping out of data.
==========

to

==========
Fixing the behavior of the candidate filter with pooled candidates.

According to JSEP, the candidate filter does not affect pooled
candidates because they can be filtered once they're ready to be
surfaced to the application.

So, pooled port allocator sessions will use a filter of CF_ALL, with a
new filter applied when the session is taken by a P2PTransportChannel.

When the filter is applied:
* Some candidates may no longer be returned by ReadyCandidates()
* Some candidates may no longer have a "related address" (for privacy)
* Some ports may no longer be returned by ReadyPorts()

To simplify this, the candidate filtering logic is now moved up from
the Ports to the BasicPortAllocator, with some helper methods to perform
the filtering and stripping out of data.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/417eebe5dd5b7ee9e9e8da11a48fc8a19df4eca3
Cr-Commit-Position: refs/heads/master@{#12856}
==========

[commit-bot: I haz the power, 2016-05-23 23:02:34]

Patchset 9 (id:??) landed as
https://crrev.com/417eebe5dd5b7ee9e9e8da11a48fc8a19df4eca3
Cr-Commit-Position: refs/heads/master@{#12856}

[Taylor Brandstetter, 2016-05-23 23:02:34]

Description was changed from

==========
Fixing the behavior of the candidate filter with pooled candidates.

According to JSEP, the candidate filter does not affect pooled
candidates because they can be filtered once they're ready to be
surfaced to the application.

So, pooled port allocator sessions will use a filter of CF_ALL, with a
new filter applied when the session is taken by a P2PTransportChannel.

When the filter is applied:
* Some candidates may no longer be returned by ReadyCandidates()
* Some candidates may no longer have a "related address" (for privacy)
* Some ports may no longer be returned by ReadyPorts()

To simplify this, the candidate filtering logic is now moved up from
the Ports to the BasicPortAllocator, with some helper methods to perform
the filtering and stripping out of data.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/417eebe5dd5b7ee9e9e8da11a48fc8a19df4eca3
Cr-Commit-Position: refs/heads/master@{#12856}
==========

to

==========
Fixing the behavior of the candidate filter with pooled candidates.

According to JSEP, the candidate filter does not affect pooled
candidates because they can be filtered once they're ready to be
surfaced to the application.

So, pooled port allocator sessions will use a filter of CF_ALL, with a
new filter applied when the session is taken by a P2PTransportChannel.

When the filter is applied:
* Some candidates may no longer be returned by ReadyCandidates()
* Some candidates may no longer have a "related address" (for privacy)
* Some ports may no longer be returned by ReadyPorts()

To simplify this, the candidate filtering logic is now moved up from
the Ports to the BasicPortAllocator, with some helper methods to perform
the filtering and stripping out of data.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

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

[Taylor Brandstetter, 2016-05-23 23:02:35]

Committed patchset #9 (id:160001) manually as
417eebe5dd5b7ee9e9e8da11a48fc8a19df4eca3 (presubmit successful).