Convert some audio code to OnceCallback.

Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code and save some refcouting. This also serves as documentation  on which callbacks are only called once, which is nice.

Also took care of lint issues.

There are input code and also some general device management/permissions code which needs converting, but I feel this CL is big enough.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2869733005
Cr-Commit-Position: refs/heads/master@{#472059}
Committed: https://chromium.googlesource.com/chromium/src/+/fd2012282b535b4fda0b1d581e0674f295dcd625

[Max Morin, 2017-05-09 14:59:52]

Description was changed from

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting.

BUG=714018
==========

to

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-05-09 15:19:05]

Description was changed from

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation on which callbacks
are ran once.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-05-09 15:29:04]

Description was changed from

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation on which callbacks
are ran once.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation  on which callbacks
are only called once, which is nice.

There are input code and also some general device management/permissions code
which needs converting, but I feel this CL is big enough.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-05-09 15:57:46]

maxmorin@chromium.org changed reviewers:
+ tzik@chromium.org

[Max Morin, 2017-05-09 15:57:49]

Tzik: Do you have some advice on how to have both a Once and Repeating version
of BindToCurrentLoop without copying all the code? I didn't manage, but that may
be because I'm bad at templates :). (No need to review the rest, as it'll need a
media owners review anyways).

[Max Morin, 2017-05-09 15:58:38]

Description was changed from

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation  on which callbacks
are only called once, which is nice.

There are input code and also some general device management/permissions code
which needs converting, but I feel this CL is big enough.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation  on which callbacks
are only called once, which is nice.

Also took care of lint issues.

There are input code and also some general device management/permissions code
which needs converting, but I feel this CL is big enough.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-05-09 15:58:47]

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

[commit-bot: I haz the power, 2017-05-09 15:59:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Max Morin, 2017-05-09 16:15:11]

Patchset #1 (id:1) has been deleted

[Max Morin, 2017-05-09 16:15:23]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-09 16:49:45]

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

[commit-bot: I haz the power, 2017-05-09 16:49:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Max Morin, 2017-05-09 17:18:20]

On 2017/05/09 15:57:49, Max Morin wrote:
> Tzik: Do you have some advice on how to have both a Once and Repeating version
> of BindToCurrentLoop without copying all the code? I didn't manage, but that
may
> be because I'm bad at templates :). (No need to review the rest, as it'll need
a
> media owners review anyways).

Oh, I see my implementation also causes issues because repeatable callbacks are
converted into OnceCallbacks, using the OnceCallback version when
RepeatingCallback was intended. Hmmm.

[Max Morin, 2017-05-09 17:31:12]

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

[commit-bot: I haz the power, 2017-05-09 17:32:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Max Morin, 2017-05-09 17:40:02]

On 2017/05/09 17:18:20, Max Morin wrote:
> On 2017/05/09 15:57:49, Max Morin wrote:
> > Tzik: Do you have some advice on how to have both a Once and Repeating
version
> > of BindToCurrentLoop without copying all the code? I didn't manage, but that
> may
> > be because I'm bad at templates :). (No need to review the rest, as it'll
need
> a
> > media owners review anyways).
> 
> Oh, I see my implementation also causes issues because repeatable callbacks
are
> converted into OnceCallbacks, using the OnceCallback version when
> RepeatingCallback was intended. Hmmm.

These issues were actually caused by overzealously moving the callback even in
the repeating case :). Fixed it in the latest PS.

[commit-bot: I haz the power, 2017-05-09 21:44:13]

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

[commit-bot: I haz the power, 2017-05-09 21:44:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2017-05-10 06:49:19]

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc
(right):

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:310:
(base::BindOnce(
nit: extra parenthes?

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:336:
(base::BindOnce(
nitto

[o1ka, 2017-05-10 08:03:02]

Nice cleanup!
Drive-by:

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_output_authorization_handler.cc
(right):

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_output_authorization_handler.cc:161:
base::Bind(&AudioOutputAuthorizationHandler::TranslateDeviceID,
Once?

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:176:
std::move(request)));
base::Passed?

[Max Morin, 2017-05-10 10:39:59]

Thanks for the comments. Tzik: any advice for BindToCurrentLoop?

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_output_authorization_handler.cc
(right):

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_output_authorization_handler.cc:161:
base::Bind(&AudioOutputAuthorizationHandler::TranslateDeviceID,
On 2017/05/10 08:03:02, o1ka wrote:
> Once?

This would require messing around with MediaDevicesManager. Maybe for another
CL, but that stuff is scary :).

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc
(right):

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:310:
(base::BindOnce(
On 2017/05/10 06:49:18, tzik wrote:
> nit: extra parenthes?

Done.

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:336:
(base::BindOnce(
On 2017/05/10 06:49:18, tzik wrote:
> nitto

Done.

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:176:
std::move(request)));
On 2017/05/10 08:03:02, o1ka wrote:
> base::Passed?

They're the same for OnceCallbacks. I prefer "move", since "Passed" can be used
with repeating callbacks, where it's dangerous (since calling a callback with a
Passed parameter several times will CHECK). In this sense, I think Passed is
unnecessary when we have OnceCallback. "move" is also more familiar for people
new to Chromium, which is nice.

[tzik, 2017-05-10 13:14:06]

On 2017/05/10 10:39:59, Max Morin wrote:
> Thanks for the comments. Tzik: any advice for BindToCurrentLoop?

That looks enough good to me. We are probably able to consolidate these impl by
using the callback type as the helper class parameter like
https://codereview.chromium.org/2874003002/, not sure it's more readable.

> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/media/audio_output_authorization_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
> content/browser/renderer_host/media/audio_output_authorization_handler.cc:161:
> base::Bind(&AudioOutputAuthorizationHandler::TranslateDeviceID,
> On 2017/05/10 08:03:02, o1ka wrote:
> > Once?
> 
> This would require messing around with MediaDevicesManager. Maybe for another
> CL, but that stuff is scary :).
> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
> File
>
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
>
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:310:
> (base::BindOnce(
> On 2017/05/10 06:49:18, tzik wrote:
> > nit: extra parenthes?
> 
> Done.
> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
>
content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:336:
> (base::BindOnce(
> On 2017/05/10 06:49:18, tzik wrote:
> > nitto
> 
> Done.
> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):
> 
>
https://codereview.chromium.org/2869733005/diff/60001/content/browser/rendere...
> content/browser/renderer_host/media/media_stream_ui_proxy.cc:176:
> std::move(request)));
> On 2017/05/10 08:03:02, o1ka wrote:
> > base::Passed?
> 
> They're the same for OnceCallbacks. I prefer "move", since "Passed" can be
used
> with repeating callbacks, where it's dangerous (since calling a callback with
a
> Passed parameter several times will CHECK). In this sense, I think Passed is
> unnecessary when we have OnceCallback. "move" is also more familiar for people
> new to Chromium, which is nice.

[Max Morin, 2017-05-12 16:45:23]

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

[commit-bot: I haz the power, 2017-05-12 16:46:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 18:45:45]

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

[commit-bot: I haz the power, 2017-05-12 18:45:46]

Dry run: This issue passed the CQ dry run.

[Max Morin, 2017-05-15 07:47:55]

maxmorin@chromium.org changed reviewers:
+ xhwang@chromium.org

[Max Morin, 2017-05-15 07:47:55]

Xhwang: PTAL.

[tzik, 2017-05-15 08:37:09]

lgtm

[xhwang, 2017-05-15 17:50:55]

lg in general with two nits

https://codereview.chromium.org/2869733005/diff/120001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_output_stream_provider.cc (right):

https://codereview.chromium.org/2869733005/diff/120001/media/mojo/services/mo...
media/mojo/services/mojo_audio_output_stream_provider.cc:20:
&MojoAudioOutputStreamProvider::OnError, base::Unretained(this)));
Can you add a comment why this base::Unretained(this) is safe?

https://codereview.chromium.org/2869733005/diff/120001/media/mojo/services/mo...
media/mojo/services/mojo_audio_output_stream_provider.cc:46:
base::Unretained(this)));
ditto

[Max Morin, 2017-05-16 07:14:38]

The CQ bit was checked by maxmorin@chromium.org

[Max Morin, 2017-05-16 07:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2869733005/#ps140001
(title: "Rebase, comments on unretained.")

[commit-bot: I haz the power, 2017-05-16 07:14:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 07:23:09]

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

[commit-bot: I haz the power, 2017-05-16 07:23:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Max Morin, 2017-05-16 07:27:34]

https://codereview.chromium.org/2869733005/diff/120001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_output_stream_provider.cc (right):

https://codereview.chromium.org/2869733005/diff/120001/media/mojo/services/mo...
media/mojo/services/mojo_audio_output_stream_provider.cc:20:
&MojoAudioOutputStreamProvider::OnError, base::Unretained(this)));
On 2017/05/15 17:50:55, xhwang wrote:
> Can you add a comment why this base::Unretained(this) is safe?

Done.

https://codereview.chromium.org/2869733005/diff/120001/media/mojo/services/mo...
media/mojo/services/mojo_audio_output_stream_provider.cc:46:
base::Unretained(this)));
On 2017/05/15 17:50:55, xhwang wrote:
> ditto

Done.

[xhwang, 2017-05-16 07:56:58]

lgtm

[Max Morin, 2017-05-16 07:58:25]

Thanks!

[Max Morin, 2017-05-16 08:36:17]

The CQ bit was checked by maxmorin@chromium.org

[commit-bot: I haz the power, 2017-05-16 08:36:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 10:30:58]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494923777937500,
"parent_rev": "d967040c162c7fb64299cd623f246da739962c38", "commit_rev":
"fd2012282b535b4fda0b1d581e0674f295dcd625"}

[commit-bot: I haz the power, 2017-05-16 10:31:10]

Description was changed from

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation  on which callbacks
are only called once, which is nice.

Also took care of lint issues.

There are input code and also some general device management/permissions code
which needs converting, but I feel this CL is big enough.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Convert some audio code to OnceCallback.

Since PostTaskAndReply and friends are Onceified now, we can convert some code
and save some refcouting. This also serves as documentation  on which callbacks
are only called once, which is nice.

Also took care of lint issues.

There are input code and also some general device management/permissions code
which needs converting, but I feel this CL is big enough.

BUG=714018
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2869733005
Cr-Commit-Position: refs/heads/master@{#472059}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd2012282b535b4fda0b1d581e06...
==========

[commit-bot: I haz the power, 2017-05-16 10:31:11]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fd2012282b535b4fda0b1d581e06...