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 "components/copresence/public/copresence_constants.h"
+
+namespace media {
+class AudioBusRefCounted;
+}
+
+namespace copresence {
+
+class AudioManager {
+ public:
+  typedef base::Callback<void(const std::string&,
+                              AudioType,
+                              const scoped_refptr<media::AudioBusRefCounted>&)>
+      SamplesCallback;

[Daniel Erat, 2014/10/22 16:34:35]

it seems confusing to have one callback named "DecodeSamplesCallback" and
another that's just named "SamplesCallback" -- how about "TokenSamplesCallback",
then?

[rkc, 2014/10/22 18:21:47]

Done.

+  typedef base::Callback<void(const std::string&,
+                              AudioType,
+                              const SamplesCallback&)> EncodeTokenCallback;
+  typedef base::Callback<void(AudioType, const std::string&)>
+      DecodeSamplesCallback;
+
+  virtual ~AudioManager() {}
+
+  // Initializes the object. Do not use this object before calling this method.
+  virtual void Initialize(const DecodeSamplesCallback& decode_cb,
+                          const EncodeTokenCallback& encode_cb) = 0;
+
+  virtual void StartPlaying(AudioType type) = 0;
+  virtual void StopPlaying(AudioType type) = 0;
+
+  virtual void StartRecording(AudioType type) = 0;
+  virtual void StopRecording(AudioType type) = 0;
+
+  virtual void SetToken(AudioType type,
+                        const std::string& url_unsafe_token) = 0;
+
+  virtual const std::string& GetToken(AudioType type) = 0;

[Daniel Erat, 2014/10/22 16:34:35]

if the returned value is short, please just return std::string directly

[rkc, 2014/10/22 18:21:47]

Any particular reason for not returning a const reference? it is a const, so the
returned reference won't be changed (without an explicit const cast anyway at
least).

[Daniel Erat, 2014/10/22 18:32:40]

in general, i find that it's better not to impose restrictions on
implementations of an interface. for example, i usually avoid const methods in
interfaces since it precludes future implementations from caching values
internally. in this case, i think that returning a const reference meant that a
stub implementation needed to have a weird empty string member that it never
sent to anything. if the method isn't called over and over, it may be cleaner to
just return a copy.

[rkc, 2014/10/22 19:28:46]

So this code will be called 'fairly' often; considering that every time we
upload a new token, we upload our currently playing tokens to - so this is
something that will at least get called once every half a second - but that's a
lifetime in terms of actual cycles. If we change this to return a new string
though, the function that calls this (which also returns a reference) and the
function that calls that, will all need to be changed, and now this will return
a copy, which will return a copy, which will return a copy; which starts to add
up.

If the side-effect of this is a some weird code in a test (which will never
actually get called, even in that test), then this should be fine?

[Daniel Erat, 2014/10/22 21:50:15]

my larger point was that specifying a reference return value in the interface
restricts what implementations can do. if a future implementation wants to
generate a random token, or return a token that it fetches from some other class
that doesn't return a const reference, suddenly it needs to find some way to
hold on to the memory until the caller is done using it (how will it know when
that's happened?).

and this case seems particularly bad, since there's a SetToken() method. i can
imagine implementations where calling that would invalidate
previously-handed-out references. the current implementation avoids this by
using an array of std::strings, but it could just as easily be using a vector.

so i'll go further: only return references if the lifetime of the underlying
memory is very clear (e.g. a read-only member that gets set in the c'tor of a
non-derived class).

+
+  virtual bool IsRecording(AudioType type) = 0;
+  virtual bool IsPlaying(AudioType type) = 0;
+};
+
+}  // namespace copresence
+
+#endif  // COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_