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

content/browser/media/audio_output_impl.h

Index: content/browser/media/audio_output_impl.h
diff --git a/content/browser/media/audio_output_impl.h b/content/browser/media/audio_output_impl.h
new file mode 100644
index 0000000000000000000000000000000000000000..7b91b65bf59801b98c771b4bd3f8fce544a2997d
--- /dev/null
+++ b/content/browser/media/audio_output_impl.h
@@ -0,0 +1,89 @@
+// 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_IMPL_H_
+#define CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_
+
+#include "base/callback.h"
+#include "base/sync_socket.h"
+#include "content/browser/renderer_host/media/audio_renderer_host.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 base {
+class SharedMemory;
+}
+
+namespace content {
+
+class AudioOutputStreamImpl;
+
+// This class implements AudioOutput Mojo interface. It will be used

[nasko, 2016/05/25 20:50:42]

nit: "will" or "is"?

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

Done.

+// by the AudioOutputClient to perform operations on audio output streams.
+// AudioOutputClient is going to perform operations remotely and Mojo will

[nasko, 2016/05/25 20:50:42]

In general, you should avoid using future tense in comments. Once the
implementation is done, the comment is incorrect, so it should be present tense.

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

Done.

+// transfer them to AudioOutputImpl.
+// This class must always be accessed from the creation thread.
+class CONTENT_EXPORT AudioOutputImpl
+    : NON_EXPORTED_BASE(public media::mojom::AudioOutput) {
+ public:
+  explicit AudioOutputImpl(RenderProcessHost* process_,
+                           int render_frame_id,
+                           media::mojom::AudioOutputRequest request);
+
+  ~AudioOutputImpl() override;
+
+  // Creates a Mojo service.
+  static void CreateService(RenderProcessHost* process_,
+                            int render_frame_id,
+                            media::mojom::AudioOutputRequest request);
+  // Initialized Mojo service on the IO thread. The service is bound to the

[nasko, 2016/05/25 20:50:42]

Empty line before comment.

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

Done.

+  // IO thread so future Mojo calls performed by the AudioOutputClient to the
+  // AudioOutput interface will be run on that thread.
+  // There are a lot of dependencies used in audio output that require to be
+  // called on the IO thread, this is why we initialize Mojo service on this
+  // thread. There is a plan to enable these dependencies to run on another

[nasko, 2016/05/25 20:50:42]

nit: Put this plan on a new line with TODO(): prefix and a bug number for it.

+  // thread and move the Mojo service to this thread as IO thread is very

[nasko, 2016/05/25 20:50:42]

nit: Overuse of the word "thread" and the comment becomes hard to read.

+  // performance sensitive and we want to move stuff off it.
+  static void CreateServiceOnIOThread(RenderProcessHost* process_,

[nasko, 2016/05/25 20:50:42]

No underscores on parameters, those are reserved for member variables.

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

Done.

+                                      int render_frame_id,
+                                      media::mojom::AudioOutputRequest request);
+
+  // Called by AudioRendererHost::DoCompleteCreation to create the streams.
+  virtual void CreateStreamFactory(
+      int stream_id,
+      base::SharedMemory* shared_memory,
+      base::SyncSocket::TransitDescriptor socket_descriptor);
+
+  virtual bool CloseStream(int stream_id);
+  // Send an error message to the renderer, then close the stream.

[nasko, 2016/05/25 20:50:42]

Empty line before comment.

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

Done.

+  virtual void ReportErrorAndCloseStream(int stream_id);
+
+ private:
+  friend class AudioOutputImplTest;
+  friend class MockAudioOutputImpl;
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, CreateStream);
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, StreamFactory);
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, RemoveStream);
+
+  static void Disconnect(int process_id, int render_frame_id);

[nasko, 2016/05/25 20:50:42]

Add comment describing what the method does.

[rchtara, 2016/05/27 16:25:22]

Done.

+
+  // AudioOutput interface implementation.
+  void CreateStream(int stream_id,
+                    const media::AudioParameters& params,
+                    const CreateStreamCallback& callback) override;
+
+  RenderProcessHost* process_;
+  int render_frame_id_;

[nasko, 2016/05/25 20:50:42]

Instead of these two variables, why not keep a pointer to the RenderFrameHost
this object is associated with?

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

Done.

+  mojo::Binding<AudioOutput> binding_;
+  std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;
+  std::map<int, media::mojom::AudioOutput::CreateStreamCallback>
+      create_stream_callbacks_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioOutputImpl);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_