Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2868263002:
[Media Router] Add some SetForTest() functions to DialMediaSinkService and update unit tests (Closed)
|
Created:
3 years, 7 months ago by zhaobin Modified:
3 years, 7 months ago CC:
chfremer+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, mfoltz+watch_chromium.org, zhaobin+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Add some SetForTest() functions to DialMediaSinkService and update unit tests
Split from https://codereview.chromium.org/2837363002/ (resolving code review comments from Kevin)
BUG=687375
Review-Url: https://codereview.chromium.org/2868263002
Cr-Commit-Position: refs/heads/master@{#471404}
Committed: https://chromium.googlesource.com/chromium/src/+/c6a62f5fbc14639db9ffaf44297995aeea3dcd3e
Patch Set 1 #Patch Set 2 : call Stop() in DialMediaSinkService dtor #
Total comments: 6
Patch Set 3 : resolve code review comments from Kevin and remove discovery_start_ flag #
Total comments: 16
Patch Set 4 : resolve code review comments from Mark #
Messages
Total messages: 22 (14 generated)
zhaobin@chromium.org changed reviewers: + kmarshall@chromium.org, mfoltz@chromium.org
https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:55: DCHECK(dial_registry_); It's not possible for dial_registry to be non-null if the service had been successfully started; this seems too defensive? https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:72: void DialMediaSinkService::SetDialRegistryForTest(DialRegistry* dial_registry) { Maybe DCHECK for !discovery_started_ on these SetForTest methods, otherwise the listeners/observers could be in a bad state
marshallk@google.com changed reviewers: + marshallk@google.com
lgtm (same comment re: owners) https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:154: auto sinks = current_sinks_; This seems like an avoidable copy.
https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:55: DCHECK(dial_registry_); On 2017/05/10 23:22:01, Kevin M wrote: > It's not possible for dial_registry to be non-null if the service had been > successfully started; this seems too defensive? Done. https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:72: void DialMediaSinkService::SetDialRegistryForTest(DialRegistry* dial_registry) { On 2017/05/10 23:22:01, Kevin M wrote: > Maybe DCHECK for !discovery_started_ on these SetForTest methods, otherwise the > listeners/observers could be in a bad state |discovery_started_| removed. Maybe test code could ensure those logic instead of adding checks here? https://codereview.chromium.org/2868263002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:154: auto sinks = current_sinks_; On 2017/05/10 23:25:10, Kevin Marshall wrote: > This seems like an avoidable copy. Done.
LGTM with minor comments addressed Thanks - smaller patches are easier to review :-) https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:67: dial_registry_ = dial_registry; DCHECK(!dial_registry_) before assigning? I assume tests won't call Start() since you want to replace it with a mock. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:68: if (dial_registry_) { When would it be okay to pass a nullptr? Should there be a DCHECK(dial_registry)? https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:76: description_service_ = std::move(description_service); It seems like you don't want the test to call GetDeviceDescriptionService() before this, so DCHECK(!description_service_) before assigning? https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:80: finish_timer_ = std::move(timer); Similar comment - you don't want the test to start a timer before assigning the mock, so DCHECK(!finish_timer_) before assignment https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:131: DCHECK(finish_timer_); Is it possible for this to fail? It looks like this can only be called after OnDialDeviceEvent() which calls StartTimer(). Also, if finish_timer_ is not set, the following line will crash so the DCHECK doesn't add a lot of new information here. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:144: DCHECK_CURRENTLY_ON(BrowserThread::IO); Super nit: This is only called from a callback passed to a timer owned by this class; as long as the method creating the timer is run on the IO thread, this will be too. In that case, checking the thread again seems unnecessary to me. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:159: DCHECK_CURRENTLY_ON(BrowserThread::IO); Similar nit: This is only called from within this class. As long as the public API and exported callbacks have thread checks, this is not necessary. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:32: virtual void Stop(); Maybe: // Stops listening for DIAL device events.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:67: dial_registry_ = dial_registry; On 2017/05/11 23:12:14, mark a. foltz wrote: > DCHECK(!dial_registry_) before assigning? > > I assume tests won't call Start() since you want to replace it with a mock. Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:68: if (dial_registry_) { On 2017/05/11 23:12:14, mark a. foltz wrote: > When would it be okay to pass a nullptr? > Should there be a DCHECK(dial_registry)? Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:76: description_service_ = std::move(description_service); On 2017/05/11 23:12:14, mark a. foltz wrote: > It seems like you don't want the test to call GetDeviceDescriptionService() > before this, so DCHECK(!description_service_) before assigning? Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:80: finish_timer_ = std::move(timer); On 2017/05/11 23:12:14, mark a. foltz wrote: > Similar comment - you don't want the test to start a timer before assigning the > mock, so > DCHECK(!finish_timer_) before assignment Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:131: DCHECK(finish_timer_); On 2017/05/11 23:12:13, mark a. foltz wrote: > Is it possible for this to fail? It looks like this can only be called after > OnDialDeviceEvent() which calls StartTimer(). > > Also, if finish_timer_ is not set, the following line will crash so the DCHECK > doesn't add a lot of new information here. Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:144: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/05/11 23:12:14, mark a. foltz wrote: > Super nit: This is only called from a callback passed to a timer owned by this > class; as long as the method creating the timer is run on the IO thread, this > will be too. In that case, checking the thread again seems unnecessary to me. Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:159: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/05/11 23:12:14, mark a. foltz wrote: > Similar nit: This is only called from within this class. As long as the public > API and exported callbacks have thread checks, this is not necessary. Done. https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2868263002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:32: virtual void Stop(); On 2017/05/11 23:12:14, mark a. foltz wrote: > Maybe: // Stops listening for DIAL device events. 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_asan_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 zhaobin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marshallk@google.com, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2868263002/#ps60001 (title: "resolve code review comments from Mark")
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": 60001, "attempt_start_ts": 1494616248817130,
"parent_rev": "e1663d2c4db343e0285d6f2cc834654d00383c82", "commit_rev":
"c6a62f5fbc14639db9ffaf44297995aeea3dcd3e"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add some SetForTest() functions to DialMediaSinkService and update unit tests Split from https://codereview.chromium.org/2837363002/ (resolving code review comments from Kevin) BUG=687375 ========== to ========== [Media Router] Add some SetForTest() functions to DialMediaSinkService and update unit tests Split from https://codereview.chromium.org/2837363002/ (resolving code review comments from Kevin) BUG=687375 Review-Url: https://codereview.chromium.org/2868263002 Cr-Commit-Position: refs/heads/master@{#471404} Committed: https://chromium.googlesource.com/chromium/src/+/c6a62f5fbc14639db9ffaf442979... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c6a62f5fbc14639db9ffaf442979... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
