Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2927833002:
[Media Router] Add CastMediaSinkService (Closed)
|
Created:
3 years, 6 months ago by zhaobin Modified:
3 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, imcheng+watch_chromium.org, mfoltz+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Add CastMediaSinkService
NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands
Discover process is similar to DialMediaSinkService:
- CastMediaSinkService::Start() registers itself with DnsRegistry
- CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer
- Start opening channel for each service on IO thread
- CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes
- Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread()
- Create MediaSinkInternal and store it in CastSinkService's sink map
- Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires
BUG=687377
Review-Url: https://codereview.chromium.org/2927833002
Cr-Original-Commit-Position: refs/heads/master@{#484979}
Committed: https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3c66b13cee3b5
Review-Url: https://codereview.chromium.org/2927833002
Cr-Commit-Position: refs/heads/master@{#485110}
Committed: https://chromium.googlesource.com/chromium/src/+/59516e7383c4e6e0231ca54d664f8fa0ea039af0
Patch Set 1 #Patch Set 2 : add dependency patchset #
Total comments: 6
Patch Set 3 : rebase and clean up namespace #Patch Set 4 : update comments for CastMediaSinkService #Patch Set 5 : add unit tests #
Total comments: 16
Patch Set 6 : rebase and resolve code review comments from Derek #Patch Set 7 : rebase #Patch Set 8 : rebase #
Total comments: 25
Patch Set 9 : resolve code review comments from Derek and Mark #Patch Set 10 : fix windows compile error #Patch Set 11 : merge with master #Patch Set 12 : fix tsan and mac unit test failures #Dependent Patchsets: Messages
Total messages: 57 (41 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:157: cast_socket_service_->OpenChannel( WIP in https://codereview.chromium.org/2925053005/. https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:17: #include "extensions/browser/api/cast_channel/cast_socket_service.h" This will become components/cast_channel/cast_socket_service after the //extensions -> //components patch lands.
I'll take a look once the dependent patch lands. Thanks! https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:34: // Cast devices (Smart TVs, Game Consoles, etc.). Cast isn't supported on most smart TVs or game consoles. So just leave off the last part.
https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:157: cast_socket_service_->OpenChannel( On 2017/06/07 18:41:12, zhaobin wrote: > WIP in https://codereview.chromium.org/2925053005/. Done. https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:17: #include "extensions/browser/api/cast_channel/cast_socket_service.h" On 2017/06/07 18:41:12, zhaobin wrote: > This will become components/cast_channel/cast_socket_service after the > //extensions -> //components patch lands. Done. https://codereview.chromium.org/2927833002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:34: // Cast devices (Smart TVs, Game Consoles, etc.). On 2017/06/07 21:13:33, mark a. foltz wrote: > Cast isn't supported on most smart TVs or game consoles. So just leave off the > last part. Done.
Some preliminary comments. Will review again after the patch to implement CastSocketService::OpenSocket is revised. https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:88: cast_socket_service_ = cast_channel::CastSocketServiceFactory::GetInstance() Would we need to add a DependsOn from MediaRouterFactory to CastSocketServiceFactory (until we make CastSocketService a singleton)? https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:125: cast_socket_service_ = cast_socket_service; Add a DCHECK to be consistent with above. https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:157: cast_socket_service_->OpenSocket( What if the socket already exists (opened by browser or the extension before)? https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:165: int channel_id, Would it be possible to pass back the CastSocket in the callback? https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:190: MediaSinkInternal sink; DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:197: if (!base::ContainsValue(current_services_, service)) { This check means we would drop the sink if we somehow had a OnDnsSdEvent not containing |service|. If we didn't do that, would the sink be eventually removed anyway if the channel errors out? https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:47: void SetDnsSdRegistryForTesting(DnsSdRegistry* registry); super nit: s/Testing/Test, here and below. https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.cc (right): https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.cc:10: : DnsSdRegistry(NULL), observer_(observer) {} nullptr
https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:88: cast_socket_service_ = cast_channel::CastSocketServiceFactory::GetInstance() On 2017/06/15 00:50:59, imcheng wrote: > Would we need to add a DependsOn from MediaRouterFactory to > CastSocketServiceFactory (until we make CastSocketService a singleton)? It seems fine since we call cast_channel::CastSocketServiceFactory::GetInstance() explicitly here? https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:125: cast_socket_service_ = cast_socket_service; On 2017/06/15 00:50:56, imcheng wrote: > Add a DCHECK to be consistent with above. Moved this to a ctor used by test. https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:157: cast_socket_service_->OpenSocket( On 2017/06/15 00:50:57, imcheng wrote: > What if the socket already exists (opened by browser or the extension before)? Handled inside CastSocketService::OpenSocket(). https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:165: int channel_id, On 2017/06/15 00:50:56, imcheng wrote: > Would it be possible to pass back the CastSocket in the callback? DISALLOW_COPY_AND_ASSIGN(CastSocketImpl); Do not feel like changing this... https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:190: MediaSinkInternal sink; On 2017/06/15 00:50:56, imcheng wrote: > DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); Done. https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:197: if (!base::ContainsValue(current_services_, service)) { On 2017/06/15 00:50:56, imcheng wrote: > This check means we would drop the sink if we somehow had a OnDnsSdEvent not > containing |service|. If we didn't do that, would the sink be eventually removed > anyway if the channel errors out? DialMediaSinkService has similar logic so copied it here. Yes, if we remove sinks when observing any socket error (not sure if we want to do that, seems a little aggressive) https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:47: void SetDnsSdRegistryForTesting(DnsSdRegistry* registry); On 2017/06/15 00:51:00, imcheng wrote: > super nit: s/Testing/Test, here and below. Done. https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.cc (right): https://codereview.chromium.org/2927833002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/mdns/mock_dns_sd_registry.cc:10: : DnsSdRegistry(NULL), observer_(observer) {} On 2017/06/15 00:51:01, imcheng wrote: > nullptr Done.
Overall looks really good. I didn't review the unit test and will do that in the next pass. I wonder if the thread hopping can be simplified by using base::PostTaskWithTraitsAndReplyWithResult. We might want to see if we can run the discovery code on a thread other than IO so we're not contending with other network requests in the browser. https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta... https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:52: std::string val = item.substr(split_idx + 1); For mDNS TXT records, the value can be optional, but I think for now Cast always assigns a value for each key. Handle the empty-value case with an empty string? https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:439: DVLOG_WITH_INSTANCE(1) << "Provider [" << provider_name << "] fould " Typo in found
LGTM % a few more minor comments. It looks like the patch description could be updated. I don't think this should block on converting from PostTask to TaskSequence, but maybe check in with Derek who has been converting some of our code to see how simple it would be for this patch. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:19: const char kCastServiceType[] = "_googlecast._tcp.local"; constexpr char here and below https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:170: cast_socket_service_->OpenSocket( IIRC the CastSocketService won't add the same observer multiple times for the same ip_endpoint, so if there is already a pending call to OpenSocket this won't do anything. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:89: // Open cast channel on IO thread. nit: Opens https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:111: // Raw pointer to DnsSdRegistry singleton. Can you comment about object lifetime here? I assume the DnsSdRegistry lives as long as the browser process? https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:115: DnsSdRegistry* test_dns_sd_registry_ = nullptr; Does this need to be held separately or can tests just reset |dns_sd_registry_|? https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:25: const char SERVICE_TYPE[] = "_googlecast._tcp.local"; This duplicates the definition in cast_media_sink_service.cc. Consider moving the declaration of kCastServiceType so it can be used here. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:37: service.service_name = "_myDevice." + std::string(SERVICE_TYPE); I thing std::string is optional here. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:108: base::MockTimer* mock_timer_; Consider declaring this as std::unique_ptr and using std::move to pass it below.
lgtm % comments https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:143: current_services_ = services; Does the timer fire if there used to be devices, but they are now all gone? Seems like the timer is only triggered when cast channel successfully opens. Similar question for DialMediaSinkService. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:52: void SetDnsSdRegistryForTest(DnsSdRegistry* registry); Can you set DnsSdRegistry in the test constructor?
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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...
https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:19: const char kCastServiceType[] = "_googlecast._tcp.local"; On 2017/06/28 22:43:13, mark a. foltz wrote: > constexpr char here and below Done. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:52: std::string val = item.substr(split_idx + 1); On 2017/06/28 07:44:52, mark a. foltz wrote: > For mDNS TXT records, the value can be optional, but I think for now Cast always > assigns a value for each key. Handle the empty-value case with an empty string? Done. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:143: current_services_ = services; On 2017/06/29 23:15:44, imcheng wrote: > Does the timer fire if there used to be devices, but they are now all gone? > Seems like the timer is only triggered when cast channel successfully opens. > Similar question for DialMediaSinkService. No, timer is not fired. Added a RestartTimer() at the end of this function to fix this... https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:170: cast_socket_service_->OpenSocket( On 2017/06/28 22:43:13, mark a. foltz wrote: > IIRC the CastSocketService won't add the same observer multiple times for the > same ip_endpoint, so if there is already a pending call to OpenSocket this won't > do anything. Yes, for socket in 'connecting' state, it wont register observer again. But still need to call it because we want to register OnChannelOpenedOnIOThread callback to the socket. We might do a clean up to have CastMediaSinkService/CastChannelAPI own observer objects. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:52: void SetDnsSdRegistryForTest(DnsSdRegistry* registry); On 2017/06/29 23:15:44, imcheng wrote: > Can you set DnsSdRegistry in the test constructor? We are going to keep this function and remove |test_dns_sd_registry_| instead. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:89: // Open cast channel on IO thread. On 2017/06/28 22:43:13, mark a. foltz wrote: > nit: Opens Done. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:111: // Raw pointer to DnsSdRegistry singleton. On 2017/06/28 22:43:13, mark a. foltz wrote: > Can you comment about object lifetime here? I assume the DnsSdRegistry lives as > long as the browser process? Done. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:115: DnsSdRegistry* test_dns_sd_registry_ = nullptr; On 2017/06/28 22:43:13, mark a. foltz wrote: > Does this need to be held separately or can tests just reset |dns_sd_registry_|? Yes, we can remove this and let SetDnsSdRegistryForTest() add observer for test registry. If we do it this way, start/restart/stop unit test does not add much value... https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:25: const char SERVICE_TYPE[] = "_googlecast._tcp.local"; On 2017/06/28 22:43:13, mark a. foltz wrote: > This duplicates the definition in cast_media_sink_service.cc. Consider moving > the declaration of kCastServiceType so it can be used here. Done. https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:37: service.service_name = "_myDevice." + std::string(SERVICE_TYPE); On 2017/06/28 22:43:13, mark a. foltz wrote: > I thing std::string is optional here. Compile error if removed...cannot add 'const char*' to 'const char*' https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:108: base::MockTimer* mock_timer_; On 2017/06/28 22:43:13, mark a. foltz wrote: > Consider declaring this as std::unique_ptr and using std::move to pass it below. Ownership of |mock_timer_| is passed into MediaSinkServiceBase. It seems not be able to change to unique_ptr here... https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:439: DVLOG_WITH_INSTANCE(1) << "Provider [" << provider_name << "] fould " On 2017/06/28 07:44:52, mark a. foltz wrote: > Typo in found Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 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.
lgtm https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc (right): https://codereview.chromium.org/2927833002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:143: current_services_ = services; On 2017/07/05 18:01:36, zhaobin wrote: > On 2017/06/29 23:15:44, imcheng wrote: > > Does the timer fire if there used to be devices, but they are now all gone? > > Seems like the timer is only triggered when cast channel successfully opens. > > Similar question for DialMediaSinkService. > > No, timer is not fired. Added a RestartTimer() at the end of this function to > fix this... Ok. Does we still need to call RestartTimer() in OnChannelOpenedOnUIThread?
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 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 mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2927833002/#ps200001 (title: "merge with master")
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": 200001, "attempt_start_ts": 1499449630876280,
"parent_rev": "d010336cd3d39d39a5500193d4bbf86ed49bc3d5", "commit_rev":
"b9a2a9b60034d3ee2138c4815bc3c66b13cee3b5"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add CastMediaSinkService NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands Discover process is similar to DialMediaSinkService: - CastMediaSinkService::Start() registers itself with DnsRegistry - CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer - Start opening channel for each service on IO thread - CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes - Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread() - Create MediaSinkInternal and store it in CastSinkService's sink map - Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires TODO: Unit test Implement CastSocketService::OpenChannel BUG=687377 ========== to ========== [Media Router] Add CastMediaSinkService NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands Discover process is similar to DialMediaSinkService: - CastMediaSinkService::Start() registers itself with DnsRegistry - CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer - Start opening channel for each service on IO thread - CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes - Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread() - Create MediaSinkInternal and store it in CastSinkService's sink map - Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires TODO: Unit test Implement CastSocketService::OpenChannel BUG=687377 Review-Url: https://codereview.chromium.org/2927833002 Cr-Commit-Position: refs/heads/master@{#484979} Committed: https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2973983002/ by bsep@chromium.org. The reason for reverting is: Causing unit_tests failures on multiple bots For example: https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests/... https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests... https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20TSan%....
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add CastMediaSinkService NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands Discover process is similar to DialMediaSinkService: - CastMediaSinkService::Start() registers itself with DnsRegistry - CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer - Start opening channel for each service on IO thread - CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes - Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread() - Create MediaSinkInternal and store it in CastSinkService's sink map - Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires TODO: Unit test Implement CastSocketService::OpenChannel BUG=687377 Review-Url: https://codereview.chromium.org/2927833002 Cr-Commit-Position: refs/heads/master@{#484979} Committed: https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3... ========== to ========== [Media Router] Add CastMediaSinkService NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands Discover process is similar to DialMediaSinkService: - CastMediaSinkService::Start() registers itself with DnsRegistry - CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer - Start opening channel for each service on IO thread - CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes - Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread() - Create MediaSinkInternal and store it in CastSinkService's sink map - Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires BUG=687377 Review-Url: https://codereview.chromium.org/2927833002 Cr-Commit-Position: refs/heads/master@{#484979} Committed: https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3... ==========
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 mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2927833002/#ps220001 (title: "fix tsan and mac unit test failures")
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": 220001, "attempt_start_ts": 1499472910507040,
"parent_rev": "e16032626e22e7ab551070c6260f96876f7179ea", "commit_rev":
"ab578bcef0800484e95d59b20a2d49ebbb5467ab"}
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499472910507040,
"parent_rev": "f62169afa7f911143f50059c52ac0d9442284e96", "commit_rev":
"59516e7383c4e6e0231ca54d664f8fa0ea039af0"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add CastMediaSinkService NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands Discover process is similar to DialMediaSinkService: - CastMediaSinkService::Start() registers itself with DnsRegistry - CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer - Start opening channel for each service on IO thread - CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes - Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread() - Create MediaSinkInternal and store it in CastSinkService's sink map - Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires BUG=687377 Review-Url: https://codereview.chromium.org/2927833002 Cr-Commit-Position: refs/heads/master@{#484979} Committed: https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3... ========== to ========== [Media Router] Add CastMediaSinkService NOTE: those extensions header files and extensions::api:: namespace will go away when https://codereview.chromium.org/2913033003/ lands Discover process is similar to DialMediaSinkService: - CastMediaSinkService::Start() registers itself with DnsRegistry - CastMediaSinkService::OnDnsSdEvent() gets invoked when Dns services comes back and starts a 3s timer - Start opening channel for each service on IO thread - CastMediaSinkService::OnChannelOpenedOnIOThread() gets invoked when channel open finishes - Post task back to UI thread via CastMediaSinkService::OnChannelOpenOnUIThread() - Create MediaSinkInternal and store it in CastSinkService's sink map - Invoke MediaSinkBase::FetchCompleted() and send sinks to MRP when timer expires BUG=687377 Review-Url: https://codereview.chromium.org/2927833002 Cr-Original-Commit-Position: refs/heads/master@{#484979} Committed: https://chromium.googlesource.com/chromium/src/+/b9a2a9b60034d3ee2138c4815bc3... Review-Url: https://codereview.chromium.org/2927833002 Cr-Commit-Position: refs/heads/master@{#485110} Committed: https://chromium.googlesource.com/chromium/src/+/59516e7383c4e6e0231ca54d664f... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/59516e7383c4e6e0231ca54d664f...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 484979 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
