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

Issue 2910263002: [Media Router tests] Add minimal ScopedAllowIO for the tests to pass. (Closed)

Created:
3 years, 6 months ago by imcheng
Modified:
3 years, 6 months ago
Reviewers:
mark a. foltz, jam, zhaobin
CC:
chromium-reviews, imcheng+watch_chromium.org, mfoltz+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Media Router tests] Add minimal ScopedAllowIO for the tests to pass. In MR integration tests we perform certain file IO for loading test data. As IO restrictions are now enforced in browser_tests, we need to use ScopedAllowIO to bypass them. Also added media_router_integration_browsertest.cc to the ScopedAllowIO whitelist. BUG=724573 Review-Url: https://codereview.chromium.org/2910263002 Cr-Commit-Position: refs/heads/master@{#476010} Committed: https://chromium.googlesource.com/chromium/src/+/a7e23ba097190eb75739fe1687002696b5962853

Patch Set 1 : 1 more ScopedAllowIO and presubmit #

Patch Set 2 : crbug #

Total comments: 4

Patch Set 3 : Addressed Mark's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -3 lines) Patch
M PRESUBMIT.py View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/test/media_router/media_router_integration_browsertest.cc View 1 2 3 chunks +12 lines, -3 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 21 (9 generated)
imcheng
Reviewers, PTAL: jam@: PRESUBMIT.py zhaobin@: *.cc Thanks!
3 years, 6 months ago (2017-05-30 19:17:58 UTC) #3
zhaobin
lgtm May need to add "base::ThreadRestrictions::ScopedAllowIO allow_io;" to media_router_one_ua_integration_browsertest.cc as well.
3 years, 6 months ago (2017-05-30 20:41:00 UTC) #4
imcheng
On 2017/05/30 20:41:00, zhaobin wrote: > lgtm > > May need to add "base::ThreadRestrictions::ScopedAllowIO allow_io;" ...
3 years, 6 months ago (2017-05-30 23:44:29 UTC) #5
mark a. foltz
Do you also need this fix for MediaRouterBaseBrowserTest::ParseCommandLine? https://codereview.chromium.org/2910263002/diff/40001/chrome/test/media_router/media_router_integration_browsertest.cc File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2910263002/diff/40001/chrome/test/media_router/media_router_integration_browsertest.cc#newcode284 chrome/test/media_router/media_router_integration_browsertest.cc:284: base::ThreadRestrictions::ScopedAllowIO ...
3 years, 6 months ago (2017-05-30 23:50:04 UTC) #7
jam
rs lgtm to whatever lines you need in the presubmit
3 years, 6 months ago (2017-05-31 00:22:54 UTC) #8
imcheng
On 2017/05/30 23:50:04, mark a. foltz wrote: > Do you also need this fix for ...
3 years, 6 months ago (2017-05-31 00:29:57 UTC) #9
imcheng
https://codereview.chromium.org/2910263002/diff/40001/chrome/test/media_router/media_router_integration_browsertest.cc File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2910263002/diff/40001/chrome/test/media_router/media_router_integration_browsertest.cc#newcode284 chrome/test/media_router/media_router_integration_browsertest.cc:284: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2017/05/30 23:50:04, mark a. foltz wrote: ...
3 years, 6 months ago (2017-05-31 00:30:22 UTC) #10
mark a. foltz
lgtm
3 years, 6 months ago (2017-05-31 15:57:53 UTC) #11
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/2910263002/60001
3 years, 6 months ago (2017-05-31 17:24:30 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/279703) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 6 months ago (2017-05-31 17:31:30 UTC) #16
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/2910263002/60001
3 years, 6 months ago (2017-05-31 19:11:01 UTC) #18
commit-bot: I haz the power
3 years, 6 months ago (2017-05-31 20:46:40 UTC) #21
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a7e23ba097190eb75739fe168700...

Powered by Google App Engine
This is Rietveld 408576698