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

Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 
https://codereview.chromium.org/1896883002

BUG=606707
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[rchtara, 2016-04-29 13:48:01]

Description was changed from

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is just the content part of the CL. The media part is in CL 1896883002.

BUG=606707
==========

to

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 1896883002.

BUG=606707
==========

[rchtara, 2016-04-29 14:07:03]

rchtara@chromium.org changed reviewers:
+ grunell@chromium.org, tommi@chromium.org

[rchtara, 2016-04-29 16:14:32]

rchtara@chromium.org changed reviewers:
+ mcasas@chromium.org, niklase@chromium.org, xingnan.wang@chromium.org

[rchtara, 2016-04-29 16:18:17]

On 2016/04/29 16:14:32, rchtara wrote:
> mailto:rchtara@chromium.org changed reviewers:
> + mailto:mcasas@chromium.org, mailto:niklase@chromium.org,
mailto:xingnan.wang@chromium.org

This is the content part of the cl.

[mcasas, 2016-04-29 18:29:01]

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.cc:60:
audio_renderer_host->audio_output_impl_ = service;
Don't make AudioOutput*Impl friend of AudioRendererHost
to allow these plugging operations. Make methods at 
the very least.

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.cc:89: std::pair<int,
std::unique_ptr<AudioOutputStreamImpl>>(
std::make_pair(stream_id, std::move(stream_ptr));

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.h:27: class CONTENT_EXPORT
AudioOutputStreamImpl
Either spin this class into a file on
its own, or forward declare around l.94 and
move definition to audio_output_impl.cc

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:57: void
SetAudioOutputClient(content::AudioOutputClient* audio_output_client) {
s/SetAudioOutputClient/set_audio_output_client/

https://www.chromium.org/developers/coding-style#TOC-Inline-functions

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:106:
scoped_ptr<content::AudioOutputClient> audio_output_client_;
std::unique_ptr.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_output_client.cc (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:32: if (it.second.get() == key) {
No {} for one-line bodies

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:35: }
Nit: Consider std::find_if()

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:36: return -1;
Consider making |kStreamIDNotSet| not 
public (and/or rename to |kStreamIDNotValid|)
and using it here.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:49:
&AudioOutputClient::OnConnectionError, base::Unretained(this)));
base::Unretained should be used sparingly.
Since here you just want to get a DLOG(ERROR),
move OnConnectionError() to the anonymous namespace 
in l.26, and make it a file-static function.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:51: service_ = 0;
No need to do this (and is semantically wrong),
instead check for service_.is_bound() in l.82.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:58: LOG(ERROR) << "Mojo client
connection error";
s/LOG/DLOG/
(I think there's a presubmit warning for this).

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:64: base::Unretained(this),
stream_id));
base::Unretained() Should not be used. If this
class is genuinely multi-threaded, consider 
making it ref-counted, but more often than not,
a WeakPtrFactory is a better solution, although
it poses restrictions on where the destructor is
run.

http://www.chromium.org/developers/coding-style/important-abstractions-and-da...

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:70:
main_thread_task_runner_->PostTask(
Consider merging with CreateStreamOnMainThread():

if (!main_thread_task_runner_->BelongsToCurrentThread()) {
  // PostTask...
  return;
}
DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
if (!service)  // Should be if (!service.is_bound())
  return;
service->CreateStream(...)

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:116: LOG(ERROR) << "Failed to pass
shared memory. Closing: "
s/LOG/DLOG/

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:167:
streams_[stream_id]->get()->Close();
Remove get()->, a unique_ptr behaves like a
pointer.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_output_client.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:39: class CONTENT_EXPORT
AudioOutputClient {
Please add class comments, including the
threading model. I see methods OnMainThread(),
OnIoThread() and without thread indication.
Per thread in use, you should strive for
having a 
DCHECK(..._thread_task_runner_->BelongsToCurrentThread());
or
DCHECK(...thread_checker().CalledOnValidThread());
to document in-code where is the method
being called.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:43: ScopedAudioOutputStreamPtrMap;
using ScopedAudioOutputStreamPtrMap = ...
?

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:45: // Create a Mojo Audio Output
Client. It's going to be created by
Method documentation are descriptive:
s/Create/Creates/, here and throughout.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:46: // RenderThreadImpl::init and
be bound to RenderThread. This means that all
init()

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:100: void OnStreamError(int
stream_id);
Can be private I believe. Check the
other methods and make private as many
as possible.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:111:
scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
const.

Leave a blank line before the next comment,
however, it being superfluous, just remove it.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.h:568: AudioOutputClient*
audio_output_client_;
Unused, right? Also the include in l.32

[tommi (sloooow) - chröme, 2016-04-29 20:19:04]

The CQ bit was checked by tommi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 20:19:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1930393002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1930393002/1

[commit-bot: I haz the power, 2016-04-29 20:22:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 20:22:06]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Henrik Grunell, 2016-05-02 12:12:24]

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.h:27: class CONTENT_EXPORT
AudioOutputStreamImpl
On 2016/04/29 18:29:00, mcasas wrote:
> Either spin this class into a file on
> its own, or forward declare around l.94 and
> move definition to audio_output_impl.cc

Agree, put in its own file.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:80: 0,      //
dwDesiredAccess ignored due to SAME_ACCESS
Two spaces after "0," instead of aligning with the below comment.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:229:
create_stream_callback_ = callback;
Put it in the initialization list?

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:303:
base::Bind(&AudioRendererHost::DoCompleteCreation, host_, stream_id_,
Could |callback_| be used again? If not, what about clearing it after the
PostTask and a DCHECK before to make sure it's valid?

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:388: // reset.
AudioOutputIPCDelegate is going to need to close the socket handle
AudioOutputIPCDelegate is an implementation detail on the client side, so don't
mention it here.

It sounds like the socket handle ownership should be transferred to the client.
Why is it closed in |mojo_application_host_| and can we avoid that?

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:791:
mojo::ScopedSharedBufferHandle shared_buffer_handle =
Nit: add empty line before this.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:114: friend class
AudioEntry;
This should not be needed. Anything that needs to be accessed should go through
getter/setter functions.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:224: // Send an error
message to the renderer, then close the stream.
Change "renderer" to "client", it's more generic.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:264: AudioOutputImpl*
audio_output_impl_;
Add a comment about this. In that also explain how a raw pointer is safe, i.e.
why it outlives |this|.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:289:
media::interfaces::AudioOutputStreamPtr stream_ptr_;
Add comment.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host_unittest.cc
(right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:99: void
OnNotifyStreamCreated(
As we talked about offline, this function should either be moved to the test
class next to the CreateCallback function, or be merged with that function.
(Depending on how much code is still left after fixing the next comment.) If you
keep the function, it could be named something like "VerifyXyz...".

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105:
shared_memory_.reset(new base::SharedMemory(handle, false));
As we talked about offline, storing in member variables in the original code
doesn't makes sense. They are not used anywhere. Remove the member variables
from the class and instead make sure to test whatever makes sense to test.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:322:
streams_[stream_id] = make_scoped_ptr(
This isn't used anywhere. We talked about this offline but I don't remember why
this is needed. Could you explain that?

https://codereview.chromium.org/1930393002/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/1930393002/diff/1/content/content_browser.gyp...
content/content_browser.gypi:81:
'../media/mojo/interfaces/mojo_bindings.gyp:audio_output_mojom_bindings',
Remove duplicate.

https://codereview.chromium.org/1930393002/diff/1/content/content_browser.gyp...
content/content_browser.gypi:1752: 'browser/media/audio_output_impl.cc',
This is not only for webrtc. Put them in the private_browser_sources target.

https://codereview.chromium.org/1930393002/diff/1/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/1930393002/diff/1/content/content_renderer.gy...
content/content_renderer.gypi:50: '../mojo/mojo_public.gyp:mojo_cpp_bindings',
Put them in alphabetical order.

https://codereview.chromium.org/1930393002/diff/1/content/content_renderer.gy...
content/content_renderer.gypi:285: 'renderer/media/audio_output_client.cc',
Ditto.

https://codereview.chromium.org/1930393002/diff/1/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1930393002/diff/1/content/content_tests.gypi#...
content/content_tests.gypi:764: 'browser/media/audio_output_impl_unittest.cc',
Same here. It's not only WebRTC.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:65: friend class
AudioOutputClient;
This should not be needed. Use setter/getter functions instead.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:106:
scoped_ptr<content::AudioOutputClient> audio_output_client_;
When, after follow-up CLs, this class is removed, who should own the
AudioOutputClient?

[rchtara, 2016-05-02 17:23:51]

Description was changed from

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 1896883002.

BUG=606707
==========

to

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 
https://codereview.chromium.org/1896883002

BUG=606707
==========

[rchtara, 2016-05-17 16:37:06]

Description was changed from

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 
https://codereview.chromium.org/1896883002

BUG=606707
==========

to

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 
https://codereview.chromium.org/1896883002

BUG=606707
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Henrik Grunell, 2016-05-18 10:29:07]

Just a few random comments.

https://codereview.chromium.org/1930393002/diff/80001/content/browser/media/a...
File content/browser/media/audio_output_impl_unittest.cc (right):

https://codereview.chromium.org/1930393002/diff/80001/content/browser/media/a...
content/browser/media/audio_output_impl_unittest.cc:45: }
Nit: empty line above and add "  // namespace".

https://codereview.chromium.org/1930393002/diff/80001/content/browser/media/a...
content/browser/media/audio_output_impl_unittest.cc:92:
FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, CreateServiceOnIOThread);
This should not be needed.

https://codereview.chromium.org/1930393002/diff/80001/content/browser/media/a...
content/browser/media/audio_output_impl_unittest.cc:133: protected:
protected goes before private.

https://codereview.chromium.org/1930393002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host_unittest.cc
(right):

https://codereview.chromium.org/1930393002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:156:
std::unique_ptr<base::SharedMemory> shared_memory_;
Looks like you can remove these?

https://codereview.chromium.org/1930393002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:316:
host_->OnStreamCreated(stream_id, length);
This is odd; call back to the host again. It kind of made sense when in the
OnNotify...() function in the MockAudioRendererHost, but not here. I guess it's
used to check that we got that call (i.e. with EXPECT_CALL)?

https://codereview.chromium.org/1930393002/diff/80001/content/renderer/media/...
File content/renderer/media/audio_message_filter_unittest.cc (right):

https://codereview.chromium.org/1930393002/diff/80001/content/renderer/media/...
content/renderer/media/audio_message_filter_unittest.cc:116:
filter->set_audio_output_client(kRenderFrameId, new AudioOutputClient(NULL));
NULL -> nullptr. Same below.

https://codereview.chromium.org/1930393002/diff/80001/content/renderer/media/...
content/renderer/media/audio_message_filter_unittest.cc:135: //
AudioMsg_NotifyStreamCreated
Change comment to probably "OnStreamCreated".

https://codereview.chromium.org/1930393002/diff/80001/content/renderer/media/...
content/renderer/media/audio_message_filter_unittest.cc:151:
filter->OnStreamCreated(kStreamId, base::SharedMemory::NULLHandle(),
Comment on why we need to call both OnMessageReceived() and OnStreamCreated().

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

got some comments on the first few files and since there are some issues with
formatting, removing debug code and style guide that need to be fixed for the
whole CL, I'll come back when that's done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1938:
GetServiceRegistry()->AddService(
run git cl format?  I'm guessing it will delete the extra whitespace below too
but in case it doesn't, please fix.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:19: LOG(ERROR) << "AudioOutputImpl";
?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:24: LOG(ERROR) << "~AudioOutputImpl";
please remove all of these

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:46:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
why would we only be on the io thread if the request is pending? (seems like
there might be a bug here)

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:52:
process->audio_renderer_host()->set_audio_output_impl(render_frame_id, service);
please run git cl format

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:78: DCHECK(ret.second);
do you get a build error for official builds?  (ret becomes an unused variable)

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:87: return false;
please just do:

return stream_impls_.erase(stream_id) != 0;

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:36: // Initialized Mojo service on the
IO thread. The service is bound to the
add an empty line above this one.

Should the first sentence be "Returns an initialized ..."?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:45: RenderProcessHost* process_,
process (no underscore for params)

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:47: media::mojom::AudioOutputRequest
request);
passed by value on purpose?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:71: std::map<int,
std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;
can you add information about this map? (maps from ? to AudioOutputStreamImpl)

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:76: AudioRendererHost*
audio_renderer_host)>
can you create a typedef for this?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl_unittest.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl_unittest.cc:170: unsigned int i = 0;
we don't use "unsigned int"

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl_unittest.cc:171: for (i = 0; i <
kStreamIdsSize; i++) {
also 'i' should be declared for the scope it's used

and use ++i

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:26:
AudioOutputStreamImpl(media::mojom::AudioOutputStreamRequest request,
by value on purpose?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:41: int stream_id_;
can this be made const?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:42: int renderer_frame_id_;
and this?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:49: AudioRendererHost*
audio_renderer_host_;
and this?

(i.e. AudioRendererHost* const audio_renderer_host_)

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:52: class CONTENT_EXPORT
AudioOutputStreamFactory {
mark the default ctor as delete?

Why does this need to be a class btw?

[Henrik Grunell, 2016-05-20 13:35:41]

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:20: factory_ =
base::Bind(AudioOutputStreamFactory::Factory);
I suppose |factory_| is needed for being modified in tests. Instead, pass the
factory in the constructor.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:46:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Why can it be called on another thread if not pending? Should be just the
DCHECK?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:52:
process->audio_renderer_host()->set_audio_output_impl(render_frame_id, service);
This is what we talked about offline, right? It won't work since this mojo
service when existing per frame will be created before the AudioRendererHost.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:64:
media::mojom::AudioOutputStreamPtr AudioOutputImpl::StreamFactory(
It seems to be unnecessarily complicated to call the ARH above, which then calls
back here to create the stream. Is it possible to move all the code here to ARH?
Or perhaps create the AOSI in CreateStream() and pass it to ARH directly?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:18: // by the AudioOutputClient to
perform operations on audio output streams.
This isn't really correct. AudioOutputClient doesn't "use" this class as in
calls functions on it. You could instead write that it's connected to the
AudioOutputClient.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:25: explicit
AudioOutputImpl(RenderProcessHost* process_,
No trailing _

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:36: // Initialized Mojo service on the
IO thread. The service is bound to the
Empty line before comment.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:41: // thread. There is a plan to
enable these dependencies to run on another
I don't think there is a plan for doing that here on the browser side. On the
render side it does. So, you should remove that part of the comment.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:63: // AudioOutput interface
implementation.
Empty line before comment.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:71: std::map<int,
std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;
Comment on why this is needed. It's just for ownership and deleting afaict.

Would it make sense to pass the ownership to the AudioRendererHost?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:77: factory_;
Indentation seems odd. Run git cl format.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:78:
DISALLOW_COPY_AND_ASSIGN(AudioOutputImpl);
Empty line before.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.cc:27: void
AudioOutputStreamImpl::Close() {
Do we do the same as here for all AudioOutputStreamImpls if the connection is
closed?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.cc:30:
audio_renderer_host_->get_audio_output_impl(renderer_frame_id_)
Same comment as in AudioOutputImpl: could the AudioRendererHost own the
AudioOutputStreamImpls? If not, here only AudioOutputStreamImpl should be
called, and it can in turn call AudioRendererHost. And this class should have a
raw pointer to AudioOutputImpl.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:19: // by the AudioOutputClient
to perform operations on audio output streams.
Same here as my comment in AudioOutputImpl.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:37: // AudioOutputStream
interface implementation.
Nit: empty line before.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:47: // The only issue that
could happen is during AudioOutputStreamImpl
How can that be an issue?

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:54: static
AudioOutputStreamImpl* Factory(
Put this function in AudioOutputStreamImpl and name it Create(). (And remove
AudioOutputStreamFactory.)

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:59: #include
"media/mojo/interfaces/audio_output.mojom.h"
Fix order.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:273: std::map<int,
AudioOutputImpl*> audio_output_impls_;
As we talked about offline, let the AudioRenderFrame track the AudioOutputImpl
for that frame. Get the AudioOutputImpl from the AudioRenderFrame instead of
storing it here.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:274: std::map<int,
int> renderer_frame_ids_;
What is this used for?

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.cc:86: Send(new
AudioHostMsg_CloseStream(stream_id));
The AudioOutputIPCImpl should handle sending all the IPC messages, i.e. remove
this function.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.cc:110: auto client =
filter_->audio_output_clients_.find(render_frame_id_);
Should the clients be registered in AudioOutputIPCImpl? They seem not to be used
in AMF.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.cc:110: auto client =
filter_->audio_output_clients_.find(render_frame_id_);
Actually, we shouldn't need to go through the IPC impl at all, but directly from
AudioOutputDevice (or corresponding) to the client. And similar to my suggestion
on the browser side, the frame could perhaps keep track of the AOC, and the AOD
could ask the frame for it.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:51: virtual void CloseStream(int
stream_id);
Add comment describing the function.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:58: // Setter for
|audio_output_client_|.
This is a registration, i.e. multiple clients can be added, change to
RegisterAudioOutputClient().

Also, you need a UnregisterXyz()

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:62:
audio_output_clients_.insert(std::make_pair(id, audio_output_client));
Remove make_pair.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:66: virtual void OnStreamCreated(
They are public because called by a AudioOutputClient, right? Rename to just
StreamCreated().

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:74: virtual void
OnStreamStateChanged(int stream_id,
Leave the OnStreamStateChanged as private. Add a public StreamStateChanged()
function which just calls OnStreamStateChanged(). That way it's clear that the
private "OnXyz" functions are picking up IPC messages, and other public
functions are only called from outside.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_output_client.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_output_client.cc:63:
DCHECK(thread_task_runner_->BelongsToCurrentThread());
No need since there is a conditional post above.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_output_client.cc:78:
AudioMessageFilter::Get()->io_task_runner()->PostTask(
Pass the io task runner in the constructor instead of getting it from the AMF.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_output_client.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_output_client.h:68: void CreateStreamCallback(int
stream_id,
Maybe call this StreamCreated()?

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_output_client.h:90: // called on |io_task_runner_|.
Explain why this is needed. (Why is it needed? Can't everything be done here for
create stream?)

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.h:646:
scoped_refptr<content::AudioOutputClient> audio_output_client_;
Isn't there one AOC per frame? Then it should be per frame and not per render
thread (which is equivalent to render process).

[rchtara, 2016-05-20 15:13:01]

Description was changed from

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 
https://codereview.chromium.org/1896883002

BUG=606707
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Switch stream creation and closing in Chrome audio rendering from IPC to Mojo
This replaces audio stream creation and closing by using  Mojo instead of IPC.
It adds AudioOuputClient and AudioOutputImpl that implement the Mojo client and
service for the Mojo interfaces defined in CL 1896883002.

In future CLs:
  * Mojo will be used for all the other audio output stream operations
(playing, pausing, ..)
  * AudioRendererHost and AudioMessageFilter will be removed and the logic
inside them will be moved to AudioOuputClient and AudioOutputImpl.

This is the content part: The media part is in CL 
https://codereview.chromium.org/1896883002

BUG=606707
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[rchtara, 2016-05-20 15:13:01]

rchtara@chromium.org changed reviewers:
+ dalecurtis@chromium.org, jochen@chromium.org

[rchtara, 2016-05-23 16:38:19]

Hello,

@DaleCurtis: I have done the changes you suggested regrading moving the code
from audio_renderer_host to the audio_output_Impl. Could you provide me please
with more detailed feedback? not a full review, just a feedback, I didn't update
the tests yet, didn't update the comments and still need to clean up staff, so
let's please don't worry about that. (I will update them later: I don't want  to
spend a lot of time working on them if I will have to do a big changes that
break them later)

Thanks a lot
Riadh

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.cc:60:
audio_renderer_host->audio_output_impl_ = service;
On 2016/04/29 18:29:00, mcasas wrote:
> Don't make AudioOutput*Impl friend of AudioRendererHost
> to allow these plugging operations. Make methods at 
> the very least.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.cc:89: std::pair<int,
std::unique_ptr<AudioOutputStreamImpl>>(
On 2016/04/29 18:29:00, mcasas wrote:
> std::make_pair(stream_id, std::move(stream_ptr));

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.h:27: class CONTENT_EXPORT
AudioOutputStreamImpl
On 2016/04/29 18:29:00, mcasas wrote:
> Either spin this class into a file on
> its own, or forward declare around l.94 and
> move definition to audio_output_impl.cc

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/media/audio...
content/browser/media/audio_output_impl.h:27: class CONTENT_EXPORT
AudioOutputStreamImpl
On 2016/05/02 12:12:22, Henrik Grunell wrote:
> On 2016/04/29 18:29:00, mcasas wrote:
> > Either spin this class into a file on
> > its own, or forward declare around l.94 and
> > move definition to audio_output_impl.cc
> 
> Agree, put in its own file.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:80: 0,      //
dwDesiredAccess ignored due to SAME_ACCESS
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Two spaces after "0," instead of aligning with the below comment.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:229:
create_stream_callback_ = callback;
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Put it in the initialization list?

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:303:
base::Bind(&AudioRendererHost::DoCompleteCreation, host_, stream_id_,
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Could |callback_| be used again? If not, what about clearing it after the
> PostTask and a DCHECK before to make sure it's valid?

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:388: // reset.
AudioOutputIPCDelegate is going to need to close the socket handle
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> AudioOutputIPCDelegate is an implementation detail on the client side, so
don't
> mention it here.
> 
> It sounds like the socket handle ownership should be transferred to the
client.
> Why is it closed in |mojo_application_host_| and can we avoid that?
Mojo close it automatically, but not sure if it's possible to transfer the
ownership from Mojo to the renderer

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:791:
mojo::ScopedSharedBufferHandle shared_buffer_handle =
On 2016/05/02 12:12:22, Henrik Grunell wrote:
> Nit: add empty line before this.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:114: friend class
AudioEntry;
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> This should not be needed. Anything that needs to be accessed should go
through
> getter/setter functions.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:224: // Send an error
message to the renderer, then close the stream.
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Change "renderer" to "client", it's more generic.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:289:
media::interfaces::AudioOutputStreamPtr stream_ptr_;
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Add comment.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host_unittest.cc
(right):

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:99: void
OnNotifyStreamCreated(
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> As we talked about offline, this function should either be moved to the test
> class next to the CreateCallback function, or be merged with that function.
> (Depending on how much code is still left after fixing the next comment.) If
you
> keep the function, it could be named something like "VerifyXyz...".

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105:
shared_memory_.reset(new base::SharedMemory(handle, false));
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> As we talked about offline, storing in member variables in the original code
> doesn't makes sense. They are not used anywhere. Remove the member variables
> from the class and instead make sure to test whatever makes sense to test.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:322:
streams_[stream_id] = make_scoped_ptr(
it's not needed anymore.

https://codereview.chromium.org/1930393002/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/1930393002/diff/1/content/content_browser.gyp...
content/content_browser.gypi:81:
'../media/mojo/interfaces/mojo_bindings.gyp:audio_output_mojom_bindings',
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Remove duplicate.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/content_browser.gyp...
content/content_browser.gypi:1752: 'browser/media/audio_output_impl.cc',
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> This is not only for webrtc. Put them in the private_browser_sources target.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/1930393002/diff/1/content/content_renderer.gy...
content/content_renderer.gypi:50: '../mojo/mojo_public.gyp:mojo_cpp_bindings',
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Put them in alphabetical order.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/content_renderer.gy...
content/content_renderer.gypi:285: 'renderer/media/audio_output_client.cc',
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Ditto.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1930393002/diff/1/content/content_tests.gypi#...
content/content_tests.gypi:764: 'browser/media/audio_output_impl_unittest.cc',
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> Same here. It's not only WebRTC.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:57: void
SetAudioOutputClient(content::AudioOutputClient* audio_output_client) {
On 2016/04/29 18:29:00, mcasas wrote:
> s/SetAudioOutputClient/set_audio_output_client/
> 
> https://www.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:65: friend class
AudioOutputClient;
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> This should not be needed. Use setter/getter functions instead.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:106:
scoped_ptr<content::AudioOutputClient> audio_output_client_;
On 2016/04/29 18:29:00, mcasas wrote:
> std::unique_ptr.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:106:
scoped_ptr<content::AudioOutputClient> audio_output_client_;
On 2016/05/02 12:12:23, Henrik Grunell wrote:
> When, after follow-up CLs, this class is removed, who should own the
> AudioOutputClient?
It should be owened by the owner of the AudioMessageFilter, RenderThreadImpl

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_output_client.cc (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:32: if (it.second.get() == key) {
On 2016/04/29 18:29:01, mcasas wrote:
> No {} for one-line bodies

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:35: }
On 2016/04/29 18:29:00, mcasas wrote:
> Nit: Consider std::find_if()

Tried to do that:

bool f(const std::pair<int,
                   std::unique_ptr<media::mojom::AudioOutputStreamPtr>> x) {
  return true;
}
auto it = std::find_if(stream_ids.begin(), stream_ids.end(), f);
But failed as I was calling a delete constructor of
'std::unique_ptr<mojo::InterfacePtr<media::mojom::AudioOutputStream>,
std::default_delete<mojo::InterfacePtr<media::mojom::AudioOutputStream> > >'
Tried to do it with lambda, failed too.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:36: return -1;
On 2016/04/29 18:29:00, mcasas wrote:
> Consider making |kStreamIDNotSet| not 
> public (and/or rename to |kStreamIDNotValid|)
> and using it here.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:51: service_ = 0;
On 2016/04/29 18:29:01, mcasas wrote:
> No need to do this (and is semantically wrong),
> instead check for service_.is_bound() in l.82.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:58: LOG(ERROR) << "Mojo client
connection error";
On 2016/04/29 18:29:01, mcasas wrote:
> s/LOG/DLOG/
> (I think there's a presubmit warning for this).

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:64: base::Unretained(this),
stream_id));
I failed to use WeakPtrFactory. So I will made this refcouned

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:70:
main_thread_task_runner_->PostTask(
On 2016/04/29 18:29:01, mcasas wrote:
> Consider merging with CreateStreamOnMainThread():
> 
> if (!main_thread_task_runner_->BelongsToCurrentThread()) {
>   // PostTask...
>   return;
> }
> DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
> if (!service)  // Should be if (!service.is_bound())
>   return;
> service->CreateStream(...)

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:116: LOG(ERROR) << "Failed to pass
shared memory. Closing: "
On 2016/04/29 18:29:00, mcasas wrote:
> s/LOG/DLOG/

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.cc:167:
streams_[stream_id]->get()->Close();
On 2016/04/29 18:29:01, mcasas wrote:
> Remove get()->, a unique_ptr behaves like a
> pointer.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_output_client.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:43: ScopedAudioOutputStreamPtrMap;
On 2016/04/29 18:29:01, mcasas wrote:
> using ScopedAudioOutputStreamPtrMap = ...
> ?

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:46: // RenderThreadImpl::init and
be bound to RenderThread. This means that all
On 2016/04/29 18:29:01, mcasas wrote:
> init()

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:100: void OnStreamError(int
stream_id);
On 2016/04/29 18:29:01, mcasas wrote:
> Can be private I believe. Check the
> other methods and make private as many
> as possible.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_output_client.h:111:
scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
On 2016/04/29 18:29:01, mcasas wrote:
> const.
> 
> Leave a blank line before the next comment,
> however, it being superfluous, just remove it.

Done.

https://codereview.chromium.org/1930393002/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1930393002/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.h:568: AudioOutputClient*
audio_output_client_;
On 2016/04/29 18:29:01, mcasas wrote:
> Unused, right? Also the include in l.32

Done.

[DaleCurtis, 2016-05-23 20:50:37]

Thanks for merging more into AudioOutputImpl from AudioRendererHost. I think
that is looking better. Since we moved the interfaces to media/ it seems like we
don't need AudioOutputStreamImpl anymore? I.e. that should be merged with
AudioOutputClient?

https://codereview.chromium.org/1930393002/diff/120001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/120001/content/browser/media/...
content/browser/media/audio_output_impl.h:48: // Called by
AudioRendererHost::DoCompleteCreation to create the streams.
This should just be CreateStream() StreamFactory isn't helpful.

[rchtara, 2016-05-24 16:13:52]

Great. thanks for the feedback:
The AudioOutputStreamImpl is implementing the AudioOutputStream interface. It's
going to run in the browser.
The AudioOutputClient will run in the renderer. It will allow creating 
AudioOutputStreamImpl in the browser via mojo and using the AudioOutputImpl in
the browser).

We have many AudioOutputStreamImpl per frame however we have just one
AudioOutputImpl and AudioOutputClient.
So, could you please explain how do you suggest to split them?

Later we are going to use AudioOutputStreamImpl to play, pause and set volume.
And for that we are going to call 
play on the AudioOutputStreamPtr for the  audio_message_filter 
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

which  will trigger a call for  the AudioOutputStreamImpl::play which will call
AudioRendererHost::OnPlayStream and OnPlayStream will call 
entry->controller()->Play();
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
The exact things happens for pause and setvolume.
So if we want to simplify that, to get rid of the stream_id (from most places
like here 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...)
and nicely merge the AudioOutputImpl with AudioRendererHost, I think we have to
merge  AudioEntry and AudioOutputStreamImpl (or at least let the
AudioOutputStreamImpl own the AudioEntry)  when merging the AudioOutputImpl with
AudioRendererHost: we need somehow to access the AudioEntry from the
AudioOutputStreamImpl.
They share many functionalities and merging them will make things more simple.  
(this out of the scope of this cl)

Thanks a lot again for your feedback.

[rchtara, 2016-05-24 17:25:55]

https://codereview.chromium.org/1930393002/diff/120001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/120001/content/browser/media/...
content/browser/media/audio_output_impl.h:48: // Called by
AudioRendererHost::DoCompleteCreation to create the streams.
On 2016/05/23 20:50:37, DaleCurtis wrote:
> This should just be CreateStream() StreamFactory isn't helpful.

Unfortunately, here the socket and shared memory required to be returned using
the callback are not created yet. As the AudioOutputImpl::CreateStream will call
asynchronously AudioRendererHost::DoCompleteCreation  (it includes calling
AudioOutputController::Create which goes to the  controller->message_loop_)
And that's when everything is ready for the stream creation, and this why
StreamFactory is needed.
So we need a way to store the callback, to use it
AudioOutputImpl::StreamFactory.
We could pass it as an argument to AudioRendererHost::OnCreateStream and pass it
around from a function to another and get it back when we are in the
DoCompleteCreation  then send it to AudioOutputImpl::StreamFactory, but we need
to change a lot of functions: (see the functions that are going to be changed, I
didn't  include the ones related to AudioOutputController::OnDeviceChange.)

AudioRendererHost::OnCreateStream
 AudioRendererHost::DoCreateStream
AudioRendererHost::AudioEntry::AudioEntry()

AudioOutputController::OnDeviceChange()  and  AudioOutputController::Create

 AudioOutputController::DoCreate 
AudioEntry::OnCreated() =>
 AudioRendererHost::DoCompleteCreation 
AudioOutputImpl::StreamFactory

So, I think storing the callback in a map is the best option for now.

[rchtara, 2016-05-25 12:39:09]

rchtara@chromium.org changed reviewers:
- jochen@chromium.org

[rchtara, 2016-05-25 14:34:23]

rchtara@chromium.org changed reviewers:
+ nasko@chromium.org

[rchtara, 2016-05-25 14:34:24]

nasko@chromium.org: Hello, Could you Please review changes in 

  content/browser/frame_host/render_frame_host_impl.cc
  content/browser/frame_host/render_frame_host_impl.h
  content/browser/renderer_host/render_process_host_impl.h
  content/common/BUILD.gn
  content/public/browser/render_frame_host.h
  content/public/browser/render_process_host.h
  content/renderer/render_frame_impl.cc
  content/renderer/render_thread_impl.h
  content/test/BUILD.gn

[nasko, 2016-05-25 20:50:43]

Few high level notes:
* The implementation of the Mojo interface must be committed at the same time as
the mojom file, otherwise you are bypassing the security review.
* This CL only touches code in content/, so it should not be adding any
content/public/ APIs at all.
* There are various style guide errors, so I would suggest a reread to refresh.
Also, using "git cl format" is highly encouraged.
* Overall the CL does not look ready for content/ level review or security
review. Please have the core media owners review the CL and ping me when it is
ready for further review.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:122: void
SetAudioOutputImpl(AudioOutputImpl* audio_output_impl) override;
No spaces between methods that are overriden.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:772: AudioOutputImpl*
audio_output_impl_;
Add a comment on what this new member is.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:20:
base::SyncSocket::TransitDescriptor DuplicateSocket(
Empty line between namespace and the code.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:41: }
Empty line between the two } and a // namespace ending comment.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:56: it !=
create_stream_callbacks_.end(); ++it)
Multiline loops need to have {}.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:62: void
AudioOutputImpl::CreateService(RenderProcessHost* process,
The comment in the header was saying this object is created and lives on the IO
thread and no mention about UI thread. However, this method expects to be called
on the UI thread. Please clarify this in the header file.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:78: if (request.is_pending()) {
Is it safe to call this on other threads if the request isn't pending?

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:102: if (callback ==
create_stream_callbacks_.end()) {
Single line if statements don't need {}.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:111:
DCHECK(stream_impls_.insert(std::make_pair(stream_id, std::move(stream)))
This statement will never execute in release build. You need to move the call
outside of the DCHECK, capture the return value, then DCHECK the return value.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:117:
mojo::WrapSharedMemoryHandle(shared_memory_handle,
Would WrapSharedMemoryHandle work successfully if the DuplicateHandle call
failed? There is no check for error condition.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:121: // reset. The renderer is going
to need to close the socket handle
nit: s/renderer/renderer process/

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:137: callback->second.Run(stream_id,
std::move(stream_ptr),
If there is always a pointer to the stream, why is the mojom file annotated as
optional? Same for the other two params.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:23: // This class implements
AudioOutput Mojo interface. It will be used
nit: "will" or "is"?

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:25: // AudioOutputClient is going to
perform operations remotely and Mojo will
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.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:41: // Initialized Mojo service on the
IO thread. The service is bound to the
Empty line before comment.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:46: // thread. There is a plan to
enable these dependencies to run on another
nit: Put this plan on a new line with TODO(): prefix and a bug number for it.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:47: // thread and move the Mojo
service to this thread as IO thread is very
nit: Overuse of the word "thread" and the comment becomes hard to read.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:49: static void
CreateServiceOnIOThread(RenderProcessHost* process_,
No underscores on parameters, those are reserved for member variables.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:60: // Send an error message to the
renderer, then close the stream.
Empty line before comment.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:70: static void Disconnect(int
process_id, int render_frame_id);
Add comment describing what the method does.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:78: int render_frame_id_;
Instead of these two variables, why not keep a pointer to the RenderFrameHost
this object is associated with?

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:33: int get_stream_id() const {
return stream_id_; }
No need for a get_ prefix, as the style guide explicitly states that simple
accessors are named the same as the variable name they expose. In this case "int
stream_id() ..."

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:147: AudioOutputImpl*
get_audio_output_impl() { return audio_output_impl_; }
No get_ prefix. The _impl suffix is also optional.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:246: // |streaa_id|
has started.
stream?

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:255: std::map<int,
int> renderer_frame_ids_;
This is never used.

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:27: class AudioOutputImpl;
This is a violation of the content API. No Impl classes are exposed outside of
content/.

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:54: virtual AudioOutputImpl*
GetAudioOutputImpl() = 0;
These two methods are not used anywhere, please remove them from the public API.

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:56: virtual void
SetAudioOutputImpl(AudioOutputImpl* audio_output_impl) = 0;
Why do we have a set method on the public interface? You also cannot have Impl
classes exposed outside of content, so this is incorrect.

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
content/renderer/media/audio_message_filter.h:59: void set_audio_output_client(
Needs to be CamelCased, as it isn't a simple assignment.

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
File content/renderer/media/audio_output_client.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
content/renderer/media/audio_output_client.h:46: // RenderThreadImpl::init() and
be bound to RenderThread. This means that all
nit: Init.

https://codereview.chromium.org/1930393002/diff/180001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/1930393002/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:72: "//media/mojo/interfaces",
Why is this dependency needed? From content/common/BUILD.gn:

# Code in content but outside of the
# content component (content/test or content/shell) should link to this
# directly.

https://codereview.chromium.org/1930393002/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:465: "//media/mojo/interfaces",
Same question as above.

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

https://codereview.chromium.org/1930393002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1938:
GetServiceRegistry()->AddService(
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> run git cl format?  I'm guessing it will delete the extra whitespace below too
> but in case it doesn't, please fix.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:19: LOG(ERROR) << "AudioOutputImpl";
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> ?

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:20: factory_ =
base::Bind(AudioOutputStreamFactory::Factory);
On 2016/05/20 13:35:39, Henrik Grunell wrote:
> I suppose |factory_| is needed for being modified in tests. Instead, pass the
> factory in the constructor.

it's removed: not needed for the new tests

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:24: LOG(ERROR) << "~AudioOutputImpl";
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> please remove all of these

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:46:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> why would we only be on the io thread if the request is pending? (seems like
> there might be a bug here)

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:46:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/05/20 13:35:39, Henrik Grunell wrote:
> Why can it be called on another thread if not pending? Should be just the
> DCHECK?

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:52:
process->audio_renderer_host()->set_audio_output_impl(render_frame_id, service);
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> please run git cl format

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:52:
process->audio_renderer_host()->set_audio_output_impl(render_frame_id, service);
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> please run git cl format

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:52:
process->audio_renderer_host()->set_audio_output_impl(render_frame_id, service);
On 2016/05/20 13:35:39, Henrik Grunell wrote:
> This is what we talked about offline, right? It won't work since this mojo
> service when existing per frame will be created before the AudioRendererHost.

yes. exactly

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:64:
media::mojom::AudioOutputStreamPtr AudioOutputImpl::StreamFactory(
On 2016/05/20 13:35:39, Henrik Grunell wrote:
> It seems to be unnecessarily complicated to call the ARH above, which then
calls
> back here to create the stream. Is it possible to move all the code here to
ARH?
> Or perhaps create the AOSI in CreateStream() and pass it to ARH directly?

Yes, but dale wants to removes code from the ARH. And doing so will make things
even more complicated

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:78: DCHECK(ret.second);
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> do you get a build error for official builds?  (ret becomes an unused I didn't
tried but I will get rid of it.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.cc:87: return false;
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> please just do:
> 
> return stream_impls_.erase(stream_id) != 0;

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:18: // by the AudioOutputClient to
perform operations on audio output streams.
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> This isn't really correct. AudioOutputClient doesn't "use" this class as in
> calls functions on it. You could instead write that it's connected to the
> AudioOutputClient.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:25: explicit
AudioOutputImpl(RenderProcessHost* process_,
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> No trailing _

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:36: // Initialized Mojo service on the
IO thread. The service is bound to the
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> add an empty line above this one.
> 
> Should the first sentence be "Returns an initialized ..."?

It doesn't return anything. it just initialize service

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:36: // Initialized Mojo service on the
IO thread. The service is bound to the
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Empty line before comment.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:41: // thread. There is a plan to
enable these dependencies to run on another
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> I don't think there is a plan for doing that here on the browser side. On the
> render side it does. So, you should remove that part of the comment.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:45: RenderProcessHost* process_,
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> process (no underscore for params)

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:47: media::mojom::AudioOutputRequest
request);
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> passed by value on purpose?

yes, it's going to be initialized in RenderFrameHostImpl::RegisterMojoServices()
like this:
 GetServiceRegistry()->AddService(
       base::Bind(&AudioOutputImpl::CreateService,
                  process_, routing_id_));
So we have to give it request by value otherwise it doesn't work

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:63: // AudioOutput interface
implementation.
On 2016/05/20 13:35:39, Henrik Grunell wrote:
> Empty line before comment.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:71: std::map<int,
std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> can you add information about this map? (maps from ? to AudioOutputStreamImpl)

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:76: AudioRendererHost*
audio_renderer_host)>
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> can you create a typedef for this?

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:77: factory_;
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Indentation seems odd. Run git cl format.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:78:
DISALLOW_COPY_AND_ASSIGN(AudioOutputImpl);
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Empty line before.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.cc:27: void
AudioOutputStreamImpl::Close() {
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Do we do the same as here for all AudioOutputStreamImpls if the connection is
> closed?

yes

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.cc:30:
audio_renderer_host_->get_audio_output_impl(renderer_frame_id_)
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Same comment as in AudioOutputImpl: could the AudioRendererHost own the
> AudioOutputStreamImpls? If not, here only AudioOutputStreamImpl should be
> called, and it can in turn call AudioRendererHost. And this class should have
a
> raw pointer to AudioOutputImpl.

discussed offline about this

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:37: // AudioOutputStream
interface implementation.
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Nit: empty line before.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:41: int stream_id_;
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> can this be made const?

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:42: int renderer_frame_id_;
On 2016/05/20 13:26:13, tommi-chrömium wrote:
> and this?

that one is not needed

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:47: // The only issue that
could happen is during AudioOutputStreamImpl
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> How can that be an issue?
outdated comment

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:49: AudioRendererHost*
audio_renderer_host_;
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> and this?
> 
> (i.e. AudioRendererHost* const audio_renderer_host_)

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:52: class CONTENT_EXPORT
AudioOutputStreamFactory {
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> mark the default ctor as delete?
> 
> Why does this need to be a class btw?

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

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:54: static
AudioOutputStreamImpl* Factory(
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Put this function in AudioOutputStreamImpl and name it Create(). (And remove
> AudioOutputStreamFactory.)

not needed anymore

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:59: #include
"media/mojo/interfaces/audio_output.mojom.h"
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Fix order.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:273: std::map<int,
AudioOutputImpl*> audio_output_impls_;
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> As we talked about offline, let the AudioRenderFrame track the AudioOutputImpl
> for that frame. Get the AudioOutputImpl from the AudioRenderFrame instead of
> storing it here.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:274: std::map<int,
int> renderer_frame_ids_;
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> What is this used for?

they  are removed

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.cc:86: Send(new
AudioHostMsg_CloseStream(stream_id));
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> The AudioOutputIPCImpl should handle sending all the IPC messages, i.e. remove
> this function.

this is going to be used for closing streams that are  authorized but not
created

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:58: // Setter for
|audio_output_client_|.
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> This is a registration, i.e. multiple clients can be added, change to
> RegisterAudioOutputClient().
> 
> Also, you need a UnregisterXyz()
we dont , there owned by the AMF so they are going to be released in AMF
destruction.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:66: virtual void OnStreamCreated(
On 2016/05/20 13:35:41, Henrik Grunell wrote:
> They are public because called by a AudioOutputClient, right? Rename to just
> StreamCreated().

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_message_filter.h:74: virtual void
OnStreamStateChanged(int stream_id,
On 2016/05/20 13:35:40, Henrik Grunell wrote:
> Leave the OnStreamStateChanged as private. Add a public StreamStateChanged()
> function which just calls OnStreamStateChanged(). That way it's clear that the
> private "OnXyz" functions are picking up IPC messages, and other public
> functions are only called from outside.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
File content/renderer/media/audio_output_client.cc (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_output_client.cc:63:
DCHECK(thread_task_runner_->BelongsToCurrentThread());
On 2016/05/20 13:35:41, Henrik Grunell wrote:
> No need since there is a conditional post above.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/media...
content/renderer/media/audio_output_client.cc:78:
AudioMessageFilter::Get()->io_task_runner()->PostTask(
On 2016/05/20 13:35:41, Henrik Grunell wrote:
> Pass the io task runner in the constructor instead of getting it from the AMF.

Done.

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.h:646:
scoped_refptr<content::AudioOutputClient> audio_output_client_;
On 2016/05/20 13:35:41, Henrik Grunell wrote:
> Isn't there one AOC per frame? Then it should be per frame and not per render
> thread (which is equivalent to render process).

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:122: void
SetAudioOutputImpl(AudioOutputImpl* audio_output_impl) override;
On 2016/05/25 20:50:41, nasko (slow) wrote:
> No spaces between methods that are overriden.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:772: AudioOutputImpl*
audio_output_impl_;
On 2016/05/25 20:50:41, nasko (slow) wrote:
> Add a comment on what this new member is.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:20:
base::SyncSocket::TransitDescriptor DuplicateSocket(
On 2016/05/25 20:50:41, nasko (slow) wrote:
> Empty line between namespace and the code.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:41: }
On 2016/05/25 20:50:41, nasko (slow) wrote:
> Empty line between the two } and a // namespace ending comment.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:56: it !=
create_stream_callbacks_.end(); ++it)
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Multiline loops need to have {}.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:62: void
AudioOutputImpl::CreateService(RenderProcessHost* process,
On 2016/05/25 20:50:41, nasko (slow) wrote:
> The comment in the header was saying this object is created and lives on the
IO
> thread and no mention about UI thread. However, this method expects to be
called
> on the UI thread. Please clarify this in the header file.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:78: if (request.is_pending()) {
On 2016/05/25 20:50:41, nasko (slow) wrote:
> Is it safe to call this on other threads if the request isn't pending?

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:102: if (callback ==
create_stream_callbacks_.end()) {
On 2016/05/25 20:50:41, nasko (slow) wrote:
> Single line if statements don't need {}.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:111:
DCHECK(stream_impls_.insert(std::make_pair(stream_id, std::move(stream)))
On 2016/05/25 20:50:41, nasko (slow) wrote:
> This statement will never execute in release build. You need to move the call
> outside of the DCHECK, capture the return value, then DCHECK the return value.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:117:
mojo::WrapSharedMemoryHandle(shared_memory_handle,
On 2016/05/25 20:50:41, nasko (slow) wrote:
> Would WrapSharedMemoryHandle work successfully if the DuplicateHandle call
> failed? There is no check for error condition.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:121: // reset. The renderer is going
to need to close the socket handle
On 2016/05/25 20:50:41, nasko (slow) wrote:
> nit: s/renderer/renderer process/

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:137: callback->second.Run(stream_id,
std::move(stream_ptr),
On 2016/05/25 20:50:41, nasko (slow) wrote:
> If there is always a pointer to the stream, why is the mojom file annotated as
> optional? Same for the other two params.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:23: // This class implements
AudioOutput Mojo interface. It will be used
On 2016/05/25 20:50:42, nasko (slow) wrote:
> nit: "will" or "is"?

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:25: // AudioOutputClient is going to
perform operations remotely and Mojo will
On 2016/05/25 20:50:42, nasko (slow) wrote:
> 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.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:41: // Initialized Mojo service on the
IO thread. The service is bound to the
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Empty line before comment.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:49: static void
CreateServiceOnIOThread(RenderProcessHost* process_,
On 2016/05/25 20:50:42, nasko (slow) wrote:
> No underscores on parameters, those are reserved for member variables.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:60: // Send an error message to the
renderer, then close the stream.
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Empty line before comment.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:78: int render_frame_id_;
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Instead of these two variables, why not keep a pointer to the RenderFrameHost
> this object is associated with?

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_stream_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_stream_impl.h:33: int get_stream_id() const {
return stream_id_; }
On 2016/05/25 20:50:42, nasko (slow) wrote:
> No need for a get_ prefix, as the style guide explicitly states that simple
> accessors are named the same as the variable name they expose. In this case
"int
> stream_id() ..."

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:147: AudioOutputImpl*
get_audio_output_impl() { return audio_output_impl_; }
On 2016/05/25 20:50:42, nasko (slow) wrote:
> No get_ prefix. The _impl suffix is also optional.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:246: // |streaa_id|
has started.
On 2016/05/25 20:50:42, nasko (slow) wrote:
> stream?

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:255: std::map<int,
int> renderer_frame_ids_;
On 2016/05/25 20:50:42, nasko (slow) wrote:
> This is never used.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:27: class AudioOutputImpl;
On 2016/05/25 20:50:42, nasko (slow) wrote:
> This is a violation of the content API. No Impl classes are exposed outside of
> content/.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:54: virtual AudioOutputImpl*
GetAudioOutputImpl() = 0;
On 2016/05/25 20:50:42, nasko (slow) wrote:
> These two methods are not used anywhere, please remove them from the public
API.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:56: virtual void
SetAudioOutputImpl(AudioOutputImpl* audio_output_impl) = 0;
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Why do we have a set method on the public interface? You also cannot have Impl
> classes exposed outside of content, so this is incorrect.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
content/renderer/media/audio_message_filter.h:59: void set_audio_output_client(
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Needs to be CamelCased, as it isn't a simple assignment.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
File content/renderer/media/audio_output_client.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/renderer/media...
content/renderer/media/audio_output_client.h:46: // RenderThreadImpl::init() and
be bound to RenderThread. This means that all
On 2016/05/25 20:50:42, nasko (slow) wrote:
> nit: Init.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/1930393002/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:72: "//media/mojo/interfaces",
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Why is this dependency needed? From content/common/BUILD.gn:
> 
> # Code in content but outside of the
> # content component (content/test or content/shell) should link to this
> # directly.

Done.

https://codereview.chromium.org/1930393002/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:465: "//media/mojo/interfaces",
On 2016/05/25 20:50:43, nasko (slow) wrote:
> Same question as above.

Done.

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

Hello,
Thanks @nasko for the review.
@DaleCurtis: could you please have a look but just ignore the tests (i will
update them later).
If you are going to ask some mojo experts to have a look to the patch, it would
be great if you could  do that as soon as possible too.
(I still have 5 days left, and would like to land the patch before leaving.)
Thanks a lot guys for you help and reviews.
Riahd

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.h:70: static void Disconnect(int
process_id, int render_frame_id);
On 2016/05/25 20:50:42, nasko (slow) wrote:
> Add comment describing what the method does.

Done.

[rchtara, 2016-05-27 16:25:32]

Hello,
Thanks @nasko for the review.
@DaleCurtis: could you please have a look but just ignore the tests (i will
update them later).
If you are going to ask some mojo experts to have a look to the patch, it would
be great if you could  do that as soon as possible too.
(I still have 5 days left, and would like to land the patch before leaving.)
Thanks a lot guys for you help and reviews.
Riadh

[nasko, 2016-05-27 22:46:34]

On 2016/05/27 16:25:32, rchtara wrote:
> Hello,
> Thanks @nasko for the review.
> @DaleCurtis: could you please have a look but just ignore the tests (i will
> update them later).
> If you are going to ask some mojo experts to have a look to the patch, it
would
> be great if you could  do that as soon as possible too.
> (I still have 5 days left, and would like to land the patch before leaving.)
> Thanks a lot guys for you help and reviews.
> Riadh

I appreciate the desire to wrap this up before leaving, but Monday is a public
holiday in the US, which leaves 2 days more. I'm afraid this is a bit too short
timeframe to ensure this CL is in shape to be committed.

[nasko, 2016-05-27 22:52:00]

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/180001/content/browser/media/...
content/browser/media/audio_output_impl.cc:137: callback->second.Run(stream_id,
std::move(stream_ptr),
On 2016/05/27 15:24:38, rchtara wrote:
> On 2016/05/25 20:50:41, nasko (slow) wrote:
> > If there is always a pointer to the stream, why is the mojom file annotated
as
> > optional? Same for the other two params.
> 
> Done.

What does "Done" mean here? I don't see a change to the mojom file, nor to this
code. How was the initial question addressed as done?

[Henrik Grunell, 2016-05-30 12:44:56]

Just a random new comment + an old one I hadn't sent out yet.

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
File content/browser/media/audio_output_impl.h (right):

https://codereview.chromium.org/1930393002/diff/100001/content/browser/media/...
content/browser/media/audio_output_impl.h:76: AudioRendererHost*
audio_renderer_host)>
On 2016/05/20 13:26:12, tommi-chrömium wrote:
> can you create a typedef for this?

Or preferable "using".

https://codereview.chromium.org/1930393002/diff/350001/content/browser/media/...
File content/browser/media/audio_output_impl.cc (right):

https://codereview.chromium.org/1930393002/diff/350001/content/browser/media/...
content/browser/media/audio_output_impl.cc:195: audio_renderer_host_ = host;
If we should surely be able to initialize the ARH here (I think that should be
the case), just set the member directly and CHECK() that it's non-null.

Also, if already set, just return before doing anything else.

[rchtara, 2016-05-31 17:09:22]

On 2016/05/27 22:46:34, nasko (slow) wrote:
> On 2016/05/27 16:25:32, rchtara wrote:
> > Hello,
> > Thanks @nasko for the review.
> > @DaleCurtis: could you please have a look but just ignore the tests (i will
> > update them later).
> > If you are going to ask some mojo experts to have a look to the patch, it
> would
> > be great if you could  do that as soon as possible too.
> > (I still have 5 days left, and would like to land the patch before leaving.)
> > Thanks a lot guys for you help and reviews.
> > Riadh
> 
> I appreciate the desire to wrap this up before leaving, but Monday is a public
> holiday in the US, which leaves 2 days more. I'm afraid this is a bit too
short
> timeframe to ensure this CL is in shape to be committed.

Hello,
@nasko: Good point.
We discussed about landing the cl before I leave in our team meeting and tommi@
and grunell@ agreed with you and thought it s better to go carefully with this
cl than to risk breaking something in chrome for example: So, even if we don't
land it before I will leave, it's not a big problem after all, I can just give
it to someone else so he can continue working on it.
Thanks a lot
Riadh