Remove candidates when doing continual gathering

When doing candidate re-gathering in the same generation, Remove the existing local candidate on the same network
and signaling the remote side to remove its remote candidate by setting the candidate priority to 0.

BUG=

Committed: https://crrev.com/84430da6817ce69c53bfad088be5c9df8b420f01
Cr-Commit-Position: refs/heads/master@{#11958}

[honghaiz3, 2016-01-29 22:08:43]

Description was changed from

==========
When doing ICE re-gathering in the same generation, Remove the existing local
candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

to

==========
When doing ICE re-gathering in the same generation, Remove the existing local
candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

[honghaiz3, 2016-01-29 22:09:55]

Description was changed from

==========
When doing ICE re-gathering in the same generation, Remove the existing local
candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

to

==========
When doing candidate re-gathering in the same generation, Remove the existing
local candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

[honghaiz3, 2016-02-09 19:48:27]

Patchset #2 (id:20001) has been deleted

[honghaiz3, 2016-02-09 19:48:36]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2016-02-09 19:48:43]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-02-09 19:50:35]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2016-02-09 19:50:46]

Patchset #1 (id:80001) has been deleted

[honghaiz3, 2016-02-09 19:58:10]

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

[honghaiz3, 2016-02-09 19:58:10]

I am working on the tests. 
It will be great if you can take a first look.

[pthatcher1, 2016-02-10 00:01:41]

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/jsepicec...
File talk/app/webrtc/jsepicecandidate.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/jsepicec...
talk/app/webrtc/jsepicecandidate.h:85: virtual int remove(const
IceCandidateInterface* target);
"target" seems like a strange name.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcsd...
File talk/app/webrtc/webrtcsdp.cc (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcsd...
talk/app/webrtc/webrtcsdp.cc:1748: type = kCandidatePrflx;
I need to think about this some more, if it makes sense to remove a prflx
candidate.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1197: if (candidate->candidate().priority() ==
0) {
I'll need to think about this more to think if this is the right way to signal
candidate removal.  It is interesting, but I'm afraid that apps not expecting it
will be surprised in a bad way.  The alternative is to have new methods of
PeerConnection.onicecandidate and PeerConnection.removeIceCandidate.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1649:
ice_observer_->OnIceCandidate(&candidate);
I think having an event of OnIceCandidateRemoved might make more sense, but we
need to think about the tradeoffs.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:405: bool StopUsingCandidate(const
IceCandidateInterface* candidate);
Can use change UseCandidate to be AddCandidate, and then call this
RemoveCandidate?

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/base/network.h#ne...
webrtc/base/network.h:276: sigslot::signal1<const Network*> SignalInterfaceDown;
Should this be SignalActiveChanged or SignalInactive?

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:175: bool IsWeaklyEquivalent(const Candidate& c)
const {
We should make this something more removal-specific, like MatchesForRemoval

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:231: void OnPortStopped(PortInterface*
port);
How is OnPortStopped different than OnPortDestroyed?

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

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:199: network_->SignalInterfaceDown.connect(this,
&Port::OnNetworkDown);
Should this be OnNetworkInactive?

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:635: SignalStopped(this);
Should we destroy the port as well?  If not, should we just call this
SignalNetworkInactive?

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:295: // If candidate is not acceptable, returns
false and sets error.
candidate => a candidate

[honghaiz3, 2016-02-10 19:22:44]

Addressed comments although still working on tests.
I looked through the code, and don't think it should cause problem with old
clients using priority 0. In the worst case, it is passed down to the
p2ptransportchannel which will detect that is a duplicate and will not create
new connections.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/jsepicec...
File talk/app/webrtc/jsepicecandidate.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/jsepicec...
talk/app/webrtc/jsepicecandidate.h:85: virtual int remove(const
IceCandidateInterface* target);
On 2016/02/10 00:01:40, pthatcher1 wrote:
> "target" seems like a strange name.

Done.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcsd...
File talk/app/webrtc/webrtcsdp.cc (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcsd...
talk/app/webrtc/webrtcsdp.cc:1748: type = kCandidatePrflx;
On 2016/02/10 00:01:40, pthatcher1 wrote:
> I need to think about this some more, if it makes sense to remove a prflx
> candidate.
OK. Let me know. 
I think it need and can be removed.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1197: if (candidate->candidate().priority() ==
0) {
On 2016/02/10 00:01:41, pthatcher1 wrote:
> I'll need to think about this more to think if this is the right way to signal
> candidate removal.  It is interesting, but I'm afraid that apps not expecting
it
> will be surprised in a bad way.  The alternative is to have new methods of
> PeerConnection.onicecandidate and PeerConnection.removeIceCandidate.
Looking at the code, I think there are two possible surprises:
1. when an old client receives a signal to remove a peer reflexive candidate. It
will fail to parse the candidate. But it does not cause real problems. It will
return a null ptr as the candidate which will be discarded later in the process.
2. The app may hold the candidates and push down as  a batch. This is not a
problem either as long as the ordering of the signaled candidates  are preserved
(if not, it will have issues too even if we use a new signaling like
"remove-candidate"). 
I believe the alternative way you mentioned will signal this as something like
"remove-candidate: ...."
I think we have discussed that approach with Justin before and he suggested
using a priority 0 to indicate a candidate being removed.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1649:
ice_observer_->OnIceCandidate(&candidate);
On 2016/02/10 00:01:40, pthatcher1 wrote:
> I think having an event of OnIceCandidateRemoved might make more sense, but we
> need to think about the tradeoffs.
See my reply above.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:405: bool StopUsingCandidate(const
IceCandidateInterface* candidate);
On 2016/02/10 00:01:41, pthatcher1 wrote:
> Can use change UseCandidate to be AddCandidate, and then call this
> RemoveCandidate?

Done.Using AddRemoteCandidate RemoveRemoteCandidate.

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/base/network.h#ne...
webrtc/base/network.h:276: sigslot::signal1<const Network*> SignalInterfaceDown;
On 2016/02/10 00:01:41, pthatcher1 wrote:
> Should this be SignalActiveChanged or SignalInactive?

Done.

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:175: bool IsWeaklyEquivalent(const Candidate& c)
const {
On 2016/02/10 00:01:41, pthatcher1 wrote:
> We should make this something more removal-specific, like MatchesForRemoval

Done.

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:231: void OnPortStopped(PortInterface*
port);
On 2016/02/10 00:01:41, pthatcher1 wrote:
> How is OnPortStopped different than OnPortDestroyed?

PortStopped is triggered by network becoming inactive. 
PortDestroyed is triggered by port being deleted (by itself). 
In the former case we need to signal candidate removal. In the latter case
that's not needed (the remote candidates should have been removed by ICE
restart).

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

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:199: network_->SignalInterfaceDown.connect(this,
&Port::OnNetworkDown);
On 2016/02/10 00:01:41, pthatcher1 wrote:
> Should this be OnNetworkInactive?

Done.

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:635: SignalStopped(this);
On 2016/02/10 00:01:41, pthatcher1 wrote:
> Should we destroy the port as well?  If not, should we just call this
> SignalNetworkInactive?

Done. Changed to SignalNetworkInactive.
Better not to destroy ports because connections are still using the port.

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1648813004/diff/100001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:295: // If candidate is not acceptable, returns
false and sets error.
On 2016/02/10 00:01:41, pthatcher1 wrote:
> candidate => a candidate

Done.

[Taylor Brandstetter, 2016-02-10 21:58:08]

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsep.h
File talk/app/webrtc/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsep.h#n...
talk/app/webrtc/jsep.h:92: virtual int remove(const IceCandidateInterface*
candidate) = 0;
Does this method need to be on IceCandidateCollection, or just
JsepIceCandidateCollection?

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsepicec...
File talk/app/webrtc/jsepicecandidate.cc (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsepicec...
talk/app/webrtc/jsepicecandidate.cc:106: return candidates_.size() -
original_size;
This will return a negative number, is that right?

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1197: if (candidate->candidate().priority() ==
0) {
Using a priority of 0 doesn't seem like an ideal long term solution. I expected
that we'd add a "removeIceCandidate" method, and "oncandidateremoved" event.
Just my two cents.

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1717: const cricket::ContentInfo*
WebRtcSession::GetRemoteMediaContent(
As long as you're making this a standalone method, could you make it work if the
candidate only has "sdp_mid" set, and not "sdp_mline_index"? This is allowed by
the spec, technically.

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:179: 
Why can't IsEquivalent be used?

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:219: void
OnCandidatesRemoved(TransportChannelImpl* channel, const Candidates& cc);
Just curious, why name the variable cc?

[pthatcher1, 2016-02-11 20:25:51]

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcsd...
File talk/app/webrtc/webrtcsdp.cc (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcsd...
talk/app/webrtc/webrtcsdp.cc:1748: type = kCandidatePrflx;
On 2016/02/10 19:22:43, honghaiz3 wrote:
> On 2016/02/10 00:01:40, pthatcher1 wrote:
> > I need to think about this some more, if it makes sense to remove a prflx
> > candidate.
> OK. Let me know. 
> I think it need and can be removed.

I think it makes sense to signal a prflx in a removal event, but not in an
addition event.

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1197: if (candidate->candidate().priority() ==
0) {
On 2016/02/10 19:22:43, honghaiz3 wrote:
> On 2016/02/10 00:01:41, pthatcher1 wrote:
> > I'll need to think about this more to think if this is the right way to
signal
> > candidate removal.  It is interesting, but I'm afraid that apps not
expecting
> it
> > will be surprised in a bad way.  The alternative is to have new methods of
> > PeerConnection.onicecandidate and PeerConnection.removeIceCandidate.
> Looking at the code, I think there are two possible surprises:
> 1. when an old client receives a signal to remove a peer reflexive candidate.
It
> will fail to parse the candidate. But it does not cause real problems. It will
> return a null ptr as the candidate which will be discarded later in the
process.
> 2. The app may hold the candidates and push down as  a batch. This is not a
> problem either as long as the ordering of the signaled candidates  are
preserved
> (if not, it will have issues too even if we use a new signaling like
> "remove-candidate"). 
> I believe the alternative way you mentioned will signal this as something like
> "remove-candidate: ...."
> I think we have discussed that approach with Justin before and he suggested
> using a priority 0 to indicate a candidate being removed. 
> 

I think having removals come out of the "added" event and into the "add" method
is the part that's surprising and risky.  

It's not just risky because of what browsers will do.  It's risky because of
what JS applications and servers and gateways and random remote endpoints will
do.  "Trickle remove" is just such a new thing that I'm sure it will cause
problems somewhere if it's implicitly done using the same events and methods and
signals, which is why I think we need new events, methods, and signals for
removal.

We can discuss it again with Justin, but I think he'll agree once he considers
how many things won't expect this and may break.

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1197: if (candidate->candidate().priority() ==
0) {
On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> Using a priority of 0 doesn't seem like an ideal long term solution. I
expected
> that we'd add a "removeIceCandidate" method, and "oncandidateremoved" event.
> Just my two cents.

I agree.

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:179: 
On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> Why can't IsEquivalent be used?

Because then we have to signal everything.  It would be nice to just have to
signal what's needed (the 3 tuple).

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:219: void
OnCandidatesRemoved(TransportChannelImpl* channel, const Candidates& cc);
On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> Just curious, why name the variable cc?

Agreed

[Taylor Brandstetter, 2016-02-11 20:55:17]

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:179: 
On 2016/02/11 20:25:51, pthatcher1 wrote:
> On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> > Why can't IsEquivalent be used?
> 
> Because then we have to signal everything.  It would be nice to just have to
> signal what's needed (the 3 tuple).

Is there a reason why signaling the whole candidate is difficult?

[pthatcher1, 2016-02-11 23:54:09]

On 2016/02/11 20:55:17, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidate.h
> File webrtc/p2p/base/candidate.h (right):
> 
>
https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidat...
> webrtc/p2p/base/candidate.h:179: 
> On 2016/02/11 20:25:51, pthatcher1 wrote:
> > On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> > > Why can't IsEquivalent be used?
> > 
> > Because then we have to signal everything.  It would be nice to just have to
> > signal what's needed (the 3 tuple).
> 
> Is there a reason why signaling the whole candidate is difficult?

No, but it seems silly to require signalling things that aren't required.  If
the application wants to optimize the signalling messages to be as small as
possible, it should be free to do so, and if require unnecessary information,
then we're just overconstraining what the application needs to signal.

[Taylor Brandstetter, 2016-02-11 23:59:36]

On 2016/02/11 23:54:09, pthatcher1 wrote:
> On 2016/02/11 20:55:17, Taylor Brandstetter wrote:
> >
>
https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidate.h
> > File webrtc/p2p/base/candidate.h (right):
> > 
> >
>
https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidat...
> > webrtc/p2p/base/candidate.h:179: 
> > On 2016/02/11 20:25:51, pthatcher1 wrote:
> > > On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> > > > Why can't IsEquivalent be used?
> > > 
> > > Because then we have to signal everything.  It would be nice to just have
to
> > > signal what's needed (the 3 tuple).
> > 
> > Is there a reason why signaling the whole candidate is difficult?
> 
> No, but it seems silly to require signalling things that aren't required.  If
> the application wants to optimize the signalling messages to be as small as
> possible, it should be free to do so, and if require unnecessary information,
> then we're just overconstraining what the application needs to signal.

If an application wanted to do that optimization, it could assign IDs to
candidates and just signal the IDs of the remote candidates. I'm just worried
that eventually we'll realize "oh, we did need that attribute after all". And it
makes the spec more complicated, since we'll have to add a special candidate
type with fewer fields, or make the existing fields nullable.

[honghaiz3, 2016-02-12 00:56:55]

I addressed the comments other than the one about a candidate-remove signaling.
Will discuss more on that.

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsep.h
File talk/app/webrtc/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsep.h#n...
talk/app/webrtc/jsep.h:92: virtual int remove(const IceCandidateInterface*
candidate) = 0;
On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> Does this method need to be on IceCandidateCollection, or just
> JsepIceCandidateCollection?
OK I keep it only in the JsepIceCandidateCollection

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsepicec...
File talk/app/webrtc/jsepicecandidate.cc (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/jsepicec...
talk/app/webrtc/jsepicecandidate.cc:106: return candidates_.size() -
original_size;
On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> This will return a negative number, is that right?
Thanks for the good catch.

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1717: const cricket::ContentInfo*
WebRtcSession::GetRemoteMediaContent(
On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> As long as you're making this a standalone method, could you make it work if
the
> candidate only has "sdp_mid" set, and not "sdp_mline_index"? This is allowed
by
> the spec, technically.

How do you know if sdp_mline_index is set or not?

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:179: 
On 2016/02/11 20:55:17, Taylor Brandstetter wrote:
> On 2016/02/11 20:25:51, pthatcher1 wrote:
> > On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> > > Why can't IsEquivalent be used?
> > 
> > Because then we have to signal everything.  It would be nice to just have to
> > signal what's needed (the 3 tuple).
> 
> Is there a reason why signaling the whole candidate is difficult?
Signaling everything will consume unnecessary bandwidth. 
In theory, another option is to fill up the missing information like here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
and then apply IsEquivalent. 
but I think adding a new method would require less computation. 
Do you think there is any case that protocco, address and component is the same
but other fields may be different so that we should treat the two candidates
differently?

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1648813004/diff/120001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:219: void
OnCandidatesRemoved(TransportChannelImpl* channel, const Candidates& cc);
On 2016/02/11 20:25:51, pthatcher1 wrote:
> On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> > Just curious, why name the variable cc?
> 
> Agreed

Changed to candidates. 
What I meant was
one c means one candidate. 
two c mean more than one candidate

[Taylor Brandstetter, 2016-02-12 02:30:38]

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1717: const cricket::ContentInfo*
WebRtcSession::GetRemoteMediaContent(
On 2016/02/12 00:56:55, honghaiz3 wrote:
> On 2016/02/10 21:58:08, Taylor Brandstetter wrote:
> > As long as you're making this a standalone method, could you make it work if
> the
> > candidate only has "sdp_mid" set, and not "sdp_mline_index"? This is allowed
> by
> > the spec, technically.
> 
> How do you know if sdp_mline_index is set or not?

Extremely unfortunately, the default value appears to be 0, which is
indistinguishable from a valid value:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'll make a note to myself to fix this. But for now, the algorithm here could be
"If sdp_mid is nonempty, use it. Otherwise, use sdp_mline_index."

[honghaiz3, 2016-02-27 01:16:30]

Patchset #4 (id:160001) has been deleted

[honghaiz3, 2016-02-27 01:17:11]

Patchset #5 (id:200001) has been deleted

[honghaiz3, 2016-02-27 01:17:32]

Patchset #4 (id:180001) has been deleted

[honghaiz3, 2016-02-27 01:17:46]

Patchset #4 (id:220001) has been deleted

[honghaiz3, 2016-02-27 01:17:57]

Patchset #4 (id:240001) has been deleted

[honghaiz3, 2016-02-27 01:56:01]

Revised to use a separate message between two peers to remove a group of
candidates. PTAL.

[honghaiz3, 2016-02-27 01:56:11]

Revised to use a separate message between two peers to remove a group of
candidates. PTAL.

[honghaiz3, 2016-02-29 18:32:22]

Patchset #3 (id:140001) has been deleted

[pthatcher1, 2016-03-01 23:51:54]

Looking good.  I'm mostly just worried about using ref counting.  I'd greatly
prefer to avoid that if we can.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h
File webrtc/api/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h#newcod...
webrtc/api/jsep.h:104: const std::vector<IceCandidateInterfaceRefPtr>&
candidates) = 0;
Why do we have to use a scoped_refptr here?  Why can't we just use a const
std::vector<IceCandidateInterface>&?  It seems like all the implementations of
this just read the values, so I don't see why we need to do refcounting.

And even if we worried about ownership, we could just copy the candidates and
pass ownership of the copy, since they are cheap to copy.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:489: const
std::vector<IceCandidateInterfaceRefPtr>& candidates){};
Same here.  I don't understand why ref counting is needed.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1746: bool WebRtcSession::CeaseRemoteCandidates(
Why is this a separate method from RemoveRemoteCandidates?  It seems weird to
have both Cease and Remove.  Looking at this method, it seems it should be
called RemoveRemoteCandidates, but we've already got one of those.

If you *have* to keep this separate, perhaps call it
RemoteRemoteCandidatesFromTransportController.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/examples/androida...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/examples/androida...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:464:
ContinualGatheringPolicy.GATHER_CONTINUALLY;
Should we put this in a different CL?

[honghaiz3, 2016-03-02 19:06:40]

PTAL.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h
File webrtc/api/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h#newcod...
webrtc/api/jsep.h:104: const std::vector<IceCandidateInterfaceRefPtr>&
candidates) = 0;
On 2016/03/01 23:51:53, pthatcher1 wrote:
> Why do we have to use a scoped_refptr here?  Why can't we just use a const
> std::vector<IceCandidateInterface>&?  It seems like all the implementations of
> this just read the values, so I don't see why we need to do refcounting.
> 
> And even if we worried about ownership, we could just copy the candidates and
> pass ownership of the copy, since they are cheap to copy.

We cannot do vector<IceCandidateInterface>. If so, we have to provide vector of
IceCandidateInterface, not vector of its subclasses. 
By the way, peerconnection_interface does use vector of Ref-counted pointers a
few times.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:489: const
std::vector<IceCandidateInterfaceRefPtr>& candidates){};
On 2016/03/01 23:51:53, pthatcher1 wrote:
> Same here.  I don't understand why ref counting is needed.

Since we need it in one place, I think we should just use it everywhere along
the message passing.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1746: bool WebRtcSession::CeaseRemoteCandidates(
On 2016/03/01 23:51:54, pthatcher1 wrote:
> Why is this a separate method from RemoveRemoteCandidates?  It seems weird to
> have both Cease and Remove.  Looking at this method, it seems it should be
> called RemoveRemoteCandidates, but we've already got one of those.
> 
> If you *have* to keep this separate, perhaps call it
> RemoteRemoteCandidatesFromTransportController.
I tried to make it symmetric to the process of adding candidates. 
CeaseRemoteCandidates is opposite to UseCandidates. 
UseCandidate means the underlying stack (TransportController,
p2ptransportchannel) starts to use the candidates to create connections. 
CeaseRemoteCandidate means that the transport channels/controllers stop using
them, while RemoveRemoteCandidates including both removing them from the
description and stop using them.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/examples/androida...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/examples/androida...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:464:
ContinualGatheringPolicy.GATHER_CONTINUALLY;
On 2016/03/01 23:51:54, pthatcher1 wrote:
> Should we put this in a different CL?

Done.

[pthatcher1, 2016-03-02 20:37:58]

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h
File webrtc/api/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h#newcod...
webrtc/api/jsep.h:104: const std::vector<IceCandidateInterfaceRefPtr>&
candidates) = 0;
On 2016/03/02 19:06:40, honghaiz3 wrote:
> On 2016/03/01 23:51:53, pthatcher1 wrote:
> > Why do we have to use a scoped_refptr here?  Why can't we just use a const
> > std::vector<IceCandidateInterface>&?  It seems like all the implementations
of
> > this just read the values, so I don't see why we need to do refcounting.
> > 
> > And even if we worried about ownership, we could just copy the candidates
and
> > pass ownership of the copy, since they are cheap to copy.
> 
> We cannot do vector<IceCandidateInterface>. If so, we have to provide vector
of
> IceCandidateInterface, not vector of its subclasses. 
> By the way, peerconnection_interface does use vector of Ref-counted pointers a
> few times. 

If this is just because we want an IceCandidateInteface so that we can have a
MockIceCandidate, then I think we should look into removing
IceCandidateInterface and just make it IceCandidate and handle mocking some
other way.  Introducing ref counting into a simple struct that's passed around
seems like too much complexity for too little gain (the MockIceCandidate).

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1746: bool WebRtcSession::CeaseRemoteCandidates(
On 2016/03/02 19:06:40, honghaiz3 wrote:
> On 2016/03/01 23:51:54, pthatcher1 wrote:
> > Why is this a separate method from RemoveRemoteCandidates?  It seems weird
to
> > have both Cease and Remove.  Looking at this method, it seems it should be
> > called RemoveRemoteCandidates, but we've already got one of those.
> > 
> > If you *have* to keep this separate, perhaps call it
> > RemoteRemoteCandidatesFromTransportController.
> I tried to make it symmetric to the process of adding candidates. 
> CeaseRemoteCandidates is opposite to UseCandidates. 
> UseCandidate means the underlying stack (TransportController,
> p2ptransportchannel) starts to use the candidates to create connections. 
> CeaseRemoteCandidate means that the transport channels/controllers stop using
> them, while RemoveRemoteCandidates including both removing them from the
> description and stop using them. 

Then please call it RemoveRemoteCandidatesFromTransportController

[honghaiz3, 2016-03-03 01:17:40]

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h
File webrtc/api/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/jsep.h#newcod...
webrtc/api/jsep.h:104: const std::vector<IceCandidateInterfaceRefPtr>&
candidates) = 0;
On 2016/03/02 20:37:58, pthatcher1 wrote:
> On 2016/03/02 19:06:40, honghaiz3 wrote:
> > On 2016/03/01 23:51:53, pthatcher1 wrote:
> > > Why do we have to use a scoped_refptr here?  Why can't we just use a const
> > > std::vector<IceCandidateInterface>&?  It seems like all the
implementations
> of
> > > this just read the values, so I don't see why we need to do refcounting.
> > > 
> > > And even if we worried about ownership, we could just copy the candidates
> and
> > > pass ownership of the copy, since they are cheap to copy.
> > 
> > We cannot do vector<IceCandidateInterface>. If so, we have to provide vector
> of
> > IceCandidateInterface, not vector of its subclasses. 
> > By the way, peerconnection_interface does use vector of Ref-counted pointers
a
> > few times. 
> 
> If this is just because we want an IceCandidateInteface so that we can have a
> MockIceCandidate, then I think we should look into removing
> IceCandidateInterface and just make it IceCandidate and handle mocking some
> other way.  Introducing ref counting into a simple struct that's passed around
> seems like too much complexity for too little gain (the MockIceCandidate).
It is going to be a lot more complicated to try to remove IceCandidateInterface
because it is used by Chrome and by chromoting. All the RefCount object adds is
just a counter. We could create a vector of IceCandidate objects and create
another vector of pointers pointing to the objects in the object vector,
although that would appear not as safe as the ref-counted pointers.

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1648813004/diff/300001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1746: bool WebRtcSession::CeaseRemoteCandidates(
On 2016/03/02 20:37:58, pthatcher1 wrote:
> On 2016/03/02 19:06:40, honghaiz3 wrote:
> > On 2016/03/01 23:51:54, pthatcher1 wrote:
> > > Why is this a separate method from RemoveRemoteCandidates?  It seems weird
> to
> > > have both Cease and Remove.  Looking at this method, it seems it should be
> > > called RemoveRemoteCandidates, but we've already got one of those.
> > > 
> > > If you *have* to keep this separate, perhaps call it
> > > RemoteRemoteCandidatesFromTransportController.
> > I tried to make it symmetric to the process of adding candidates. 
> > CeaseRemoteCandidates is opposite to UseCandidates. 
> > UseCandidate means the underlying stack (TransportController,
> > p2ptransportchannel) starts to use the candidates to create connections. 
> > CeaseRemoteCandidate means that the transport channels/controllers stop
using
> > them, while RemoveRemoteCandidates including both removing them from the
> > description and stop using them. 
> 
> Then please call it RemoveRemoteCandidatesFromTransportController

Done.

[honghaiz3, 2016-03-04 23:49:18]

Patchset #9 (id:380001) has been deleted

[honghaiz3, 2016-03-05 01:11:33]

Patchset #9 (id:400001) has been deleted

[honghaiz3, 2016-03-05 01:13:45]

Now I removed the refcount and refPtr and added transport_name to candidates
before it is signaled for removing. PTAL.
Tested end-to-end.

[pthatcher1, 2016-03-05 02:14:32]

This is looking good.  Thanks for doing it. 

Now, do we need some more unit tests?

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/jsep.h
File webrtc/api/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/jsep.h#newcode25
webrtc/api/jsep.h:25: typedef std::vector<Candidate> Candidates;
Can you just use std::vector<Candidate> everywhere instead of Candidates?  The
style guide frowns on this sort of thing (even though we used to do it along
time ago, with C++11 for-loops helping out, we've moved away from it)

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/webrtcsdp.h
File webrtc/api/webrtcsdp.h (right):

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/webrtcsdp.h#n...
webrtc/api/webrtcsdp.h:75: // transport_name - The transport name of the
candidate.
transport name (MID)

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:175: }
We should probably put a comment explaining what this is: the transport->name()
of the P2pTransport of this candidate.

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:260: std::string transport_name_;
Can you make this the first thing (before id_) and add it to the
ToStringInternal?

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:263: typedef std::vector<Candidate> Candidates;
Same here as before.

[honghaiz3, 2016-03-09 17:38:51]

Patchset #11 (id:460001) has been deleted

[honghaiz3, 2016-03-09 17:40:07]

Added tests for webrtcsession and p2ptransportchannel. 
Other changes are mostly passing messages around.

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/jsep.h
File webrtc/api/jsep.h (right):

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/jsep.h#newcode25
webrtc/api/jsep.h:25: typedef std::vector<Candidate> Candidates;
On 2016/03/05 02:14:32, pthatcher1 wrote:
> Can you just use std::vector<Candidate> everywhere instead of Candidates?  The
> style guide frowns on this sort of thing (even though we used to do it along
> time ago, with C++11 for-loops helping out, we've moved away from it)

Done.

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/webrtcsdp.h
File webrtc/api/webrtcsdp.h (right):

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/api/webrtcsdp.h#n...
webrtc/api/webrtcsdp.h:75: // transport_name - The transport name of the
candidate.
On 2016/03/05 02:14:32, pthatcher1 wrote:
> transport name (MID)

Done.

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:175: }
On 2016/03/05 02:14:32, pthatcher1 wrote:
> We should probably put a comment explaining what this is: the
transport->name()
> of the P2pTransport of this candidate.

Done.

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:260: std::string transport_name_;
On 2016/03/05 02:14:32, pthatcher1 wrote:
> Can you make this the first thing (before id_) and add it to the
> ToStringInternal?

Done. The first thing was foundation.

https://codereview.webrtc.org/1648813004/diff/440001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:263: typedef std::vector<Candidate> Candidates;
On 2016/03/05 02:14:32, pthatcher1 wrote:
> Same here as before.

Done.

[pthatcher1, 2016-03-09 18:00:53]

lgtm

Just some nits in the unit test

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

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:101: enum { MSG_CANDIDATE,
MSG_CANDIDATE_REMOVALS };
Can you call these MSG_ADD_CANDIDATES and MSG_REMOVE_CANDIDATES?

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:313: this,
&P2PTransportChannelTestBase::OnCandidate);
Can you rename this OnCandidateGathered?

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:676: cricket::Candidate&
candidate = data->candidates[0];
Why not just make this a for loop?

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:706: cricket::Candidate c =
data->candidates[0];
Why not make this a for loop?

[honghaiz3, 2016-03-10 02:17:48]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-03-10 02:17:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1648813004/#ps540001 (title:
"Delete ice_candidate when it is removed and add a test")

[commit-bot: I haz the power, 2016-03-10 02:17:54]

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

[commit-bot: I haz the power, 2016-03-10 02:30:23]

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

[commit-bot: I haz the power, 2016-03-10 02:30:24]

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

[honghaiz3, 2016-03-10 02:34:14]

honghaiz@webrtc.org changed reviewers:
+ glaznev@webrtc.org

[honghaiz3, 2016-03-10 02:34:16]

Alex, 

Can you take a look at this CL?
The webrtc/examples dir is owned by you (and Zeke).

Thanks

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

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:101: enum { MSG_CANDIDATE,
MSG_CANDIDATE_REMOVALS };
On 2016/03/09 18:00:53, pthatcher1 wrote:
> Can you call these MSG_ADD_CANDIDATES and MSG_REMOVE_CANDIDATES?

Done.

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:313: this,
&P2PTransportChannelTestBase::OnCandidate);
On 2016/03/09 18:00:53, pthatcher1 wrote:
> Can you rename this OnCandidateGathered?

Done.

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:676: cricket::Candidate&
candidate = data->candidates[0];
On 2016/03/09 18:00:53, pthatcher1 wrote:
> Why not just make this a for loop?

Done.

https://codereview.webrtc.org/1648813004/diff/480001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:706: cricket::Candidate c =
data->candidates[0];
On 2016/03/09 18:00:53, pthatcher1 wrote:
> Why not make this a for loop?

Done.

[AlexG, 2016-03-11 19:18:52]

lgtm

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/api/java/jni/peer...
File webrtc/api/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:195: jmethodID ctor =
GetMethodID(jni(), candidate_class,
This may re use code from ToJavaCandidate.
Add helper function?

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/examples/androida...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/examples/androida...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:672:
if (queuedRemoteCandidates != null) {
Just call drainCandidates() - check if (queuedRemoteCandidates != null) will be
done inside drainCandidates()

[honghaiz3, 2016-03-11 19:46:47]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-03-11 19:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1648813004/#ps560001 (title:
"Address Alex's comments")

[commit-bot: I haz the power, 2016-03-11 19:46:55]

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

[honghaiz3, 2016-03-11 19:47:14]

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/api/java/jni/peer...
File webrtc/api/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:195: jmethodID ctor =
GetMethodID(jni(), candidate_class,
On 2016/03/11 19:18:52, AlexG wrote:
> This may re use code from ToJavaCandidate.
> Add helper function?
There is some complication because OnIceCandidate uses IceCandidateInterface
while OnIceCandidatesRemoved uses cricket::Candidate. 
The transport name is extracted from different field and the mline_index is not
used in the latter. We did so because we eventually want to get rid of
IceCandidateInterface and use cricket::Candidate (with extra field).

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/examples/androida...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1648813004/diff/540001/webrtc/examples/androida...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:672:
if (queuedRemoteCandidates != null) {
On 2016/03/11 19:18:52, AlexG wrote:
> Just call drainCandidates() - check if (queuedRemoteCandidates != null) will
be
> done inside drainCandidates() 

Done. Thanks!

[commit-bot: I haz the power, 2016-03-11 21:28:12]

Description was changed from

==========
When doing candidate re-gathering in the same generation, Remove the existing
local candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

to

==========
When doing candidate re-gathering in the same generation, Remove the existing
local candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

[commit-bot: I haz the power, 2016-03-11 21:28:14]

Committed patchset #15 (id:560001)

[commit-bot: I haz the power, 2016-03-11 21:28:21]

Description was changed from

==========
When doing candidate re-gathering in the same generation, Remove the existing
local candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=
==========

to

==========
When doing candidate re-gathering in the same generation, Remove the existing
local candidate on the same network
and signaling the remote side to remove its remote candidate by setting the
candidate priority to 0.

BUG=

Committed: https://crrev.com/84430da6817ce69c53bfad088be5c9df8b420f01
Cr-Commit-Position: refs/heads/master@{#11958}
==========

[commit-bot: I haz the power, 2016-03-11 21:28:22]

Patchset 15 (id:??) landed as
https://crrev.com/84430da6817ce69c53bfad088be5c9df8b420f01
Cr-Commit-Position: refs/heads/master@{#11958}

[tommi, 2016-03-11 22:04:55]

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

The reason for reverting is: Breaks the build.  Suggest we reland with a default
implementation of the new method, update Chrome, land a change that changes |{}|
-> |= 0;|

Here's the error:

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/content/renderer/media/webrtc/test_support_content.mock_peer_connection_dependency_factory.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=262839-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1
-DDCHECK_ALWAYS_ON=1 -DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DMOJO_USE_SYSTEM_IMPL -DGTEST_HAS_POSIX_RE=0
-DGTEST_LANG_CXX11=0 -DSK_SUPPORT_GPU=1 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS
-DUNIT_TEST -DGTEST_HAS_RTTI=0 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0
-DU_STATIC_IMPLEMENTATION -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DCHROME_PNG_WRITE_SUPPORT
-DPNG_USER_CONFIG -DFEATURE_ENABLE_SSL -DFEATURE_ENABLE_VOICEMAIL
-DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH -DNO_MAIN_THREAD_WRAPPING
-DNO_SOUND_SYSTEM -DOSX -DWEBRTC_MAC -DWEBRTC_POSIX -DXML_STATIC
-DWEBRTC_CHROMIUM_BUILD -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-D_FORTIFY_SOURCE=2 -Igen -I../.. -I../../third_party/khronos -I../../gpu
-I../../skia/config -Igen/angle -I../../third_party/WebKit/Source
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils
-I../../third_party/skia/include/utils/mac -I../../skia/ext
-I../../testing/gmock/include -I../../testing/gtest/include
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-Igen/ui/resources -Igen/protoc_out -I../../third_party/protobuf
-I../../third_party/protobuf/src -I../../third_party/WebKit -I../../ipc
-I../../third_party/opus/src/include -I../../third_party/WebKit
-I../../third_party/npapi -I../../third_party/npapi/bindings
-I../../third_party/libpng -I../../third_party/zlib -I../../third_party/libwebp
-I../../third_party/ots/include -I../../third_party/qcms/src
-I../../third_party/iccjpeg -I../../third_party/libjpeg_turbo -I../../v8/include
-I../../third_party/webrtc_overrides -I../../third_party/libjingle/overrides
-I../../third_party/libjingle/source -I../../third_party
-I../../third_party/expat/files/lib -I../../third_party/libvpx/source/libvpx
-isysroot
/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-O2 -gdwarf-2 -fvisibility=hidden -Werror -mmacosx-version-min=10.6 -arch x86_64
-Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wno-shift-negative-value -std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
/b/build/slave/Mac_Builder/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -Xclang
-plugin-arg-find-bad-constructs -Xclang follow-macro-expansion
-fcolor-diagnostics -fno-strict-aliasing  -c
../../content/renderer/media/webrtc/mock_peer_connection_dependency_factory.cc
-o
obj/content/renderer/media/webrtc/test_support_content.mock_peer_connection_dependency_factory.o
../../content/renderer/media/webrtc/mock_peer_connection_dependency_factory.cc:404:14:
error: allocating an object of abstract class type
'content::MockSessionDescription'
  return new MockSessionDescription(type, sdp);
             ^
../../third_party/webrtc/api/jsep.h:100:18: note: unimplemented pure virtual
method 'RemoveCandidates' in 'MockSessionDescription'
  virtual size_t RemoveCandidates(
                 ^
1 error generated.
ninja: build stopped: subcommand failed..