Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872953002:
iOS audio session isInterrupted flag does not get reset correctly: (Closed)
|
Created:
3 years, 7 months ago by jtt_webrtc Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, audio-team_agora.io, sdk-team_agora.io, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptioniOS audio session isInterrupted flag does not get reset correctly:
BUG=webrtc:7605
Review-Url: https://codereview.webrtc.org/2872953002
Cr-Commit-Position: refs/heads/master@{#18072}
Committed: https://chromium.googlesource.com/external/webrtc/+/5171a7f58c99774953c4563fc37b8c59effcc3a4
Patch Set 1 #Patch Set 2 : Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into interrupt #
Total comments: 4
Patch Set 3 : Moved unit test to right file #Patch Set 4 : Added gtest dependency #
Total comments: 3
Patch Set 5 : renamed vars #Patch Set 6 : Reformatted comments #Patch Set 7 : Errant dot #
Total comments: 1
Patch Set 8 : added comments #
Total comments: 3
Patch Set 9 : Updated comments #Patch Set 10 : Updated comments #Patch Set 11 : Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into interrupt #Messages
Total messages: 37 (18 generated)
jtteh@webrtc.org changed reviewers: + tkchin@webrtc.org
The CQ bit was checked by jtteh@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16884)
Description was changed from ========== iOS audio session isInterrupted flag does not get reset correctly: ========== to ========== iOS audio session isInterrupted flag does not get reset correctly: webrtc::7605 ==========
Description was changed from ========== iOS audio session isInterrupted flag does not get reset correctly: webrtc::7605 ========== to ========== iOS audio session isInterrupted flag does not get reset correctly: BUG=webrtc::7605 ==========
Description was changed from ========== iOS audio session isInterrupted flag does not get reset correctly: BUG=webrtc::7605 ========== to ========== iOS audio session isInterrupted flag does not get reset correctly: BUG=webrtc:7605 ==========
https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:15: #define private public oof. Don't do this - not good to redefine keywords, even in tests. It will eventually come back to bother us. https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:252: - (void)testInterruptedAudioSession { this belongs in: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_device/... https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:277: delete audioBuffer; fyi it is extremely rare to use delete now since unique_ptr is available
The CQ bit was checked by jtteh@webrtc.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.webrtc.org/...
On 2017/05/09 17:48:25, tkchin_webrtc wrote: > https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): > https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:15: #define private > public > oof. Don't do this - not good to redefine keywords, even in tests. It will > eventually come back to bother us. Made friend test class. > https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:252: - > (void)testInterruptedAudioSession { > this belongs in: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_device/... Moved and renamed from .cc to .mm > https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:277: delete > audioBuffer; > fyi it is extremely rare to use delete now since unique_ptr is available Done.
https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm (right): https://codereview.webrtc.org/2872953002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm:277: delete audioBuffer; On 2017/05/09 17:48:25, tkchin_webrtc wrote: > fyi it is extremely rare to use delete now since unique_ptr is available Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25480) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24374)
The CQ bit was checked by jtteh@webrtc.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.webrtc.org/...
https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm (right): https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:830: std::unique_ptr<webrtc::AudioDeviceIOS> audioDevice; use C++ style in C++ file. underscores. https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:832: std::unique_ptr<webrtc::AudioDeviceBuffer> audioBuffer; There are several methods in this test file already that create buffers / start playout etc. Reuse those? https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:837: [session notifyDidBeginInterruption]; // force interruption nit: we typically do not comment inline, move comment before, use periods and capitalize
On 2017/05/09 21:07:02, tkchin_webrtc wrote: > https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm (right): > > https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:830: > std::unique_ptr<webrtc::AudioDeviceIOS> audioDevice; > use C++ style in C++ file. underscores. > > https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:832: > std::unique_ptr<webrtc::AudioDeviceBuffer> audioBuffer; > There are several methods in this test file already that create buffers / start > playout etc. Reuse those? Can't use those as those methods use AudioDeviceModule which wrap AudioDeviceIOS. > > https://codereview.webrtc.org/2872953002/diff/60001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:837: [session > notifyDidBeginInterruption]; // force interruption > nit: we typically do not comment inline, move comment before, use periods and > capitalize Done.
Fixed errant dot.
lgtm
The CQ bit was checked by jtteh@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494366502060060,
"parent_rev": "d277a7cccee58367f78a4a18e493d6301e5b5e88", "commit_rev":
"5171a7f58c99774953c4563fc37b8c59effcc3a4"}
Message was sent while issue was closed.
Description was changed from ========== iOS audio session isInterrupted flag does not get reset correctly: BUG=webrtc:7605 ========== to ========== iOS audio session isInterrupted flag does not get reset correctly: BUG=webrtc:7605 Review-Url: https://codereview.webrtc.org/2872953002 Cr-Commit-Position: refs/heads/master@{#18072} Committed: https://chromium.googlesource.com/external/webrtc/+/5171a7f58c99774953c4563fc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/5171a7f58c99774953c4563fc...
Message was sent while issue was closed.
henrika@webrtc.org changed reviewers: + henrika@webrtc.org
Message was sent while issue was closed.
Great work. https://codereview.webrtc.org/2872953002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm (right): https://codereview.webrtc.org/2872953002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:828: TEST_F(AudioDeviceTest, testInterruptedAudioSession) { Care to add some comments above this test to explain its purpose?
Message was sent while issue was closed.
Added comments per henrika.
Message was sent while issue was closed.
Appreciated. lgtm
Message was sent while issue was closed.
https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm (right): https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:828: // Verifies that the AudioDeviceIOS is_interrupted_ flag is reset correctly after an iOS Please use 80 characters per line as in rest of the file. https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:830: // When AudioDeviceIOS is interrupted, is_interrupted_ set to true. 'is_interrupted is set to true' https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:833: // is not passed to the delegate as it is no longer in the delegates list. Could you make it more clear that this is the expected behaviour, e.g. by using 'should' instead of 'is' here?
Message was sent while issue was closed.
On 2017/05/11 08:00:28, henrika_webrtc wrote: > https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... > File webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm (right): > > https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... > webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:828: // Verifies > that the AudioDeviceIOS is_interrupted_ flag is reset correctly after an iOS > Please use 80 characters per line as in rest of the file. > > https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... > webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:830: // When > AudioDeviceIOS is interrupted, is_interrupted_ set to true. > 'is_interrupted is set to true' > > https://codereview.webrtc.org/2872953002/diff/140001/webrtc/modules/audio_dev... > webrtc/modules/audio_device/ios/audio_device_unittest_ios.mm:833: // is not > passed to the delegate as it is no longer in the delegates list. > Could you make it more clear that this is the expected behaviour, e.g. by using > 'should' instead of 'is' here? Rewrote comments and reformatted.
Message was sent while issue was closed.
Rewrote comments.
Message was sent while issue was closed.
Bump.
Message was sent while issue was closed.
Sorry for the delay. lgtm |
