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

Issue 2698503005: Remove unnecessary casts in AudioManagerDeleter on Mac. (Closed)

Created:
3 years, 10 months ago by Max Morin
Modified:
3 years, 10 months ago
Reviewers:
DaleCurtis
CC:
audio-mojo-cl_google.com, chromium-reviews, feature-media-reviews_chromium.org, o1ka
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove unnecessary casts in AudioManagerDeleter on Mac. The code currently does a const_cast (ewwww), and then a static_cast<AudioManagerMac*>. The const cast is not needed, since delete works on const pointers, and the static_cast is not needed either, since the destructor is virtual so the correct one will be called either way. This also means that tests currently invokes undefined behavior when using ScopedAudioManagerPtr with a mock/fake AudioManager. Oops. This Cl fixes that. BUG=693041 CQ_INCLUDE_TRYBOTS=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/2698503005 Cr-Commit-Position: refs/heads/master@{#451050} Committed: https://chromium.googlesource.com/chromium/src/+/2fcc731a7f1502b514d622723b7aecd4771ee422

Patch Set 1 #

Patch Set 2 : Fix friendship. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -5 lines) Patch
M media/audio/audio_manager.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M media/audio/audio_manager.cc View 2 chunks +3 lines, -3 lines 1 comment Download
M media/audio/mac/audio_manager_mac.h View 1 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 19 (15 generated)
Max Morin
Dale: PTAL. https://codereview.chromium.org/2698503005/diff/20001/media/audio/audio_manager.cc File media/audio/audio_manager.cc (right): https://codereview.chromium.org/2698503005/diff/20001/media/audio/audio_manager.cc#newcode9 media/audio/audio_manager.cc:9: #include <utility> Linter wanted it for move.
3 years, 10 months ago (2017-02-16 15:10:51 UTC) #12
DaleCurtis
lgtm
3 years, 10 months ago (2017-02-16 16:42:48 UTC) #13
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/2698503005/20001
3 years, 10 months ago (2017-02-16 16:47:39 UTC) #16
commit-bot: I haz the power
3 years, 10 months ago (2017-02-16 19:29:21 UTC) #19
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2fcc731a7f1502b514d622723b7a...

Powered by Google App Engine
This is Rietveld 408576698