Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(63)

Issue 2900043002: Added initial set of logging for AudioOutputController. (Closed)

Created:
3 years, 7 months ago by ossu-chromium
Modified:
3 years, 6 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, xjz+watch_chromium.org, mfoltz+watch_chromium.org, miu+watch_chromium.org, Henrik Grunell
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Added initial set of logging for AudioOutputController. BUG=chromium:714119 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/2900043002 Cr-Commit-Position: refs/heads/master@{#476257} Committed: https://chromium.googlesource.com/chromium/src/+/9e1250518374b610d8a23e8b87c44a717a8c919a

Patch Set 1 #

Patch Set 2 : Improved printouts a bit using StringPrintf. #

Total comments: 3

Patch Set 3 : Turned DoLog static, so we can log things even when delegate_ has disappeared. #

Total comments: 10

Patch Set 4 : Bind -> BindOnce; DoLog now a lambda; removed extra blank lines. #

Total comments: 6

Patch Set 5 : Rebase #

Patch Set 6 : Put logging directly in OnLog. Provide stream_id explicitly. #

Total comments: 1

Patch Set 7 : impliciticity #

Unified diffs Side-by-side diffs Delta from patch set Stats (+48 lines, -6 lines) Patch
M content/browser/renderer_host/media/audio_output_delegate_impl.cc View 1 2 3 4 5 6 4 chunks +19 lines, -6 lines 0 comments Download
M media/audio/audio_output_controller.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M media/audio/audio_output_controller.cc View 1 2 3 4 6 chunks +27 lines, -0 lines 0 comments Download
M media/audio/audio_output_controller_unittest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 38 (21 generated)
ossu-chromium
I've put in some initial logging for AudioOutputController and it SeemsToWork(tm), but my local Chrome ...
3 years, 7 months ago (2017-05-24 17:19:41 UTC) #7
Max Morin
On 2017/05/24 17:19:41, ossu-chromium wrote: > I've put in some initial logging for AudioOutputController and ...
3 years, 7 months ago (2017-05-25 13:29:03 UTC) #8
Max Morin
https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_output_controller.cc File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_output_controller.cc#newcode111 media/audio/audio_output_controller.cc:111: handler_->OnLog(base::StringPrintf("AOC::DoCreate (for device change: %s)", I think we should ...
3 years, 6 months ago (2017-05-29 08:12:53 UTC) #9
ossu-chromium
+liberato@ for owner review. I've had to change DoLog to be static and the event ...
3 years, 6 months ago (2017-05-29 14:19:43 UTC) #15
Max Morin
https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_output_controller.cc File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_output_controller.cc#newcode111 media/audio/audio_output_controller.cc:111: handler_->OnLog(base::StringPrintf("AOC::DoCreate (for device change: %s)", On 2017/05/29 14:19:43, ossu-chromium ...
3 years, 6 months ago (2017-05-29 14:25:44 UTC) #16
ossu-chromium
https://codereview.chromium.org/2900043002/diff/60001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode79 content/browser/renderer_host/media/audio_output_delegate_impl.cc:79: base::Bind(&AudioOutputDelegateImpl::DoLog, stream_id_, message)); On 2017/05/29 14:25:44, Max Morin wrote: ...
3 years, 6 months ago (2017-05-29 14:35:48 UTC) #17
Max Morin
https://codereview.chromium.org/2900043002/diff/60001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode79 content/browser/renderer_host/media/audio_output_delegate_impl.cc:79: base::Bind(&AudioOutputDelegateImpl::DoLog, stream_id_, message)); On 2017/05/29 14:35:48, ossu-chromium wrote: > ...
3 years, 6 months ago (2017-05-29 14:44:44 UTC) #18
liberato (no reviews please)
lgtm % some pretty minor nits, and the Bind vs BindOnce thing (good catch, btw). ...
3 years, 6 months ago (2017-05-30 16:53:35 UTC) #21
ossu-chromium
I figured DoLog doesn't need to be visible outside of OnLog, so I turned it ...
3 years, 6 months ago (2017-05-31 11:16:27 UTC) #24
Max Morin
Still lgtm, but some nits. https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode45 content/browser/renderer_host/media/audio_output_delegate_impl.cc:45: : delegate_(std::move(delegate)), stream_id_(delegate_->stream_id_) {} ...
3 years, 6 months ago (2017-05-31 11:52:41 UTC) #25
Max Morin
https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode84 content/browser/renderer_host/media/audio_output_delegate_impl.cc:84: content::MediaStreamManager::SendMessageToNativeLog(out_message); On 2017/05/31 11:52:40, Max Morin wrote: > Since ...
3 years, 6 months ago (2017-05-31 11:56:23 UTC) #26
ossu-chromium
https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode84 content/browser/renderer_host/media/audio_output_delegate_impl.cc:84: content::MediaStreamManager::SendMessageToNativeLog(out_message); On 2017/05/31 11:56:23, Max Morin wrote: > On ...
3 years, 6 months ago (2017-05-31 11:59:45 UTC) #27
ossu-chromium
Simplified version up. https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode45 content/browser/renderer_host/media/audio_output_delegate_impl.cc:45: : delegate_(std::move(delegate)), stream_id_(delegate_->stream_id_) {} On 2017/05/31 ...
3 years, 6 months ago (2017-05-31 13:47:21 UTC) #28
Max Morin
Final nit now, promise :) https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/renderer_host/media/audio_output_delegate_impl.cc#newcode45 content/browser/renderer_host/media/audio_output_delegate_impl.cc:45: : delegate_(std::move(delegate)), stream_id_(delegate_->stream_id_) {} ...
3 years, 6 months ago (2017-05-31 13:52:00 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2900043002/140001
3 years, 6 months ago (2017-06-01 08:59:01 UTC) #34
commit-bot: I haz the power
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9e1250518374b610d8a23e8b87c44a717a8c919a
3 years, 6 months ago (2017-06-01 12:27:41 UTC) #37
ossu-chromium
3 years, 6 months ago (2017-06-01 12:30:34 UTC) #38
Message was sent while issue was closed.
+cc grunell

Powered by Google App Engine
This is Rietveld 408576698