Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809293006:
[Chromoting] Move DefaultAudioDeviceChangeDetector out of AudioCapturerWin (Closed)
Description[Chromoting] Move DefaultAudioDeviceChangeDetector out of AudioCapturerWin
This is a trivial change to move IMMNotificationClient implementation out of
AudioCapturerWin. Now the new DefaultAudioDeviceChangeDetector can unregister
itself from IMMDeviceEnumerator in destructor.
BUG=669070
Review-Url: https://codereview.chromium.org/2809293006
Cr-Commit-Position: refs/heads/master@{#466824}
Committed: https://chromium.googlesource.com/chromium/src/+/00c53fe4b76655f083d3cf25e25154467ffbe675
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #
Total comments: 12
Patch Set 3 : Remove override of destructor #
Messages
Total messages: 40 (31 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_ozone_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_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Chromoting] Move DefaultAudioDeviceChangeDetector out of AudioCapturerWin This is a trivial change to move IMMNotificationClient implementation out of AudioCapturerWin. Now the new DefaultAudioDeviceChangeDetector can unregister itself from IMMDeviceEnumerator in destructor. BUG=669070 ========== to ========== [Chromoting] Move DefaultAudioDeviceChangeDetector out of AudioCapturerWin This is a trivial change to move IMMNotificationClient implementation out of AudioCapturerWin. Now the new DefaultAudioDeviceChangeDetector can unregister itself from IMMDeviceEnumerator in destructor. BUG=669070 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... File remoting/host/win/default_audio_device_change_detector.h (right): https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:16: // This class does not use the default referring memory management method s/default referring memory management method/reference counting/ Or say "This class doesn't increment ref-count for the IMMDeviceEnumerator". But given the last sentence in this comment I don't think we need to mention ref-counting at all. Do we need this restriction? If you update this class to use ScopedComPtr for enumerator_ then the capturer wouldn't have to keep mm_device_enumerator_, and this would make destruction cleaner and less error-prone. https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:20: // IMMDeviceEnumerator::UnregisterEndpointNotificationCallback() will be called s/will be/is/ in the two places above. https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:28: HRESULT __stdcall OnDefaultDeviceChanged(EDataFlow flow, Move IMMNotificationClient implementation to the private section? https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:37: HRESULT __stdcall OnDeviceAdded(LPCWSTR pwstrDeviceId) override { Normally all virtual method overrides should be in the .cc file.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... File remoting/host/win/default_audio_device_change_detector.h (right): https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:16: // This class does not use the default referring memory management method On 2017/04/13 01:30:12, Sergey Ulanov wrote: > s/default referring memory management method/reference counting/ > Or say "This class doesn't increment ref-count for the IMMDeviceEnumerator". > > But given the last sentence in this comment I don't think we need to mention > ref-counting at all. > > Do we need this restriction? If you update this class to use ScopedComPtr for > enumerator_ then the capturer wouldn't have to keep mm_device_enumerator_, and > this would make destruction cleaner and less error-prone. I have not noticed the mm_device_enumerator_ is for this class only. I will use ScopedComPtr instead of raw pointer. But here we have two issues, the one is the ref-count of IMMDeviceEnumerator, the other is the ref-count of DefaultAudioDeviceChangeDetector itself. MSDN suggested to delete this if ref-counter reaches zero in Release() function, but this class does not implement this functionality because of the limited user scenario. My comment is confusing, I will update it anyway. https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:20: // IMMDeviceEnumerator::UnregisterEndpointNotificationCallback() will be called On 2017/04/13 01:30:12, Sergey Ulanov wrote: > s/will be/is/ in the two places above. Done. https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:28: HRESULT __stdcall OnDefaultDeviceChanged(EDataFlow flow, On 2017/04/13 01:30:12, Sergey Ulanov wrote: > Move IMMNotificationClient implementation to the private section? Done. https://codereview.chromium.org/2809293006/diff/40001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:37: HRESULT __stdcall OnDeviceAdded(LPCWSTR pwstrDeviceId) override { On 2017/04/13 01:30:12, Sergey Ulanov wrote: > Normally all virtual method overrides should be in the .cc file. Done.
Any other comments for this change?
lgtm with a few comments https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... File remoting/host/win/default_audio_device_change_detector.cc (right): https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.cc:21: LOG(ERROR) << "Failed to register IMMNotificationClient, we may not be " Should this be a WARNING instead or is ERROR the right level? https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... File remoting/host/win/default_audio_device_change_detector.h (right): https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:17: // IMMDeviceEnumerator in constructor, and unregisters in destructor. nit: remove comma https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:18: // This class does not use the default referring memory management method s/referring/ref-counting https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:24: const base::win::ScopedComPtr<IMMDeviceEnumerator>& enumerator); single param c'tors should be marked explicit https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:25: ~DefaultAudioDeviceChangeDetector(); override? https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:36: // No-ops overrides. I wouldn't call out the no-op bit here, just document the methods: // IMMNotificationClient implementation.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM after Joes comments are addressed
The CQ bit was unchecked by commit-bot@chromium.org
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...)
I really cannot imagine this change may impact Android webview. https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... File remoting/host/win/default_audio_device_change_detector.cc (right): https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.cc:21: LOG(ERROR) << "Failed to register IMMNotificationClient, we may not be " On 2017/04/24 17:29:41, joedow wrote: > Should this be a WARNING instead or is ERROR the right level? Done. https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... File remoting/host/win/default_audio_device_change_detector.h (right): https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:17: // IMMDeviceEnumerator in constructor, and unregisters in destructor. On 2017/04/24 17:29:41, joedow wrote: > nit: remove comma Done. https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:18: // This class does not use the default referring memory management method On 2017/04/24 17:29:41, joedow wrote: > s/referring/ref-counting Done. https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:24: const base::win::ScopedComPtr<IMMDeviceEnumerator>& enumerator); On 2017/04/24 17:29:41, joedow wrote: > single param c'tors should be marked explicit Done. https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:25: ~DefaultAudioDeviceChangeDetector(); On 2017/04/24 17:29:41, joedow wrote: > override? e:\b\c\b\win\src\remoting\host\win\default_audio_device_change_detector.h(25): error C3668: 'remoting::DefaultAudioDeviceChangeDetector::~DefaultAudioDeviceChangeDetector': method with override specifier 'override' did not override any base class methods It looks like IMMNotificationClient does not have a destructor at all. https://codereview.chromium.org/2809293006/diff/60001/remoting/host/win/defau... remoting/host/win/default_audio_device_change_detector.h:36: // No-ops overrides. On 2017/04/24 17:29:41, joedow wrote: > I wouldn't call out the no-op bit here, just document the methods: > // IMMNotificationClient implementation. I think both comments should be useful here. No-ops overrides can tell the readers only the two functions above are relative.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2809293006/#ps100001 (title: "Remove override of destructor")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493073909135710,
"parent_rev": "dcb37de1f6962d7e2fef16098b271ce7f49dd960", "commit_rev":
"00c53fe4b76655f083d3cf25e25154467ffbe675"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Move DefaultAudioDeviceChangeDetector out of AudioCapturerWin This is a trivial change to move IMMNotificationClient implementation out of AudioCapturerWin. Now the new DefaultAudioDeviceChangeDetector can unregister itself from IMMDeviceEnumerator in destructor. BUG=669070 ========== to ========== [Chromoting] Move DefaultAudioDeviceChangeDetector out of AudioCapturerWin This is a trivial change to move IMMNotificationClient implementation out of AudioCapturerWin. Now the new DefaultAudioDeviceChangeDetector can unregister itself from IMMDeviceEnumerator in destructor. BUG=669070 Review-Url: https://codereview.chromium.org/2809293006 Cr-Commit-Position: refs/heads/master@{#466824} Committed: https://chromium.googlesource.com/chromium/src/+/00c53fe4b76655f083d3cf25e251... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/00c53fe4b76655f083d3cf25e251... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
