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

Issue 2799363005: Removing public access to AudioManager device info interface. (Closed)

Created:
3 years, 8 months ago by o1ka
Modified:
3 years, 7 months ago
Reviewers:
DaleCurtis, Max Morin
CC:
audio-mojo-cl_google.com, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, posciak+watch_chromium.org, alokp
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Removing public access to AudioManager device info interface. With audio moving to a separate process, device info access is allowed through AudioSystem interface only. We want to avoid AudioManager device info interface leakage. * AudioManager device info methods moved to protected. * AudioSystem becomes AudioManager's friend. * For the unit tests of the functionality moving to audio process (media unit tests): AudioDeviceInfoAccessorForTests class is introduced, which is AudioManager's friend and is compiled only for media_unittests target. BUG=672468 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/2799363005 Cr-Commit-Position: refs/heads/master@{#468998} Committed: https://chromium.googlesource.com/chromium/src/+/0abd2199c67e65774e8fff05a31068d17b2ae805

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : Fix for build errors on mac, win, android #

Patch Set 4 : another round of build error fixes #

Patch Set 5 : more fixes #

Patch Set 6 : missing header for win added #

Total comments: 2

Patch Set 7 : missed media_export header added #

Patch Set 8 : other way around #

Patch Set 9 : rebase #

Patch Set 10 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+419 lines, -244 lines) Patch
M content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +10 lines, -10 lines 0 comments Download
M media/audio/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M media/audio/android/audio_android_unittest.cc View 1 2 3 7 chunks +18 lines, -9 lines 0 comments Download
A media/audio/audio_device_info_accessor_for_tests.h View 1 2 3 4 5 6 7 1 chunk +49 lines, -0 lines 0 comments Download
A media/audio/audio_device_info_accessor_for_tests.cc View 1 2 3 4 1 chunk +64 lines, -0 lines 0 comments Download
M media/audio/audio_input_unittest.cc View 3 chunks +6 lines, -3 lines 0 comments Download
M media/audio/audio_low_latency_input_output_unittest.cc View 4 chunks +8 lines, -5 lines 0 comments Download
M media/audio/audio_manager.h View 4 chunks +66 lines, -64 lines 0 comments Download
M media/audio/audio_manager_base.h View 3 chunks +17 lines, -16 lines 0 comments Download
M media/audio/audio_manager_unittest.cc View 16 chunks +23 lines, -17 lines 0 comments Download
M media/audio/audio_output_proxy_unittest.cc View 3 chunks +8 lines, -6 lines 0 comments Download
M media/audio/audio_system_impl.h View 1 chunk +17 lines, -0 lines 0 comments Download
M media/audio/audio_system_impl.cc View 1 2 3 4 5 6 7 8 9 6 chunks +67 lines, -66 lines 0 comments Download
M media/audio/mac/audio_auhal_mac_unittest.cc View 1 2 3 5 chunks +7 lines, -4 lines 0 comments Download
M media/audio/mac/audio_low_latency_input_mac_unittest.cc View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M media/audio/mock_audio_manager.h View 3 chunks +22 lines, -22 lines 0 comments Download
M media/audio/win/audio_low_latency_input_win_unittest.cc View 1 2 3 4 chunks +9 lines, -6 lines 0 comments Download
M media/audio/win/audio_low_latency_output_win_unittest.cc View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M media/audio/win/audio_output_win_unittest.cc View 1 2 3 4 5 16 chunks +20 lines, -14 lines 0 comments Download

Messages

Total messages: 63 (51 generated)
DaleCurtis
Does the presubmit catch _for_tests in class names? Otherwise this sgtm.
3 years, 8 months ago (2017-04-10 19:27:08 UTC) #28
Max Morin
lgtm https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_device_info_accessor_for_tests.h File media/audio/audio_device_info_accessor_for_tests.h (right): https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_device_info_accessor_for_tests.h#newcode19 media/audio/audio_device_info_accessor_for_tests.h:19: class MEDIA_EXPORT AudioDeviceInfoAccessorForTests { I still think having ...
3 years, 8 months ago (2017-04-11 07:27:08 UTC) #29
o1ka
On 2017/04/10 19:27:08, DaleCurtis wrote: > Does the presubmit catch _for_tests in class names? Otherwise ...
3 years, 8 months ago (2017-04-11 07:55:27 UTC) #30
Max Morin
https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_manager.h File media/audio/audio_manager.h (right): https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_manager.h#newcode199 media/audio/audio_manager.h:199: protected: Maybe add a comment to warn against adding ...
3 years, 8 months ago (2017-04-11 07:57:43 UTC) #31
o1ka
On 2017/04/11 07:27:08, Max Morin wrote: > lgtm > > https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_device_info_accessor_for_tests.h > File media/audio/audio_device_info_accessor_for_tests.h (right): ...
3 years, 8 months ago (2017-04-11 08:24:44 UTC) #34
o1ka
On 2017/04/11 07:57:43, Max Morin wrote: > https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_manager.h > File media/audio/audio_manager.h (right): > > https://codereview.chromium.org/2799363005/diff/100001/media/audio/audio_manager.h#newcode199 ...
3 years, 8 months ago (2017-04-11 08:40:00 UTC) #35
o1ka
Ready for review now - PTAL.
3 years, 8 months ago (2017-04-11 13:54:46 UTC) #42
DaleCurtis
Does the presubmit actually catch usage of the for_tests class in production code? It doesn't ...
3 years, 8 months ago (2017-04-11 19:02:14 UTC) #43
o1ka
On 2017/04/11 19:02:14, DaleCurtis wrote: > Does the presubmit actually catch usage of the for_tests ...
3 years, 8 months ago (2017-04-11 19:42:00 UTC) #44
DaleCurtis
Okay, I was hoping it'd just be a few friends, if it's hundreds, this approach ...
3 years, 8 months ago (2017-04-11 20:46:12 UTC) #45
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/2799363005/180001
3 years, 7 months ago (2017-05-03 16:13:03 UTC) #60
commit-bot: I haz the power
3 years, 7 months ago (2017-05-03 16:18:50 UTC) #63
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/0abd2199c67e65774e8fff05a310...

Powered by Google App Engine
This is Rietveld 408576698