FileRecorder + FilePlayer: Let Create functions return unique_ptr

FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)

Committed: https://crrev.com/5a25d9504acdaab540e1a7f1f26cf3bab34a7d5e
Cr-Commit-Position: refs/heads/master@{#13797}

[kwiberg-webrtc, 2016-06-09 08:19:36]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-08-17 09:38:18]

Description was changed from

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors.
==========

to

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)

NOPRESUBMIT=true
==========

[kwiberg-webrtc, 2016-08-17 09:39:07]

Description was changed from

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)

NOPRESUBMIT=true
==========

to

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)
==========

[kwiberg-webrtc, 2016-08-17 09:39:24]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2016-08-17 09:57:02]

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[ossu, 2016-08-17 11:09:06]

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/file_player.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/file_player.h:32: static
std::unique_ptr<FilePlayer> NewFilePlayer(
I've a slight issue with the naming here, though I understand its purpose.
Having looked through our existing code base, I find almost no constructor
function whose name starts with New - we consistently use Create* for that.
There are only a few exceptions and I'm not sure we want to add to them.

Now, we can't have CreateFilePlayer, since there's no way to distinguish the two
versions, and we need to keep the old version not to break other users.

Maybe just call it FilePlayer::Create - it should be sufficiently clear what
we're creating from the naming, however nothing else seems to have a plain
Create() function either. Perhaps FilePlayer::CreatePlayer?

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/file_recorder.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/file_recorder.h:27: static
std::unique_ptr<FileRecorder> CreateFileRecorder(
Do we not need to retain compatibility for CreateFileRecorder as well? I can't
find any external uses of it. But if the contruction function for FilePlayer is
renamed, the one for FileRecorder should probably match it.

Or are you aiming for just jumping through hoops right now, so that external
users can update their code, and in a while remove the old CreateFilePlayer and
rename NewFilePlayer?

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/voice_engine/outpu...
File webrtc/voice_engine/output_mixer.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/voice_engine/outpu...
webrtc/voice_engine/output_mixer.h:124: std::unique_ptr<FileRecorder>
output_file_recorder_;
Should this have some sort of annotation if it's being protected by a critical
section? Probably not as part of this CL, though.

[kwiberg-webrtc, 2016-08-17 12:27:39]

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/file_player.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/file_player.h:32: static
std::unique_ptr<FilePlayer> NewFilePlayer(
On 2016/08/17 11:09:05, ossu wrote:
> I've a slight issue with the naming here, though I understand its purpose.
> Having looked through our existing code base, I find almost no constructor
> function whose name starts with New - we consistently use Create* for that.
> There are only a few exceptions and I'm not sure we want to add to them.
> 
> Now, we can't have CreateFilePlayer, since there's no way to distinguish the
two
> versions, and we need to keep the old version not to break other users.
> 
> Maybe just call it FilePlayer::Create - it should be sufficiently clear what
> we're creating from the naming, however nothing else seems to have a plain
> Create() function either. Perhaps FilePlayer::CreatePlayer?

FilePlayer::CreatePlayer is probably just as rare in our tree as the other two
options---just harder to search for. This is actually a good thing, since it
means we use the same convention everywhere, but it's inconvenient in this case.

Since this name is going to be temporary, as discussed later, I'd rather not
change it to something that's only marginally better.

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/file_recorder.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/file_recorder.h:27: static
std::unique_ptr<FileRecorder> CreateFileRecorder(
On 2016/08/17 11:09:05, ossu wrote:
> Do we not need to retain compatibility for CreateFileRecorder as well? I can't
> find any external uses of it. But if the contruction function for FilePlayer
is
> renamed, the one for FileRecorder should probably match it.

No, FileRecorder has no external users as far as I know (and I did send out a
PSA about it earlier, so there shouldn't be).

> Or are you aiming for just jumping through hoops right now, so that external
> users can update their code, and in a while remove the old CreateFilePlayer
and
> rename NewFilePlayer?

Yes, if create functions are called CreateFoo all through our code base, then
that's what I should do.

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/voice_engine/outpu...
File webrtc/voice_engine/output_mixer.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/voice_engine/outpu...
webrtc/voice_engine/output_mixer.h:124: std::unique_ptr<FileRecorder>
output_file_recorder_;
On 2016/08/17 11:09:06, ossu wrote:
> Should this have some sort of annotation if it's being protected by a critical
> section? Probably not as part of this CL, though.

Yes.

[ossu, 2016-08-17 13:49:14]

lgtm

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/file_player.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/file_player.h:32: static
std::unique_ptr<FilePlayer> NewFilePlayer(
On 2016/08/17 12:27:39, kwiberg-webrtc wrote:
> On 2016/08/17 11:09:05, ossu wrote:
> > I've a slight issue with the naming here, though I understand its purpose.
> > Having looked through our existing code base, I find almost no constructor
> > function whose name starts with New - we consistently use Create* for that.
> > There are only a few exceptions and I'm not sure we want to add to them.
> > 
> > Now, we can't have CreateFilePlayer, since there's no way to distinguish the
> two
> > versions, and we need to keep the old version not to break other users.
> > 
> > Maybe just call it FilePlayer::Create - it should be sufficiently clear what
> > we're creating from the naming, however nothing else seems to have a plain
> > Create() function either. Perhaps FilePlayer::CreatePlayer?
> 
> FilePlayer::CreatePlayer is probably just as rare in our tree as the other two
> options---just harder to search for. This is actually a good thing, since it
> means we use the same convention everywhere, but it's inconvenient in this
case.
> 
> Since this name is going to be temporary, as discussed later, I'd rather not
> change it to something that's only marginally better.

Alright, I'm fine with that! Just didn't want us to have NewFilePlayer as the
permanent name in future.

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/file_recorder.h (right):

https://codereview.webrtc.org/2049683003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/file_recorder.h:27: static
std::unique_ptr<FileRecorder> CreateFileRecorder(
On 2016/08/17 12:27:39, kwiberg-webrtc wrote:
> On 2016/08/17 11:09:05, ossu wrote:
> > Do we not need to retain compatibility for CreateFileRecorder as well? I
can't
> > find any external uses of it. But if the contruction function for FilePlayer
> is
> > renamed, the one for FileRecorder should probably match it.
> 
> No, FileRecorder has no external users as far as I know (and I did send out a
> PSA about it earlier, so there shouldn't be).
> 
> > Or are you aiming for just jumping through hoops right now, so that external
> > users can update their code, and in a while remove the old CreateFilePlayer
> and
> > rename NewFilePlayer?
> 
> Yes, if create functions are called CreateFoo all through our code base, then
> that's what I should do.

Acknowledged.

[kwiberg-webrtc, 2016-08-17 13:50:12]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-08-17 13:50:25]

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, 2016-08-17 14:31:17]

Description was changed from

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)
==========

to

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)
==========

[commit-bot: I haz the power, 2016-08-17 14:31:18]

Committed patchset #1 (id:40001)

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

Description was changed from

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)
==========

to

==========
FileRecorder + FilePlayer: Let Create functions return unique_ptr

Because passing ownership in raw pointers makes kittens cry.

This also means we can ditch the Destroy functions and the protected
destructors. (Well, almost. We need to keep the old CreateFilePlayer
and DestroyFilePlayer around for a little while longer because of an
external caller.)

Committed: https://crrev.com/5a25d9504acdaab540e1a7f1f26cf3bab34a7d5e
Cr-Commit-Position: refs/heads/master@{#13797}
==========

[commit-bot: I haz the power, 2016-08-17 14:31:24]

Patchset 1 (id:??) landed as
https://crrev.com/5a25d9504acdaab540e1a7f1f26cf3bab34a7d5e
Cr-Commit-Position: refs/heads/master@{#13797}