FileRecorder + FilePlayer: Let Create functions return unique_ptr

webrtc/modules/utility/include/file_player.h

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #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 {
  public:
   // The largest decoded frame size in samples (60ms with 32kHz sample rate).
   enum { MAX_AUDIO_BUFFER_IN_SAMPLES = 60 * 32 };
   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);
 
-  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).
   virtual int Get10msAudioFromFile(int16_t* outBuffer,
                                    size_t& lengthInSamples,
                                    int frequencyInHz) = 0;
 
   // Register callback for receiving file playing notifications.
   virtual int32_t RegisterModuleFileCallback(FileCallback* callback) = 0;
@@ skipping 22 matching lines @@
 
   virtual int32_t GetPlayoutPosition(uint32_t& durationMs) = 0;
 
   // Set audioCodec to the currently used audio codec.
   virtual int32_t AudioCodec(CodecInst& audioCodec) const = 0;
 
   virtual int32_t Frequency() const = 0;
 
   // 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_