FileRecorder + FilePlayer: Let Create functions return unique_ptr

webrtc/modules/utility/include/file_player.h

Index: webrtc/modules/utility/include/file_player.h
diff --git a/webrtc/modules/utility/include/file_player.h b/webrtc/modules/utility/include/file_player.h
index d11261de869733971b291ea9106894ad59dfca1b..5f79fd28e21c5598d5ac835e164c305b65204e6b 100644
--- a/webrtc/modules/utility/include/file_player.h
+++ b/webrtc/modules/utility/include/file_player.h
@@ -11,12 +11,15 @@
 #ifndef WEBRTC_MODULES_UTILITY_INCLUDE_FILE_PLAYER_H_
 #define WEBRTC_MODULES_UTILITY_INCLUDE_FILE_PLAYER_H_
 
+#include <memory>
+
 #include "webrtc/common_types.h"
 #include "webrtc/engine_configurations.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
+
 class FileCallback;
 
 class FilePlayer {
@@ -26,11 +29,17 @@ class FilePlayer {
   enum { MAX_AUDIO_BUFFER_IN_BYTES = MAX_AUDIO_BUFFER_IN_SAMPLES * 2 };
 
   // Note: will return NULL for unsupported formats.
+  static std::unique_ptr<FilePlayer> NewFilePlayer(

[ossu, 2016/08/17 11:09:05]

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?

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

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.

[ossu, 2016/08/17 13:49:13]

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

+      const uint32_t instanceID,
+      const FileFormats fileFormat);
+
+  // Deprecated creation/destruction functions. Use NewFilePlayer instead.
   static FilePlayer* CreateFilePlayer(const uint32_t instanceID,
                                       const FileFormats fileFormat);
-
   static void DestroyFilePlayer(FilePlayer* player);
 
+  virtual ~FilePlayer() = default;
+
   // Read 10 ms of audio at |frequencyInHz| to |outBuffer|. |lengthInSamples|
   // will be set to the number of samples read (not the number of samples per
   // channel).
@@ -72,9 +81,6 @@ class FilePlayer {
 
   // Note: scaleFactor is in the range [0.0 - 2.0]
   virtual int32_t SetAudioScaling(float scaleFactor) = 0;
-
- protected:
-  virtual ~FilePlayer() {}
 };
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_UTILITY_INCLUDE_FILE_PLAYER_H_