Pass ownership of candidate to PeerConnection::OnIceCandidate

Pass ownership of candidate to PeerConnection::OnIceCandidate

This will later allow calling the "PeerConnectionObserver::OnIceCandidate"
method asynchronously while keeping the object alive.

BUG=webrtc:3721
Review-Url: https://codereview.webrtc.org/2748253003
Cr-Commit-Position: refs/heads/master@{#17380}
Committed: https://chromium.googlesource.com/external/webrtc/+/81bf7b07256970f4410bc0ad0f44ccf0f7144b00

[joachim, 2017-03-14 23:50:13]

Description was changed from

==========
Make IceCandidateInterface ref-counted.

This will later allow calling the "OnIceCandidate" method asynchronously to
the "PeerConnectionObserver" while keeping the object alive. Currently this
method takes a raw pointer..

BUG=webrtc:3721
==========

to

==========
Make IceCandidateInterface ref-counted.

This will later allow calling the "OnIceCandidate" method asynchronously to
the "PeerConnectionObserver" while keeping the object alive. Currently this
method takes a raw pointer.

BUG=webrtc:3721
==========

[joachim, 2017-03-19 22:32:21]

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

[joachim, 2017-03-19 22:32:22]

Ptal, this is in preparation to make signalling through PeerConnectionObserver
asynchronous.

https://codereview.webrtc.org/2748253003/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2748253003/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:804: virtual void OnIceCandidate(
The better solution would be to keep the old version of the method and call it
from the new one by default so existing implementatios wouldn't need to be
changed.
This however requires to add "using PeerConnectionObserver::OnIceCandidate" to
all subclasses (to avoid hiding the new API) - which imho doesn't make it easier
to migrade to the new API.

[Taylor Brandstetter, 2017-03-20 18:48:01]

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

[Taylor Brandstetter, 2017-03-20 18:48:02]

I'm not sure this approach is necessary. In general we'd like to have less
ref-counting. Is there a reason the asynchronous OnIceCandidate callback can't
own the candidate instance?

+pthatcher, in case he has any thoughts to add.

[joachim, 2017-03-20 19:38:57]

On 2017/03/20 18:48:02, Taylor Brandstetter wrote:
> I'm not sure this approach is necessary. In general we'd like to have less
> ref-counting. Is there a reason the asynchronous OnIceCandidate callback can't
> own the candidate instance?
> 
> +pthatcher, in case he has any thoughts to add.

I wanted to match the existing OnXXX methods that take a scoped_refptr, so I
didn't look into passing ownership of the object. How should that be done, I
assume through a std::unique_ptr?

[Taylor Brandstetter, 2017-03-20 20:32:03]

On 2017/03/20 19:38:57, joachim wrote:
> On 2017/03/20 18:48:02, Taylor Brandstetter wrote:
> > I'm not sure this approach is necessary. In general we'd like to have less
> > ref-counting. Is there a reason the asynchronous OnIceCandidate callback
can't
> > own the candidate instance?
> > 
> > +pthatcher, in case he has any thoughts to add.
> 
> I wanted to match the existing OnXXX methods that take a scoped_refptr, so I
> didn't look into passing ownership of the object. How should that be done, I
> assume through a std::unique_ptr?

Yep, with unique_ptr. The OnIceCandidate signature shouldn't even need to
change, which would be difficult to do in a backwards-compatible way as you
pointed out.

[joachim, 2017-03-20 21:19:54]

Description was changed from

==========
Make IceCandidateInterface ref-counted.

This will later allow calling the "OnIceCandidate" method asynchronously to
the "PeerConnectionObserver" while keeping the object alive. Currently this
method takes a raw pointer.

BUG=webrtc:3721
==========

to

==========
Pass ownership of candidate to PeerConnection::OnIceCandidate

This will later allow calling the "PeerConnectionObserver::OnIceCandidate"
method asynchronously to while keeping the object alive.

BUG=webrtc:3721
==========

[joachim, 2017-03-20 21:20:23]

Changed to use std::unique_ptr and updated CL description. Ptal

[joachim, 2017-03-20 21:36:13]

Description was changed from

==========
Pass ownership of candidate to PeerConnection::OnIceCandidate

This will later allow calling the "PeerConnectionObserver::OnIceCandidate"
method asynchronously to while keeping the object alive.

BUG=webrtc:3721
==========

to

==========
Pass ownership of candidate to PeerConnection::OnIceCandidate

This will later allow calling the "PeerConnectionObserver::OnIceCandidate"
method asynchronously while keeping the object alive.

BUG=webrtc:3721
==========

[pthatcher1, 2017-03-21 06:22:27]

I think the idea of changing this to a unique_ptr is fine, but it is rather
silly to need this in the first place because and IceCandidate at this point in
the code shouldn't really be a pointer in the first place.  It should just be a
copyable struct.  The only reason we have it as a pointer is so we can have a
"mock" version.  But I think that's rather silly.

So I think that the right solution is to get rid of all this complexity with
candidates and just replace everything with one struct used everywhere.  Then we
wouldn't need this.

However, I understand if that's too much of a pain at this point and prefer to,
in the short-term, just make this a unique_ptr.

Taylor, what do you think?

[Taylor Brandstetter, 2017-03-23 01:33:07]

lgtm. I agree we should clean up cricket::Candidate, JsepIceCandidate and
IceCandidateInterface, but that can be done separately from the goal of making
PeerConnection callbacks asynchronous.

[pthatcher1, 2017-03-25 03:24:20]

lgtm

[joachim, 2017-03-25 15:11:12]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2017-03-25 15:11:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-25 15:31:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490454672912680,
"parent_rev": "07ce245895f0ece9d080ed72a1c2781d5da54023", "commit_rev":
"81bf7b07256970f4410bc0ad0f44ccf0f7144b00"}

[commit-bot: I haz the power, 2017-03-25 15:31:17]

Description was changed from

==========
Pass ownership of candidate to PeerConnection::OnIceCandidate

This will later allow calling the "PeerConnectionObserver::OnIceCandidate"
method asynchronously while keeping the object alive.

BUG=webrtc:3721
==========

to

==========
Pass ownership of candidate to PeerConnection::OnIceCandidate

This will later allow calling the "PeerConnectionObserver::OnIceCandidate"
method asynchronously while keeping the object alive.

BUG=webrtc:3721

Review-Url: https://codereview.webrtc.org/2748253003
Cr-Commit-Position: refs/heads/master@{#17380}
Committed:
https://chromium.googlesource.com/external/webrtc/+/81bf7b07256970f4410bc0ad0...
==========

[commit-bot: I haz the power, 2017-03-25 15:31:18]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/81bf7b07256970f4410bc0ad0...