Mojo interfaces needed for switching audio rendering stream creation and closing 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..0231dbb3bf8f70247ea8fe5fc764fb9339375057
--- /dev/null
+++ b/content/browser/media/audio_output_impl.h
@@ -0,0 +1,91 @@
+// 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/compiler_specific.h"
+#include "base/memory/ref_counted.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 base {
+class FilePath;
+}
+
+namespace content {
+
+class CONTENT_EXPORT AudioOutputStreamImpl : public mojom::AudioOutputStream {
+  // All the instance of this class are going to be owned by AudioOutputImpl.

[Henrik Grunell, 2016/04/22 08:23:03]

Put comment before the "class" line.

[Henrik Grunell, 2016/04/22 08:23:04]

Add a brief general description of the class. Then you can just have "Owned by
AudioOutputImpl." after that.

[rchtara, 2016/04/29 12:54:44]

Done.

[rchtara, 2016/04/29 12:54:46]

Done.

+ public:
+  explicit AudioOutputStreamImpl(

[xhwang, 2016/04/26 22:54:42]

no explicit

[rchtara, 2016/04/29 12:54:45]

Done.

+      mojo::InterfaceRequest<mojom::AudioOutputStream> request,

[tommi (sloooow) - chröme, 2016/04/26 15:29:53]

Niklas - can you recommend someone to remove this CL from the mojo perspective?

+      int stream_id,
+      AudioRendererHost* audio_renderer_host);

[tommi (sloooow) - chröme, 2016/04/26 15:29:53]

document ownership/lifetime of raw pointers

[rchtara, 2016/04/29 12:54:45]

I have done that while describing |audio_renderer_host_|

+
+  // AudioOutputStream interface implementation.
+  void Close() override;

[tommi (sloooow) - chröme, 2016/04/26 15:29:53]

make this private? (assuming callers of this method will have an interface
pointer and not a pointer to this implementation.

[rchtara, 2016/04/29 12:54:45]

Done.

+
+  ~AudioOutputStreamImpl() override;

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

dtor follows ctor(s) in class layout

[rchtara, 2016/04/29 12:54:45]

Done.

+
+ private:
+  mojo::StrongBinding<mojom::AudioOutputStream> binding_;
+  int stream_id_;
+  // |audio_renderer_host_| is scoped_refptr in AudioOutputImpl. And because all

[Henrik Grunell, 2016/04/22 08:23:03]

You could put it as:
"AudioRendererHost is ref counted and AudioOutputImpl holds a reference to it."

[rchtara, 2016/04/29 12:54:45]

Done.

+  // AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl

[Henrik Grunell, 2016/04/22 08:23:03]

"going to be" means that it will happen in the future.

Maybe something like "AudioOutputStreamImpl is owned by AudioOutputImpl".

[rchtara, 2016/04/29 12:54:46]

Done.

+  // too, it's safe to access |audio_renderer_host| from AudioOutputStreamImpl.
+  // The only issue that could happen is during AudioOutputStreamImpl
+  // destructor. |audio_renderer_host| cannot access from there.
+  AudioRendererHost* audio_renderer_host_;
+};
+
+// This class must always be accessed from the creation thread.
+class CONTENT_EXPORT AudioOutputImpl
+    : NON_EXPORTED_BASE(public mojom::AudioOutput) {
+ public:
+  AudioOutputImpl(mojom::AudioOutputRequest request);

[xhwang, 2016/04/26 22:54:42]

explicit

[rchtara, 2016/04/29 12:54:45]

Done.

+
+  ~AudioOutputImpl() override;
+
+  // Create a Mojo service.
+  static void CreateService(
+      scoped_refptr<AudioRendererHost> audio_renderer_host,
+      mojom::AudioOutputRequest request);

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

passed by value on purpose?

[rchtara, 2016/04/29 12:54:45]

yes it's a mojo::InterfaceRequest<mojom::AudioOutput> which needs to be handle
as if it's a smart pointer. If I start using references, the compiler will give
me a lot of error.
   

+
+  // Initialize Mojo on the BrowserThread::IO. Mojo is going to be bound to

[Henrik Grunell, 2016/04/22 08:23:03]

Does it create or only initialize? Both the comment and function name should
reflect what it does.

"Mojo" -> "Mojo service"

"BrowserThread::IO" -> "IO thread"

"Mojo is going to be bound" -> "The service is bound"

Rephrase/clarify "future Mojo calls". (What is a "mojo call"?)

[rchtara, 2016/04/29 12:54:45]

Done.

+  // BrowserThread::IO so future Mojo calls are going to be run on
+  // that thread.

[xhwang, 2016/04/26 22:54:41]

Could you please explain more why this service has to run on the IO thread?
Typically IO thread is very performance sensitive and we want to move stuff off
the IO thread.

[rchtara, 2016/04/29 12:54:45]

Done.

+  static void CreateServiceOnIOThread(
+      scoped_refptr<AudioRendererHost> audio_renderer_host,
+      mojo::InterfaceRequest<mojom::AudioOutput> request);

[xhwang, 2016/04/26 22:54:42]

You can use mojom::AudioOutputRequest now I believe.

[rchtara, 2016/04/29 12:54:45]

Done.

+
+  // Called by AudioRendererHost::DoCompleteCreation to create the streams.
+  mojom::AudioOutputStreamPtr* StreamFactory(
+      int stream_id,
+      AudioRendererHost* audio_renderer_host);
+
+ private:
+

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

remove this empty line

[rchtara, 2016/04/29 12:54:45]

Done.

+  // AudioOutput interface implementation.
+  void CreateStream(int stream_id,
+                    int render_frame_id,
+                    media::interfaces::AudioOutputStreamParametersPtr params,

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

intentionally by value?

[rchtara, 2016/04/29 12:54:45]

AudioOutputStreamParametersPtr is just a smart pointer: InterfacePtr
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
This method is going to be called by the mojo client and this is how it should
be called.

+                    const CreateStreamCallback& callback) override;
+
+  // Mojo connection error callback.
+  void OnDisconnect(AudioOutputImpl* impl);
+
+  scoped_refptr<AudioRendererHost> audio_renderer_host_;
+  mojo::StrongBinding<AudioOutput> binding_;
+  std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;

[tommi (sloooow) - chröme, 2016/04/26 15:29:53]

since there's no lock in this class, can we use a thread checker to ensure that
all method calls execute on expected threads?

[rchtara, 2016/04/29 12:54:45]

Done.

+  DISALLOW_COPY_AND_ASSIGN(AudioOutputImpl);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_