Switch stream creation and closing in Chrome audio rendering from IPC to Mojo

content/browser/media/audio_output_stream_impl.h

+// Copyright 2016 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 CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_STREAM_IMPL_H_
+#define CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_STREAM_IMPL_H_
+
+#include "base/callback.h"
+#include "content/browser/media/audio_output_impl.h"
+#include "content/browser/renderer_host/media/audio_renderer_host.h"
+#include "content/common/content_export.h"
+#include "media/mojo/interfaces/audio_output.mojom.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+
+namespace content {
+
+// This class implements AudioOutputStream Mojo interface. It will be used
+// by the AudioOutputClient to perform operations on audio output streams.

[Henrik Grunell, 2016/05/20 13:35:40]

Same here as my comment in AudioOutputImpl.

+// AudioOutputClient will perform operations remotely on the
+// AudioOutputStreamPtr and Mojo will transfer them to AudioOutputStreamImpl.
+// Owned by AudioOutputImpl.
+class CONTENT_EXPORT AudioOutputStreamImpl
+    : public media::mojom::AudioOutputStream {
+ public:
+  AudioOutputStreamImpl(media::mojom::AudioOutputStreamRequest request,

[tommi (sloooow) - chröme, 2016/05/20 13:26:13]

by value on purpose?

+                        int stream_id,
+                        int renderer_frame_id,
+                        AudioRendererHost* audio_renderer_host);
+
+  ~AudioOutputStreamImpl() override;
+
+  int get_stream_id() const { return stream_id_; }
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputStreamImplTest, Close);
+  // AudioOutputStream interface implementation.

[Henrik Grunell, 2016/05/20 13:35:40]

Nit: empty line before.

[rchtara, 2016/05/27 15:24:37]

Done.

+  void Close() override;
+
+  mojo::Binding<media::mojom::AudioOutputStream> binding_;
+  int stream_id_;

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

can this be made const?

[rchtara, 2016/05/27 15:24:37]

Done.

+  int renderer_frame_id_;

[tommi (sloooow) - chröme, 2016/05/20 13:26:13]

and this?

[rchtara, 2016/05/27 15:24:37]

that one is not needed

+  // AudioRendererHost is ref counted and AudioOutputImpl holds a reference to
+  // it. And because all AudioOutputStreamImpl instances will be owned by
+  // AudioOutputImpl too, it's safe to access AudioRendererHost from
+  // AudioOutputStreamImpl.
+  // The only issue that could happen is during AudioOutputStreamImpl

[Henrik Grunell, 2016/05/20 13:35:40]

How can that be an issue?

[rchtara, 2016/05/27 15:24:37]

outdated comment

+  // destructor. AudioRendererHost cannot access from there.
+  AudioRendererHost* audio_renderer_host_;

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

and this?

(i.e. AudioRendererHost* const audio_renderer_host_)

[rchtara, 2016/05/27 15:24:37]

Done.

+};
+
+class CONTENT_EXPORT AudioOutputStreamFactory {

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

mark the default ctor as delete?

Why does this need to be a class btw?

[rchtara, 2016/05/27 15:24:37]

this was needed for the old version tests, but it's removed now

+ public:
+  static AudioOutputStreamImpl* Factory(

[Henrik Grunell, 2016/05/20 13:35:40]

Put this function in AudioOutputStreamImpl and name it Create(). (And remove
AudioOutputStreamFactory.)

[rchtara, 2016/05/27 15:24:37]

not needed anymore

+      media::mojom::AudioOutputStreamRequest request,
+      int stream_id,
+      int renderer_frame_id,
+      AudioRendererHost* audio_renderer_host);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_STREAM_IMPL_H_