Redesign the copresence audio handlers.

components/copresence/mediums/audio/audio_manager.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_
+#define COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/cancelable_callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_vector.h"
+#include "components/copresence/mediums/audio/audio_recorder.h"
+#include "components/copresence/public/copresence_constants.h"
+#include "components/copresence/timed_map.h"
+
+namespace base {
+class MessageLoop;
+}
+
+namespace media {
+class AudioBus;
+}
+
+namespace copresence {
+
+class AudioPlayer;
+
+// The AudioManager class will record audio until told to stop.

[Charlie, 2014/10/17 22:42:26]

Provide some more detail. What is this class responsible for? What functionality
does it add on top of the bare player and recorder?

[rkc, 2014/10/18 00:21:55]

Done.

+class AudioManager {
+ public:
+  typedef base::Callback<void(const std::string&,
+                              AudioType,
+                              const scoped_refptr<media::AudioBusRefCounted>&)>
+      SamplesCallback;
+  typedef base::Callback<void(const std::string&,
+                              AudioType,
+                              const SamplesCallback&)> EncodeTokenCallback;
+  typedef base::Callback<void(AudioType, const std::string&)>
+      DecodeSamplesCallback;
+
+  AudioManager();
+  virtual ~AudioManager();
+
+  // Initializes the object. Do not use this object before calling this method.
+  virtual void Initialize(const DecodeSamplesCallback& decode_cb,

[Charlie, 2014/10/17 22:42:26]

In this new class, don't use the "new Foo(); foo->Initialize()" pattern. Instead
make the constructor private and provide a static Create() method. See
crbug/424735.

[rkc, 2014/10/18 00:21:55]

I am not quite convinced that is the way to go. Again, let's discuss this
offline.

+                          const EncodeTokenCallback& encode_cb);
+
+  virtual void StartPlaying(AudioType type);
+  virtual void StopPlaying(AudioType type);
+
+  virtual void StartRecording(AudioType type);
+  virtual void StopRecording(AudioType type);
+
+  virtual void SetToken(AudioType type, const std::string& url_unsafe_token);
+
+  const std::string& GetToken(AudioType type) { return token_[type]; }

[Daniel Erat, 2014/10/17 22:26:00]

nit: rename to get_token() since it's a simple inlined method

[rkc, 2014/10/18 00:21:55]

Done.

+
+  // For test.

[Daniel Erat, 2014/10/17 22:26:00]

nit: don't need this comment; method names already have _for_testing in them

[Charlie, 2014/10/17 22:42:26]

The first two of these can be folded into the Create() method. The rest belong
in MockAudioManager.

[rkc, 2014/10/18 00:21:55]

Done.

[rkc, 2014/10/18 00:21:55]

Acknowledged.

+  void set_player_for_testing(AudioType type, AudioPlayer* player) {
+    player_[type] = player;
+  }
+

[Daniel Erat, 2014/10/17 22:26:00]

nit: i'd probably delete all the blank lines between these setters

[rkc, 2014/10/18 00:21:55]

Done.

+  void set_recorder_for_testing(AudioRecorder* recorder) {
+    recorder_ = recorder;
+  }
+
+  void set_recording_for_testing(AudioType type, bool is_recording) {
+    recording_[type] = is_recording;
+  }
+
+  void set_playing_for_testing(AudioType type, bool is_playing) {
+    playing_[type] = is_playing;
+  }
+
+  bool is_recording_for_testing(AudioType type) { return recording_[type]; }
+
+  bool is_playing_for_testing(AudioType type) { return playing_[type]; }
+
+ private:
+  friend class AudioManagerTest;
+  FRIEND_TEST_ALL_PREFIXES(AudioManagerTest, BasicRecordAndStop);
+  FRIEND_TEST_ALL_PREFIXES(AudioManagerTest, OutOfOrderRecordAndStopMultiple);
+
+  typedef TimedMap<std::string, scoped_refptr<media::AudioBusRefCounted>>
+      SamplesMap;
+
+  // This is the method that the whispernet client needs to call to return
+  // samples to us.
+  void OnTokenEncoded(const std::string& token,
+                      AudioType type,
+                      const scoped_refptr<media::AudioBusRefCounted>& samples);
+
+  // Update our currently playing token with the new token. Change the playing
+  // samples if needed. Prerequisite: Samples corresponding to this token
+  // should already be in the samples cache.
+  void UpdateToken(AudioType type, const std::string& token);
+
+  // Connector callback to forward the audio samples to Whispernet, based on
+  // the type of recording we've been instructed to do.
+  void DecodeSamplesConnector(const std::string& samples);
+
+  // Callbacks to speak with whispernet.
+  DecodeSamplesCallback decode_cb_;
+  EncodeTokenCallback encode_cb_;
+
+  // We need this cancelable callback to pass to the recorder. The recorder's
+  // destruction will happen on the audio thread, so it can outlive us.
+  base::CancelableCallback<void(const std::string&)> decode_cancelable_cb_;
+
+  // 0 = AUDIBLE, 1 = INAUDIBLE.

[Daniel Erat, 2014/10/17 22:26:00]

please instead just document the name of the enum that's used to index into
these. you should probably also add compile-time assertions that their values
are less than 2.

[rkc, 2014/10/18 00:21:55]

Done.

+  bool playing_[2];
+  bool recording_[2];
+
+  // AudioPlayer and AudioRecorder objects are self-deleting. When we call
+  // Finalize on them, they clean themselves up on the Audio thread.
+  // 0 = AUDIBLE, 1 = INAUDIBLE.
+  AudioPlayer* player_[2];
+  AudioRecorder* recorder_;
+
+  // 0 = AUDIBLE, 1 = INAUDIBLE.
+  std::string token_[2];
+
+  // Cache that holds the encoded samples. After reaching its limit, the cache
+  // expires the oldest samples first.
+  // 0 = AUDIBLE, 1 = INAUDIBLE.
+  ScopedVector<SamplesMap> samples_cache_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioManager);
+};
+
+}  // namespace copresence
+
+#endif  // COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_