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

Issue 2924033002: TrackObserver DCHECK not on main thread, tests updated. (Closed)

Created:
3 years, 6 months ago by hbos_chromium
Modified:
3 years, 6 months ago
Reviewers:
Guido Urdaneta
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

TrackObserver DCHECK not on main thread, tests updated. The test was using a ThreadChecker to check that OnChanged is invoked on the webrtc signaling thread. The problem is it is attached to whatever thread first calls OnChanged, which in several tests was the main thread. The DCHECK is changed to "not on main thread", which should imply its invoked on the webrtc signaling thread. This is good enough, and avoids refactoring and changing initialization order that would be necessary to pass the webrtc signaling thread for the sake of a single DCHECK. The DCHECK showed that RTCPeerConnectionHandlerTest and MediaStreamRemoteVideoSourceTest was using the TrackObserver on the wrong thread - tests are updated. BUG=705901 Review-Url: https://codereview.chromium.org/2924033002 Cr-Commit-Position: refs/heads/master@{#478569} Committed: https://chromium.googlesource.com/chromium/src/+/9e96b2036f3956bdf7bfc180044b90b71496a454

Patch Set 1 #

Patch Set 2 : MediaStreamRemoteVideoSourceTest using the signaling thread #

Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -54 lines) Patch
M content/renderer/media/rtc_peer_connection_handler_unittest.cc View 18 chunks +77 lines, -45 lines 0 comments Download
M content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc View 1 3 chunks +45 lines, -6 lines 0 comments Download
M content/renderer/media/webrtc/track_observer.cc View 4 chunks +2 lines, -3 lines 0 comments Download

Messages

Total messages: 20 (16 generated)
hbos_chromium
Please take a look, guidou.
3 years, 6 months ago (2017-06-08 09:31:13 UTC) #10
Guido Urdaneta
lgtm
3 years, 6 months ago (2017-06-09 11:34:49 UTC) #14
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/2924033002/20001
3 years, 6 months ago (2017-06-12 07:57:03 UTC) #16
commit-bot: I haz the power
3 years, 6 months ago (2017-06-12 09:02:02 UTC) #20
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9e96b2036f3956bdf7bfc180044b...

Powered by Google App Engine
This is Rietveld 408576698