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

Issue 2571473002: Fix cases where RunLoop is created without a MessageLoop. (Closed)

Created:
4 years ago by Alexander Semashko
Modified:
4 years ago
Reviewers:
jam
CC:
chromium-reviews, vmpstr+watch_chromium.org, jam, Randy Smith (Not in Mondays), feature-media-reviews_chromium.org, darin-cc_chromium.org, loading-reviews_chromium.org, chirantan+watch_chromium.org, mmenke
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix cases where RunLoop is created without a MessageLoop. The tests in these cases can pass only if Quit is called before Run, or if Run is never called at all. If this is guaranteed to hold true, then RunLoop is not needed (such as in timer_unittest.cc). Other cases (where it is not guaranteed) are tests that can crash occasionally. BUG=668707 Committed: https://crrev.com/68c9f104d87f31212b353d0fc20c3c57ec23125e Cr-Commit-Position: refs/heads/master@{#438126}

Patch Set 1 #

Patch Set 2 : Remove unused ScreenshotTester::run_loop_ that triggered the DCHECK on chromeos. #

Patch Set 3 : Add missing includes. #

Patch Set 4 : rebase #

Patch Set 5 : Fix chromeos tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -42 lines) Patch
M base/run_loop.cc View 1 chunk +1 line, -0 lines 0 comments Download
M base/timer/timer_unittest.cc View 3 chunks +54 lines, -29 lines 0 comments Download
M chrome/browser/chromeos/login/screenshot_testing/screenshot_tester.h View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/login/screenshot_testing/screenshot_tester.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/login/screenshot_testing/screenshot_testing_mixin.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/login/signin/device_id_browsertest.cc View 1 2 3 4 4 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/system/device_disabling_browsertest.cc View 1 2 3 4 4 chunks +5 lines, -3 lines 0 comments Download
M content/browser/loader/async_revalidation_manager_browsertest.cc View 3 chunks +12 lines, -3 lines 0 comments Download
M media/base/audio_renderer_mixer_input_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 19 (14 generated)
Alexander Semashko
4 years ago (2016-12-12 13:46:04 UTC) #2
jam
lgtm
4 years ago (2016-12-12 17:58:07 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/2571473002/80001
4 years ago (2016-12-13 09:43:31 UTC) #14
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years ago (2016-12-13 11:35:20 UTC) #17
commit-bot: I haz the power
4 years ago (2016-12-13 11:39:05 UTC) #19
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/68c9f104d87f31212b353d0fc20c3c57ec23125e
Cr-Commit-Position: refs/heads/master@{#438126}

Powered by Google App Engine
This is Rietveld 408576698