Switching some interfaces to use std::unique_ptr<>.

Switching some interfaces to use std::unique_ptr<>.

This helps show where ownership is transfered between objects.

Specifically, this CL wraps cricket::VideoCapturer, MediaEngineInterface
and DataEngineInterface in unique_ptr.

BUG=None
TBR=magjed@webrtc.org

Review-Url: https://codereview.webrtc.org/2685093002
Cr-Commit-Position: refs/heads/master@{#16548}
Committed: https://chromium.googlesource.com/external/webrtc/+/112b2e99d806a8f60ad3dcb5abc9dd82b704df65

[pthatcher1, 2017-02-10 18:21:25]

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

[pthatcher1, 2017-02-10 18:21:25]

lgtm, but it would be nice to have a second reviewer, perhaps someone from STO. 
solenberg?  tommi?

https://codereview.webrtc.org/2685093002/diff/1/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/2685093002/diff/1/webrtc/pc/channelmanager.h#ne...
webrtc/pc/channelmanager.h:42: // ownership of the objects passed in as
unique_ptrs.
Isn't that kind of assumed by a std::unique_ptr?  You can probably delete the
whole last sentence there.

[Taylor Brandstetter, 2017-02-10 21:55:47]

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

[Taylor Brandstetter, 2017-02-10 21:55:48]

nisse@, since you've been working on similar cleanup efforts, want to take a
look?

https://codereview.webrtc.org/2685093002/diff/1/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/2685093002/diff/1/webrtc/pc/channelmanager.h#ne...
webrtc/pc/channelmanager.h:42: // ownership of the objects passed in as
unique_ptrs.
On 2017/02/10 18:21:25, pthatcher1 wrote:
> Isn't that kind of assumed by a std::unique_ptr?  You can probably delete the
> whole last sentence there.

Removed.

[the sun, 2017-02-10 22:00:10]

solenberg@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[the sun, 2017-02-10 22:00:11]

lgtm

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

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:970: // TODO(deadbeef): Make pure virtual
once implemented by chromium subclasses.
Do you mean MockPeerConnectionImpl?

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:980: const MediaConstraintsInterface*
constraints) {
Since we're clarifying ownership in this CL, what is the contract regarding the
constraints? Is it just borrowed in this call or is it expected to outlive the
PC object?

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/pc/channelmanager_...
File webrtc/pc/channelmanager_unittest.cc (right):

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/pc/channelmanager_...
webrtc/pc/channelmanager_unittest.cc:46:
std::unique_ptr<MediaEngineInterface>(fme_),
did git cl format do this to us? :/

[Taylor Brandstetter, 2017-02-11 01:50:08]

deadbeef@webrtc.org changed reviewers:
- nisse@webrtc.org

[Taylor Brandstetter, 2017-02-11 01:50:09]

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

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:970: // TODO(deadbeef): Make pure virtual
once implemented by chromium subclasses.
On 2017/02/10 22:00:10, the sun wrote:
> Do you mean MockPeerConnectionImpl?

Actually... It looks like there aren't any chromium subclasses of
PeerConnectionFactoryInterface. So I'll change this.

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:980: const MediaConstraintsInterface*
constraints) {
On 2017/02/10 22:00:10, the sun wrote:
> Since we're clarifying ownership in this CL, what is the contract regarding
the
> constraints? Is it just borrowed in this call or is it expected to outlive the
> PC object?

It's only used within this method, so it's not required to outlive the PC. I'll
add a comment.

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/pc/channelmanager_...
File webrtc/pc/channelmanager_unittest.cc (right):

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/pc/channelmanager_...
webrtc/pc/channelmanager_unittest.cc:46:
std::unique_ptr<MediaEngineInterface>(fme_),
On 2017/02/10 22:00:10, the sun wrote:
> did git cl format do this to us? :/

Yeah, it's four spaces of indentation; just happens to line up with the paren

[Taylor Brandstetter, 2017-02-11 01:51:00]

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

[Taylor Brandstetter, 2017-02-11 01:51:01]

+magjed for the Obj-C file

[Taylor Brandstetter, 2017-02-11 02:13:22]

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

https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:970: // TODO(deadbeef): Make pure virtual
once implemented by chromium subclasses.
On 2017/02/11 01:50:08, Taylor Brandstetter wrote:
> On 2017/02/10 22:00:10, the sun wrote:
> > Do you mean MockPeerConnectionImpl?
> 
> Actually... It looks like there aren't any chromium subclasses of
> PeerConnectionFactoryInterface. So I'll change this.

Oh, but we DO have mock PC factory implementations internally.

[Taylor Brandstetter, 2017-02-11 02:17:02]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 02:17:06]

Dry run: 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-02-11 02:31:36]

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

[commit-bot: I haz the power, 2017-02-11 02:31:37]

Dry run: Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/21033)

[Taylor Brandstetter, 2017-02-11 03:05:46]

Description was changed from

==========
Switching some interfaces to use std::unique_ptr<>.

This helps show where ownership is transfered between objects.

Specifically, this CL wraps cricket::VideoCapturer, MediaEngineInterface
and DataEngineInterface in unique_ptr.

BUG=None
==========

to

==========
Switching some interfaces to use std::unique_ptr<>.

This helps show where ownership is transfered between objects.

Specifically, this CL wraps cricket::VideoCapturer, MediaEngineInterface
and DataEngineInterface in unique_ptr.

BUG=None
TBR=magjed@webrtc.org
==========

[Taylor Brandstetter, 2017-02-11 03:05:52]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-11 03:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2685093002/#ps100001 (title:
"Rebase onto master")

[commit-bot: I haz the power, 2017-02-11 03:05:55]

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-02-11 04:13:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486782352237120,
"parent_rev": "2c87d9991bca942113ec1b774895ea220ec2c3f4", "commit_rev":
"112b2e99d806a8f60ad3dcb5abc9dd82b704df65"}

[commit-bot: I haz the power, 2017-02-11 04:13:44]

Description was changed from

==========
Switching some interfaces to use std::unique_ptr<>.

This helps show where ownership is transfered between objects.

Specifically, this CL wraps cricket::VideoCapturer, MediaEngineInterface
and DataEngineInterface in unique_ptr.

BUG=None
TBR=magjed@webrtc.org
==========

to

==========
Switching some interfaces to use std::unique_ptr<>.

This helps show where ownership is transfered between objects.

Specifically, this CL wraps cricket::VideoCapturer, MediaEngineInterface
and DataEngineInterface in unique_ptr.

BUG=None
TBR=magjed@webrtc.org

Review-Url: https://codereview.webrtc.org/2685093002
Cr-Commit-Position: refs/heads/master@{#16548}
Committed:
https://chromium.googlesource.com/external/webrtc/+/112b2e99d806a8f60ad3dcb5a...
==========

[commit-bot: I haz the power, 2017-02-11 04:13:46]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/112b2e99d806a8f60ad3dcb5a...

[nisse-webrtc, 2017-02-13 08:25:09]

On 2017/02/11 02:13:22, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
> File webrtc/api/peerconnectioninterface.h (right):
> 
>
https://codereview.webrtc.org/2685093002/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnectioninterface.h:970: // TODO(deadbeef): Make pure virtual
> once implemented by chromium subclasses.
> On 2017/02/11 01:50:08, Taylor Brandstetter wrote:
> > On 2017/02/10 22:00:10, the sun wrote:
> > > Do you mean MockPeerConnectionImpl?
> > 
> > Actually... It looks like there aren't any chromium subclasses of
> > PeerConnectionFactoryInterface. So I'll change this.
> 
> Oh, but we DO have mock PC factory implementations internally.

See https://codereview.webrtc.org/2632203003/ for some of the things that break.

[nisse-webrtc, 2017-02-13 08:25:57]

nisse@webrtc.org changed reviewers:
+ nisse@webrtc.org

[nisse-webrtc, 2017-02-13 08:25:58]

One scary TODO comment, but generally looks good.

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

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:968: // Creates a
VideoTrackSourceInterface. The new source takes ownership of
You could drop the ownership comment here too, since it's implied by unique_ptr.

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:973:
std::unique_ptr<cricket::VideoCapturer> capturer) {
We should aim to kill cricket::VideoCapturer. As far as I'm aware, it's
now used only by the layers of wrappers around webrtc's own platform specific
capturers. Which should should be refactored into to implementing
VideoTrackSourceInterface directly.

(Not an objection to this change, it's just annoying that we still have this
method).

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectionfactory.cc:212: // TODO:  Need to make sure only one VoE
is created inside
When? Do you intend to ensure that before landing?

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/videocapturert...
File webrtc/pc/videocapturertracksource.h (right):

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/videocapturert...
webrtc/pc/videocapturertracksource.h:37: // VideoCapturerTrackSource takes
ownership of |capturer|.
Another redundant ownership comment.

[the sun, 2017-02-13 09:02:31]

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectionfactory.cc:212: // TODO:  Need to make sure only one VoE
is created inside
On 2017/02/13 08:25:58, nisse-webrtc wrote:
> When? Do you intend to ensure that before landing?

This is an old comment (mid 2013) and nothing I'd be terribly concerned with for
several reasons:

1. I can find two instances in VoE of global state - both instance counters used
exclusively in logging.
2. Much of the code using them is going to be removed within Q1.

[nisse-webrtc, 2017-02-13 09:11:20]

On 2017/02/13 09:02:31, the sun wrote:
>
https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
> File webrtc/pc/peerconnectionfactory.cc (right):
> 
>
https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
> webrtc/pc/peerconnectionfactory.cc:212: // TODO:  Need to make sure only one
VoE
> is created inside
> On 2017/02/13 08:25:58, nisse-webrtc wrote:
> > When? Do you intend to ensure that before landing?
> 
> This is an old comment (mid 2013) and nothing I'd be terribly concerned with
for
> several reasons:
> 
> 1. I can find two instances in VoE of global state - both instance counters
used
> exclusively in logging.
> 2. Much of the code using them is going to be removed within Q1.

lgtm then.

[Taylor Brandstetter, 2017-02-13 17:47:45]

Addressed these things in this follow-up CL (only affected comments):
https://codereview.webrtc.org/2692923002/

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

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:968: // Creates a
VideoTrackSourceInterface. The new source takes ownership of
On 2017/02/13 08:25:58, nisse-webrtc wrote:
> You could drop the ownership comment here too, since it's implied by
unique_ptr.

Done.

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:973:
std::unique_ptr<cricket::VideoCapturer> capturer) {
On 2017/02/13 08:25:58, nisse-webrtc wrote:
> We should aim to kill cricket::VideoCapturer. As far as I'm aware, it's
> now used only by the layers of wrappers around webrtc's own platform specific
> capturers. Which should should be refactored into to implementing
> VideoTrackSourceInterface directly.
> 
> (Not an objection to this change, it's just annoying that we still have this
> method).

Added TODO.

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectionfactory.cc:212: // TODO:  Need to make sure only one VoE
is created inside
On 2017/02/13 09:02:31, the sun wrote:
> On 2017/02/13 08:25:58, nisse-webrtc wrote:
> > When? Do you intend to ensure that before landing?
> 
> This is an old comment (mid 2013) and nothing I'd be terribly concerned with
for
> several reasons:
> 
> 1. I can find two instances in VoE of global state - both instance counters
used
> exclusively in logging.
> 2. Much of the code using them is going to be removed within Q1. 

Removing comment.

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/videocapturert...
File webrtc/pc/videocapturertracksource.h (right):

https://codereview.webrtc.org/2685093002/diff/100001/webrtc/pc/videocapturert...
webrtc/pc/videocapturertracksource.h:37: // VideoCapturerTrackSource takes
ownership of |capturer|.
On 2017/02/13 08:25:58, nisse-webrtc wrote:
> Another redundant ownership comment.

Fixed.