Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886683003:
media: Shutdown AudioManager in TestMojoMediaClient (Closed)
|
Created:
3 years, 7 months ago by xhwang Modified:
3 years, 7 months ago Reviewers:
alokp CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Shutdown AudioManager in TestMojoMediaClient
By AudioManager API, Shutdown() must be called before destruction. This
isn't caught by tests because media_service_unittests is only run on
release builds.
Review-Url: https://codereview.chromium.org/2886683003
Cr-Commit-Position: refs/heads/master@{#472225}
Committed: https://chromium.googlesource.com/chromium/src/+/6e0f47b9cc33450f93d02d238cd4e60dffa89421
Patch Set 1 #Patch Set 2 : more fix #
Total comments: 2
Patch Set 3 : drop flushing message loop #Messages
Total messages: 21 (15 generated)
The CQ bit was checked by xhwang@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...
xhwang@chromium.org changed reviewers: + alokp@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 xhwang@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...
lgtm https://codereview.chromium.org/2886683003/diff/20001/media/mojo/services/tes... File media/mojo/services/test_mojo_media_client.cc (right): https://codereview.chromium.org/2886683003/diff/20001/media/mojo/services/tes... media/mojo/services/test_mojo_media_client.cc:33: // AudioManager destructor requires MessageLoop. You do not need to manually reset and flush the message loop. With the latest changes, only Shutdown is necessary.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2886683003/diff/20001/media/mojo/services/tes... File media/mojo/services/test_mojo_media_client.cc (right): https://codereview.chromium.org/2886683003/diff/20001/media/mojo/services/tes... media/mojo/services/test_mojo_media_client.cc:33: // AudioManager destructor requires MessageLoop. On 2017/05/16 16:52:26, alokp wrote: > You do not need to manually reset and flush the message loop. With the latest > changes, only Shutdown is necessary. Done.
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_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 xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2886683003/#ps40001 (title: "drop flushing message loop")
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": 40001, "attempt_start_ts": 1494961312163090,
"parent_rev": "ff403efe7925316215b23c13d953305d194ceffc", "commit_rev":
"6e0f47b9cc33450f93d02d238cd4e60dffa89421"}
Message was sent while issue was closed.
Description was changed from ========== media: Shutdown AudioManager in TestMojoMediaClient By AudioManager API, Shutdown() must be called before destruction. This isn't caught by tests because media_service_unittests is only run on release builds. ========== to ========== media: Shutdown AudioManager in TestMojoMediaClient By AudioManager API, Shutdown() must be called before destruction. This isn't caught by tests because media_service_unittests is only run on release builds. Review-Url: https://codereview.chromium.org/2886683003 Cr-Commit-Position: refs/heads/master@{#472225} Committed: https://chromium.googlesource.com/chromium/src/+/6e0f47b9cc33450f93d02d238cd4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6e0f47b9cc33450f93d02d238cd4...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2890733004/ by kolos@chromium.org. The reason for reverting is: Layout tests failure ; mojo/module-loading(-manual-deps-loading).html BUG=723461. |
