Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837363002:
[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl (Closed)
|
Created:
3 years, 8 months ago by zhaobin Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, feature-media-reviews_chromium.org, media-router+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] Use DialMediaSinkServiceDelegate in MediaRouterMojoImpl
- Added DialMediaSinkServiceDelegate to handle thread hopping between IO/UI thread
- Added MediaRouterMojoImpl::StartDiscovery()
TODO:
- Call StartDiscovery() in MediaRouterMojoImpl::RegisterMediaRouteProvider()
BUG=687375
Review-Url: https://codereview.chromium.org/2837363002
Cr-Commit-Position: refs/heads/master@{#475230}
Committed: https://chromium.googlesource.com/chromium/src/+/ff8d8838d10847c5c41b7e7bc6ee488ccebbcda2
Patch Set 1 #
Total comments: 8
Patch Set 2 : rebase with upstream patch #Patch Set 3 : resolve code review comments from Mark #
Total comments: 20
Patch Set 4 : resolve code review comments from Mark #
Total comments: 42
Patch Set 5 : resolve code review comments from Kevin #
Total comments: 15
Patch Set 6 : resolve code review comments from Kevin #Patch Set 7 : merge with master #Patch Set 8 : fix windows unit tests failures #
Total comments: 20
Patch Set 9 : add dial_media_sink_service_delegate #
Total comments: 32
Patch Set 10 : rebase with master #Patch Set 11 : resolve code review comments from Mark #Patch Set 12 : resolve code review comments from Mark #
Total comments: 19
Patch Set 13 : resolve code review comments from Kevin #Patch Set 14 : resolve code review comments from Kevin #
Total comments: 6
Patch Set 15 : resolve code review comments from Mark #Patch Set 16 : rebase with master #
Total comments: 6
Patch Set 17 : resolve code review comments from Mark #Patch Set 18 : fix android compile error #
Total comments: 12
Patch Set 19 : resolve code review comments from Kevin #
Total comments: 6
Patch Set 20 : resolve code review comments from Kevin #
Total comments: 16
Patch Set 21 : resolve code review comments from Derek #Messages
Total messages: 79 (42 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
Overall looks good, a couple of comments https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:54: "EnableBrowserSideDiscovery", base::FEATURE_DISABLED_BY_DEFAULT}; EnableDialLocalDiscovery ? https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.h:399: #if !defined(OS_ANDROID) Do we even compile MediaRouterMojoImpl for Android? I seem to remember we excluded it in BUILD.gn. https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.h:462: bool enable_browser_side_discovery_; We probably want flags for individual providers (DIAL and Cast) as we will want to control migration individually. https://codereview.chromium.org/2837363002/diff/1/chrome/common/media_router/... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2837363002/diff/1/chrome/common/media_router/... chrome/common/media_router/mojo/media_router.mojom:426: (string instance_id, bool enable_browser_side_discovery); As I mentioned elsewhere passing a mojo struct like MediaRouteProviderConfig { bool enable_dial_discovery; bool enable_cast_discovery; } to pass flags from the browser to the provider would be more extensible. It would also allow the bindings to just pass the mojo config object directly into the MRPM, instead of keeping flags.
Looks good overall - a few comments. Please update description when you get a chance :-) https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:105: auto* profile = Profile::FromBrowserContext(context); Can the rest of this method be factored into StartDiscoveryOnIOThread()? https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:181: !base::FeatureList::IsEnabled(kEnableDialLocalDiscovery); Nice :) https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1072: DVLOG_WITH_INSTANCE(1) << "StartBrowserSideDiscovery"; DCHECK that this is running on the IO thread. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1077: dial_media_sink_service_.reset(new DialMediaSinkService( = MakeUnique<>() https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:51: extern const base::Feature kEnableDialLocalDiscovery; Can these go in media_router_feature.cc and exposed through an API in media_router_feature.h? https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:52: extern const base::Feature kEnableCastDiscovery; Is it okay to do static initialization of base::Feature? https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:403: void StartBrowserSideDiscovery(net::URLRequestContextGetter* request_context); Just StartDiscovery()? Since this is running in the MR it must be running in the current (browser) process. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:405: // Invoked when browser side sink disocvery finished. Typo in discovery https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:480: content::BrowserThread::DeleteOnIOThread> There could be a shutdown race condition here - this assumes the IO thread is still alive at the time the MediaRouter keyed service is destroyed. Not a huge deal, but this dtor may DCHECK if there is no IO thread to post the task to. https://codereview.chromium.org/2837363002/diff/40001/chrome/common/media_rou... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2837363002/diff/40001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:192: // if browser side DIAL device discovery is enabled. Would it make more sense to reverse the sense here, because this is a configuration being passed *to* the MRP? E.g., enable_dial_discovery means the MRP should enable discovery in the component extension.
Make DialMediaSinkService RefCountedThreadSafe and handle post tasks between IO/UI thread. https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:54: "EnableBrowserSideDiscovery", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/04/26 23:52:55, mark a. foltz wrote: > EnableDialLocalDiscovery ? Done. https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.h:399: #if !defined(OS_ANDROID) On 2017/04/26 23:52:55, mark a. foltz wrote: > Do we even compile MediaRouterMojoImpl for Android? I seem to remember we > excluded it in BUILD.gn. Done. https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.h:462: bool enable_browser_side_discovery_; On 2017/04/26 23:52:55, mark a. foltz wrote: > We probably want flags for individual providers (DIAL and Cast) as we will want > to control migration individually. Done. https://codereview.chromium.org/2837363002/diff/1/chrome/common/media_router/... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2837363002/diff/1/chrome/common/media_router/... chrome/common/media_router/mojo/media_router.mojom:426: (string instance_id, bool enable_browser_side_discovery); On 2017/04/26 23:52:56, mark a. foltz wrote: > As I mentioned elsewhere passing a mojo struct like > > MediaRouteProviderConfig { > bool enable_dial_discovery; > bool enable_cast_discovery; > } > > to pass flags from the browser to the provider would be more extensible. It > would also allow the bindings to just pass the mojo config object directly into > the MRPM, instead of keeping flags. Done. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:105: auto* profile = Profile::FromBrowserContext(context); On 2017/05/01 20:32:57, mark a. foltz wrote: > Can the rest of this method be factored into StartDiscoveryOnIOThread()? Done. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:181: !base::FeatureList::IsEnabled(kEnableDialLocalDiscovery); On 2017/05/01 20:32:57, mark a. foltz wrote: > Nice :) Acknowledged. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1072: DVLOG_WITH_INSTANCE(1) << "StartBrowserSideDiscovery"; On 2017/05/01 20:32:57, mark a. foltz wrote: > DCHECK that this is running on the IO thread. Done. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1077: dial_media_sink_service_.reset(new DialMediaSinkService( On 2017/05/01 20:32:57, mark a. foltz wrote: > = MakeUnique<>() dial_media_sink_service_ is a scoped_refptr now... https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:51: extern const base::Feature kEnableDialLocalDiscovery; On 2017/05/01 20:32:57, mark a. foltz wrote: > Can these go in media_router_feature.cc and exposed through an API in > media_router_feature.h? Done. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:52: extern const base::Feature kEnableCastDiscovery; On 2017/05/01 20:32:57, mark a. foltz wrote: > Is it okay to do static initialization of base::Feature? Code removed. Seems fine. https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?type=cs https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:403: void StartBrowserSideDiscovery(net::URLRequestContextGetter* request_context); On 2017/05/01 20:32:57, mark a. foltz wrote: > Just StartDiscovery()? Since this is running in the MR it must be running in > the current (browser) process. Done. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:405: // Invoked when browser side sink disocvery finished. On 2017/05/01 20:32:57, mark a. foltz wrote: > Typo in discovery Done. https://codereview.chromium.org/2837363002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:480: content::BrowserThread::DeleteOnIOThread> On 2017/05/01 20:32:57, mark a. foltz wrote: > There could be a shutdown race condition here - this assumes the IO thread is > still alive at the time the MediaRouter keyed service is destroyed. Not a huge > deal, but this dtor may DCHECK if there is no IO thread to post the task to. > Per discussion with Derek, we are going to make DialMediaSinkService a refcounted thread safe class DeleteOnIOThread. https://codereview.chromium.org/2837363002/diff/40001/chrome/common/media_rou... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2837363002/diff/40001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:192: // if browser side DIAL device discovery is enabled. On 2017/05/01 20:32:57, mark a. foltz wrote: > Would it make more sense to reverse the sense here, because this is a > configuration being passed *to* the MRP? > > E.g., enable_dial_discovery means the MRP should enable discovery in the > component extension. Done.
Description was changed from ========== [Media Router] Use DialMediaSinkService in MediaRouterMojoImpl - Add kEnableBrowserSideDiscovery feature to turn on/off browser side discovery feature and pass it to MRP - StartBrowserSideDiscovery() in MediaRouterMojoImpl::BindToRequest() cl/154204127 handles enable_browser_side_discovery flag in extension side. BUG=687375 ========== to ========== [Media Router] Use DialMediaSinkService in MediaRouterMojoImpl - Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off browser side DIAL and Cast discovery feature and pass it to MRP - StartDiscovery() in MediaRouterMojoImpl::RegisterMediaRouteProvider() cl/154204127 handles enable_dial_discovery and enable_dial_discovery flag in extension side. BUG=687375 ==========
imcheng@chromium.org changed reviewers: + kmarshall@chromium.org
+kmarshall to review. Thanks Kevin!
https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:76: auto sinks = current_sinks_; Can you just work with current_sinks_ and mrp_sinks_ throughout this function here, instead of making a copy? https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:95: finish_timer_.reset(new base::OneShotTimer()); Can we reuse the finish_timer object instead of allocating new ones on each DIAL event? Also, consider making the timer support dependency injection, so that test code can pass in a MockTimer. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:25: // This class is thread safe. It is created on UI thread and destroyed on IO * If it was threadsafe then one could use it on any thread, which is the opposite of how this is implemented? If that's the case, then what about using "thread affine" instead of "thread safe"? * Add comments regarding thread affinity to the method calls themselves. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:27: class DialMediaSinkService : public base::RefCountedThreadSafe< totally low pri nit: move RefCountedThreadSafe to the end of the list for readability. IMO it's the least interesting override from a reader's POV - they're going to be more interested in the base classes that affect the class' method signatures. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:39: void Stop(); Move non-overrides above overrides. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:44: // Returns instance of device registry. Document why this is protected virtual? (testing, right?) Nit: it might be better to set these via ForTest() injection methods instead of defining test subclasses (composition preferred over inheritance) https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:95: ~TestDialMediaSinkService() override {} Is this necessary? The object will be deleted from inside a scoped_refptr. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:111: DialMediaSinkService* media_sink_service() { Getter method isn't necessary? Can just use arrow operator on media_sink_service_ directly https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:117: base::RunLoop run_loop; Nit.. *The runloop doesn't have to be in scope when tasks are posted. *You can call RunUntilIdle on a RunLoop rvalue at the end of the function, e.g. base::RunLoop().RunUntilIdle() here and elsewhere https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:123: media_sink_service()->OnFetchCompleted(); Also, see my previous comment about Timer dependency injection. Working with the timer is a higher fidelity test than spoofing calls to media_sink_service::OnFetchCompleted(). https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:143: scoped_refptr<TestDialMediaSinkService> media_sink_service_; DISALLOW_COPY_AND_ASSIGN(DialMediaSinkServiceTest); https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:166: EXPECT_TRUE(media_sink_service()->finish_timer_->IsRunning()); No need to use a getter function; you can use the arrow operator on the field. e.g."media_sink_service_->finish_timer_->IsRunning()" https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.h:78: // are registered event listeners. Virtual for testing? https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:77: context_ = context; Put this in the initialization list? https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:160: config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled(); Eh??? This doesn't make sense - you're setting an enabled flag to the negative of another enabled flag?? https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:219: explicit MediaRouterMojoImpl(extensions::EventPageTracker* event_page_tracker, Remove "explicit" (>1 arg ctor) https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_test.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:168: mojom::MediaRouteProviderConfigPtr config) { This is to deal with move-only |config|, right? Maybe add a little comment to that effect here. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:208: std::unique_ptr<TestingProfile> profile_; Why is this a unique_ptr? https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:209: std::unique_ptr<MediaRouterMojoImpl> mock_media_router_; Ditto here https://codereview.chromium.org/2837363002/diff/60001/chrome/common/media_rou... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:190: // Pass feature flags from browser to Media Route Provider Used to pass feature configuration data from the Browser to the Media Route Provider. https://codereview.chromium.org/2837363002/diff/60001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:192: // If the MRP should enable DIAL discovery in component extension. Nit: It seems like the component extension part is already implied by the "MRP" subject in this sentence. Remove that part?
https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:76: auto sinks = current_sinks_; On 2017/05/03 21:07:27, Kevin M wrote: > Can you just work with current_sinks_ and mrp_sinks_ throughout this function > here, instead of making a copy? Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:95: finish_timer_.reset(new base::OneShotTimer()); On 2017/05/03 21:07:27, Kevin M wrote: > Can we reuse the finish_timer object instead of allocating new ones on each DIAL > event? > > Also, consider making the timer support dependency injection, so that test code > can pass in a MockTimer. Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:25: // This class is thread safe. It is created on UI thread and destroyed on IO On 2017/05/03 21:07:27, Kevin M wrote: > * If it was threadsafe then one could use it on any thread, which is the > opposite of how this is implemented? If that's the case, then what about using > "thread affine" instead of "thread safe"? > > * Add comments regarding thread affinity to the method calls themselves. Methods in this class can be invoked on both UI and IO thread so it is thread safe? Remove the second half to avoid confusion. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:27: class DialMediaSinkService : public base::RefCountedThreadSafe< On 2017/05/03 21:07:27, Kevin M wrote: > totally low pri nit: move RefCountedThreadSafe to the end of the list for > readability. IMO it's the least interesting override from a reader's POV - > they're going to be more interested in the base classes that affect the class' > method signatures. Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:39: void Stop(); On 2017/05/03 21:07:27, Kevin M wrote: > Move non-overrides above overrides. Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:44: // Returns instance of device registry. On 2017/05/03 21:07:27, Kevin M wrote: > Document why this is protected virtual? (testing, right?) > > Nit: it might be better to set these via ForTest() injection methods instead of > defining test subclasses (composition preferred over inheritance) Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:95: ~TestDialMediaSinkService() override {} On 2017/05/03 21:07:27, Kevin M wrote: > Is this necessary? The object will be deleted from inside a scoped_refptr. Code removed. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:111: DialMediaSinkService* media_sink_service() { On 2017/05/03 21:07:27, Kevin M wrote: > Getter method isn't necessary? Can just use arrow operator on > media_sink_service_ directly Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:117: base::RunLoop run_loop; On 2017/05/03 21:07:27, Kevin M wrote: > Nit.. > > *The runloop doesn't have to be in scope when tasks are posted. > *You can call RunUntilIdle on a RunLoop rvalue at the end of the function, e.g. > > base::RunLoop().RunUntilIdle() > > > here and elsewhere Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:123: media_sink_service()->OnFetchCompleted(); On 2017/05/03 21:07:27, Kevin M wrote: > Also, see my previous comment about Timer dependency injection. Working with the > timer is a higher fidelity test than spoofing calls to > media_sink_service::OnFetchCompleted(). This aims to test OnFetchCompleted() function only. Will create a new function to test timer. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:143: scoped_refptr<TestDialMediaSinkService> media_sink_service_; On 2017/05/03 21:07:27, Kevin M wrote: > DISALLOW_COPY_AND_ASSIGN(DialMediaSinkServiceTest); Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:166: EXPECT_TRUE(media_sink_service()->finish_timer_->IsRunning()); On 2017/05/03 21:07:27, Kevin M wrote: > No need to use a getter function; you can use the arrow operator on the field. > > e.g."media_sink_service_->finish_timer_->IsRunning()" > Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.h:78: // are registered event listeners. On 2017/05/03 21:07:27, Kevin M wrote: > Virtual for testing? Yes... https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:77: context_ = context; On 2017/05/03 21:07:27, Kevin M wrote: > Put this in the initialization list? Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:160: config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled(); On 2017/05/03 21:07:27, Kevin M wrote: > Eh??? This doesn't make sense - you're setting an enabled flag to the negative > of another enabled flag?? Yes. config->enable_dial_discovery controls extension discovery; media_router::DialLocalDiscoveryEnabled() controls browser side discovery. If browser side discovery is enabled, disable extension discovery. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:219: explicit MediaRouterMojoImpl(extensions::EventPageTracker* event_page_tracker, On 2017/05/03 21:07:28, Kevin M wrote: > Remove "explicit" (>1 arg ctor) Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_test.h (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:168: mojom::MediaRouteProviderConfigPtr config) { On 2017/05/03 21:07:28, Kevin M wrote: > This is to deal with move-only |config|, right? Maybe add a little comment to > that effect here. Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:208: std::unique_ptr<TestingProfile> profile_; On 2017/05/03 21:07:28, Kevin M wrote: > Why is this a unique_ptr? Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:209: std::unique_ptr<MediaRouterMojoImpl> mock_media_router_; On 2017/05/03 21:07:28, Kevin M wrote: > Ditto here It was uniqe_ptr before. Leave it as it it... https://codereview.chromium.org/2837363002/diff/60001/chrome/common/media_rou... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2837363002/diff/60001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:190: // Pass feature flags from browser to Media Route Provider On 2017/05/03 21:07:28, Kevin M wrote: > Used to pass feature configuration data from the Browser to the Media Route > Provider. Done. https://codereview.chromium.org/2837363002/diff/60001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:192: // If the MRP should enable DIAL discovery in component extension. On 2017/05/03 21:07:28, Kevin M wrote: > Nit: It seems like the component extension part is already implied by the "MRP" > subject in this sentence. Remove that part? Done.
https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:25: dial_registry_(nullptr), Suggestion: use inline setters in header file. dial_registry_ = nullptr; https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:111: if (!finish_timer_) Could we move these conditional object creation calls to StartOnIOThread() instead? It would consolidate the dependency creation code and reduce code duplication (finish_timer_ is checked twice by different methods). https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:34: void Stop(); Add a comment for this method. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:85: mock_description_service_ = new MockDeviceDescriptionService( Best practice point: try to bind pointers to unique_ptrs at allocation time. Otherwise there is a window of time in which the pointer is unowned, which could lead to regressions in the future if someone refactored the code (e.g. adding an exception handler.) So: auto mock_description_service = base::MakeUniuqe<MockDeviceDescriptionService>(...); mock_description_service_ = mock_description_service.get(); mss->SetForTest(std::move(mock_description_service); https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:90: new base::MockTimer(true /*retain_user_task*/, false /*is_repeating*/); Suggestion: remove inline comments. A 2-bool function signature ought to be simple enough to not need extra annotations. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_feature.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_feature.cc:22: const base::Feature kEnableDialLocalDiscovery{ Class-typed variables (like Feature) cannot have static storage duration; they can't be used as kConstants. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_test.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.cc:29: RegisterMediaRouteProviderHandler::RegisterMediaRouteProviderHandler() {} Separate these with newlines
Getting close, these are mostly nits :)
https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:90: new base::MockTimer(true /*retain_user_task*/, false /*is_repeating*/); On 2017/05/08 at 18:22:19, Kevin M wrote: > Suggestion: remove inline comments. A 2-bool function signature ought to be simple enough to not need extra annotations. Please keep them. Unlike a simple setter, MockTimer() gives no clue as to what its arguments mean. This is when inline comments are most helpful. It usually means the designer of the class should have used static factories with descriptive names or a builder pattern instead of lists-of-boolean-initializers.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:25: dial_registry_(nullptr), On 2017/05/08 18:22:19, Kevin M wrote: > Suggestion: use inline setters in header file. > > dial_registry_ = nullptr; Done. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:111: if (!finish_timer_) On 2017/05/08 18:22:19, Kevin M wrote: > Could we move these conditional object creation calls to StartOnIOThread() > instead? It would consolidate the dependency creation code and reduce code > duplication (finish_timer_ is checked twice by different methods). Done. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:34: void Stop(); On 2017/05/08 18:22:19, Kevin M wrote: > Add a comment for this method. Done. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:85: mock_description_service_ = new MockDeviceDescriptionService( On 2017/05/08 18:22:19, Kevin M wrote: > Best practice point: try to bind pointers to unique_ptrs at allocation time. > Otherwise there is a window of time in which the pointer is unowned, which could > lead to regressions in the future if someone refactored the code (e.g. adding an > exception handler.) > > So: > > auto mock_description_service = > base::MakeUniuqe<MockDeviceDescriptionService>(...); > mock_description_service_ = mock_description_service.get(); > mss->SetForTest(std::move(mock_description_service); Done. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:90: new base::MockTimer(true /*retain_user_task*/, false /*is_repeating*/); On 2017/05/08 18:29:02, mark a. foltz wrote: > On 2017/05/08 at 18:22:19, Kevin M wrote: > > Suggestion: remove inline comments. A 2-bool function signature ought to be > simple enough to not need extra annotations. > > Please keep them. Unlike a simple setter, MockTimer() gives no clue as to what > its arguments mean. This is when inline comments are most helpful. > > It usually means the designer of the class should have used static factories > with descriptive names or a builder pattern instead of > lists-of-boolean-initializers. Acknowledged. https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_feature.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_feature.cc:22: const base::Feature kEnableDialLocalDiscovery{ On 2017/05/08 18:22:19, Kevin M wrote: > Class-typed variables (like Feature) cannot have static storage duration; they > can't be used as kConstants. > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables chrome_features.cc uses it this way... https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?rcl=5ca... https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_test.cc (right): https://codereview.chromium.org/2837363002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.cc:29: RegisterMediaRouteProviderHandler::RegisterMediaRouteProviderHandler() {} On 2017/05/08 18:22:19, Kevin M wrote: > Separate these with newlines 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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:85: if (!dial_registry_) Assuming the service was Start()ed, then is it possible for this to be null? Same for the calls to DCHECK(finish_timer_) - those seem unnecessary too. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:113: base::TimeDelta finish_delay = Suggestion: refactor these calls into a method "ResetTimer()"? https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:113: base::TimeDelta finish_delay = This code resets the timer for every DIAL event, so FetchCompleted is called N seconds after the *last* record. OnDeviceDescriptionAvailable does not reset the timer once it's running, so FetchCompleted is called N seconds after the *first* record. Is this intentional? https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1091: base::Unretained(this), "dial"), Are there potential lifetime issues in giving an refcounted object an Unretained callback into |this|? * OnFetchCompleted() called * Task posted for OnFetchCompletedWithUiThread() bound to refcounted MediaSinkService. (Refcount for MSS is now 2.) * MediaRouterMojoImpl destroyed. (MediaSinkService NOT destroyed, because its refcount is still positive!) * MSS::OnFetchCompletedOnUIThread invoked. * MSS calls sink discovery callback to MediaRouterMojoImpl. * Use after free crash. If you agree with this assessment, then a WeakPtr might be in order here.
It looks like the threading was redesigned in DialMediaSinkService and some threading issues have cropped up in the current PS. Is it necessary to expose the service on the UI thread or would it be possible to run all the services on the IO thread, and use some higher level API to transfer sink lists back to the MR? Maybe this deserves a separate discussion. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:29: DialMediaSinkService::~DialMediaSinkService() {} There needs to be some guarantees that the following have or will happen when this dtor is called: - Objects created on the IO thread are destroyed. - Listener/observer removed from DialRegistry on the IO thread (e.g. by calling Stop()), otherwise the DialRegistry may use |this| after free. The DeviceDescriptionService and Timer should either be deleted already, or have a task posted to the IO thread to delete them, instead of allowing their dtors to run on the UI thread (unless they are truly threadsafe or refcounted). https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:119: current_sinks_.clear(); This is being called on the IO thread, but current_sinks_ was allocated on the UI thread. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:155: current_sinks_.insert(MediaSinkInternal(sink, extra_data)); Ditto. This class could be redesigned to pass these results to a task posted on the UI thread that updates |current_sinks_|. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:25: // This class is thread safe. Yes, but the public API (including the OnSinksDiscoveredCallback) must be called on the UI thread because of the DCHECKs(). It would be important to mention that. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:98: std::unique_ptr<DeviceDescriptionService> description_service_; I didn't see where this was set; is this only set for tests? If so, name it test_description_service_ to be specific. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:101: DialRegistry* dial_registry_ = nullptr; What guarantees that the DialRegistry will outlive this object?
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
Resolved dial_media_sink_service related comments in a seperate patch. Added dial_media_sink_service_delegate to handle thread hopping between IO/UI thread and revert most changes in dial_media_sink_service. Splitting into smaller patches: [Media Router] Add features to control browser side discovery https://codereview.chromium.org/2873893003/ [Media Router] Add some SetForTest() functions to DialMediaSinkService and update unit tests https://codereview.chromium.org/2868263002/ https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:29: DialMediaSinkService::~DialMediaSinkService() {} On 2017/05/09 18:21:13, mark a. foltz wrote: > There needs to be some guarantees that the following have or will happen when > this dtor is called: > - Objects created on the IO thread are destroyed. > - Listener/observer removed from DialRegistry on the IO thread (e.g. by calling > Stop()), otherwise the DialRegistry may use |this| after free. > > The DeviceDescriptionService and Timer should either be deleted already, or have > a task posted to the IO thread to delete them, instead of allowing their dtors > to run on the UI thread (unless they are truly threadsafe or refcounted). Marked DialMediaSinkService as content::BrowserThread::DeleteOnIOThread, it should handle destroying DeviceDescriptionService and Timer on IO thread. Will call Stop in dtor(). https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:85: if (!dial_registry_) On 2017/05/09 17:32:06, Kevin M wrote: > Assuming the service was Start()ed, then is it possible for this to be null? > > Same for the calls to DCHECK(finish_timer_) - those seem unnecessary too. Will change this to DCHECK. DCHECK(xxx) should be ok...release code wont have it, earlier DCHECK crash is easier to debug. We have a lot of DCHECk in presentation API code to ensure pre-conditions. https://cs.chromium.org/chromium/src/content/browser/presentation/presentatio... https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:113: base::TimeDelta finish_delay = On 2017/05/09 17:32:06, Kevin M wrote: > This code resets the timer for every DIAL event, so FetchCompleted is called N > seconds after the *last* record. > > OnDeviceDescriptionAvailable does not reset the timer once it's running, so > FetchCompleted is called N seconds after the *first* record. > > Is this intentional? Yes. DIAL event is not for the *last* record. It returns a device list, instead of single device description. DIAL event (return a list of devices) OnDeviceDescriptionAvailable() OnDeviceDescriptionAvailable() OnDeviceDescriptionAvailable() ... (N seconds) FetchCompleted (timer set on DIAL event) OnDeviceDescriptionAvailable() // did not come back in N seconds ... N seconds FetchCompleted (timer set on OnDeviceDescriptionAvailable()) https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:113: base::TimeDelta finish_delay = On 2017/05/09 17:32:05, Kevin M wrote: > Suggestion: refactor these calls into a method "ResetTimer()"? Done. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:119: current_sinks_.clear(); On 2017/05/09 18:21:13, mark a. foltz wrote: > This is being called on the IO thread, but current_sinks_ was allocated on the > UI thread. Created a delegate to handle thread hopping. DialMediaSinkService lives on IO now :) https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:155: current_sinks_.insert(MediaSinkInternal(sink, extra_data)); On 2017/05/09 18:21:13, mark a. foltz wrote: > Ditto. This class could be redesigned to pass these results to a task posted on > the UI thread that updates |current_sinks_|. Done. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:25: // This class is thread safe. On 2017/05/09 18:21:13, mark a. foltz wrote: > Yes, but the public API (including the OnSinksDiscoveredCallback) must be called > on the UI thread because of the DCHECKs(). It would be important to mention > that. Done. https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:98: std::unique_ptr<DeviceDescriptionService> description_service_; On 2017/05/09 18:21:13, mark a. foltz wrote: > I didn't see where this was set; is this only set for tests? If so, name it > test_description_service_ to be specific. It is set in DialMediaSinkService::GetDescriptionService() https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:101: DialRegistry* dial_registry_ = nullptr; On 2017/05/09 18:21:13, mark a. foltz wrote: > What guarantees that the DialRegistry will outlive this object? DialRegistry is a Leaky Singleton... https://cs.chromium.org/chromium/src/chrome/browser/media/router/discovery/di... https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1091: base::Unretained(this), "dial"), On 2017/05/09 17:32:06, Kevin M wrote: > Are there potential lifetime issues in giving an refcounted object an Unretained > callback into |this|? > > * OnFetchCompleted() called > * Task posted for OnFetchCompletedWithUiThread() bound to refcounted > MediaSinkService. (Refcount for MSS is now 2.) > * MediaRouterMojoImpl destroyed. (MediaSinkService NOT destroyed, because its > refcount is still positive!) > * MSS::OnFetchCompletedOnUIThread invoked. > * MSS calls sink discovery callback to MediaRouterMojoImpl. > * Use after free crash. > > If you agree with this assessment, then a WeakPtr might be in order here. Done.
Description was changed from ========== [Media Router] Use DialMediaSinkService in MediaRouterMojoImpl - Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off browser side DIAL and Cast discovery feature and pass it to MRP - StartDiscovery() in MediaRouterMojoImpl::RegisterMediaRouteProvider() cl/154204127 handles enable_dial_discovery and enable_dial_discovery flag in extension side. BUG=687375 ========== to ========== [Media Router] Use DialMediaSinkServiceDelegate in MediaRouterMojoImpl - Added DialMediaSinkServiceDelegate to handle thread hopping between IO/UI thread - Added MediaRouterMojoImpl::StartDiscovery() TODO: - Call StartDiscovery() in MediaRouterMojoImpl::RegisterMediaRouteProvider() BUG=687375 ==========
LGTM with minor comments addressed. I did have one comment on the object design, although it would require some refactoring that would be simpler in a different patch I think. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc (right): https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:44: dial_media_sink_service_ = std::move(dial_media_sink_service); Ensure this is set only once? E.g., before this line DCHECK(dial_media_sink_service) DCHECK(!dial_media_sink_service_) https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:49: DCHECK_CURRENTLY_ON(BrowserThread::IO); Nit: as this is a private method, may not be necessary to check threading here. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:50: if (!dial_media_sink_service_) { Is Start()/Stop()/Start() okay? https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:56: dial_media_sink_service_->Start(); Do you need to check that the service is not already started? https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:62: dial_media_sink_service_->Stop(); Do you need to check that the service is not already stopped? https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:77: DCHECK_CURRENTLY_ON(BrowserThread::UI); This is only called from a private callback - thread check not necessary. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h (right): https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:24: class DialMediaSinkServiceDelegate Bikeshed: Usually I would see a "delegate" and think of an interface that allows you to implement part of the DialMediaSinkService with another object. In this case, this class is acting as a wrapper for the DialMediaSinkService to allow it to be used on the UI thread. One object design would then be: DialMediaSinkService => virtual interface with Start()/Stop(), calling code is thread-agnostic DialMediaSinkServiceImpl => Implements DialMediaSinkService, runs on IO thread DialMediaSinkServiceProxy => Same as the current Delegate, implements DialMediaSinkService, allows Impl to be constructed and used on any other thread So I would propose naming this DialMediaSinkServiceProxy and (optionally) extracting a DialMediaSinkService interface to capture the common API between this class and the implementation. What do you think? https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:29: // Store |callback| and post task to IO thread. Suggested comment: Starts discovery of DIAL devices on IO thread. |callback| is invoked on the UI thread when sinks are discovered. |request_context| is used for HTTP requests made for discovery. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:33: // Clear |sink_discovery_callback_| and post task to IO thread. Suggest: Stops discovery of DIAL devices on IO thread. The callback passed to Start() is cleared. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:46: // Create |dial_media_sink_service_| if none exists before and start DIAL Creates https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:47: // discvoery. typo in discovery https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:50: // Stop DIAL discovery. Stops https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:57: // Invoke |sink_discovery_callback_| on UI thread. Invokes https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc:42: mock_dial_media_sink_service; = MakeUnique<MockDialMediaSinkService>(...) https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc:70: base::RunLoop().RunUntilIdle(); Nit: This can be put in TearDown(). https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc:82: EXPECT_CALL(mock_sink_discovered_cb_, Run(_)); Nit: .Times(1)
Will refactor object design in another patch and update this patch later. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc (right): https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:44: dial_media_sink_service_ = std::move(dial_media_sink_service); On 2017/05/12 21:37:37, mark a. foltz wrote: > Ensure this is set only once? E.g., before this line > > DCHECK(dial_media_sink_service) > DCHECK(!dial_media_sink_service_) Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:49: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/05/12 21:37:36, mark a. foltz wrote: > Nit: as this is a private method, may not be necessary to check threading here. Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:50: if (!dial_media_sink_service_) { On 2017/05/12 21:37:36, mark a. foltz wrote: > Is Start()/Stop()/Start() okay? Yes. MediaRouterMojoImpl will only do Start()/Stop() though. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:56: dial_media_sink_service_->Start(); On 2017/05/12 21:37:37, mark a. foltz wrote: > Do you need to check that the service is not already started? Handled in unserlying DialMediaSinkService. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:62: dial_media_sink_service_->Stop(); On 2017/05/12 21:37:37, mark a. foltz wrote: > Do you need to check that the service is not already stopped? Handled in unserlying DialMediaSinkService. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.cc:77: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/05/12 21:37:37, mark a. foltz wrote: > This is only called from a private callback - thread check not necessary. Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h (right): https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:24: class DialMediaSinkServiceDelegate On 2017/05/12 21:37:37, mark a. foltz wrote: > Bikeshed: > > Usually I would see a "delegate" and think of an interface that allows you to > implement part of the DialMediaSinkService with another object. > > In this case, this class is acting as a wrapper for the DialMediaSinkService to > allow it to be used on the UI thread. > > One object design would then be: > > DialMediaSinkService => virtual interface with Start()/Stop(), calling code is > thread-agnostic > DialMediaSinkServiceImpl => Implements DialMediaSinkService, runs on IO thread > DialMediaSinkServiceProxy => Same as the current Delegate, implements > DialMediaSinkService, allows Impl to be constructed and used on any other thread > > So I would propose naming this DialMediaSinkServiceProxy and (optionally) > extracting a DialMediaSinkService interface to capture the common API between > this class and the implementation. > > What do you think? > > > > Sure. Will do refactor in another patch and update this patch later. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:29: // Store |callback| and post task to IO thread. On 2017/05/12 21:37:37, mark a. foltz wrote: > Suggested comment: > > Starts discovery of DIAL devices on IO thread. |callback| is invoked on the UI > thread when sinks are discovered. |request_context| is used for HTTP requests > made for discovery. Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:33: // Clear |sink_discovery_callback_| and post task to IO thread. On 2017/05/12 21:37:37, mark a. foltz wrote: > Suggest: > > Stops discovery of DIAL devices on IO thread. The callback passed to Start() is > cleared. Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:46: // Create |dial_media_sink_service_| if none exists before and start DIAL On 2017/05/12 21:37:37, mark a. foltz wrote: > Creates Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:47: // discvoery. On 2017/05/12 21:37:37, mark a. foltz wrote: > typo in discovery Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:50: // Stop DIAL discovery. On 2017/05/12 21:37:37, mark a. foltz wrote: > Stops Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate.h:57: // Invoke |sink_discovery_callback_| on UI thread. On 2017/05/12 21:37:37, mark a. foltz wrote: > Invokes Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc:42: mock_dial_media_sink_service; On 2017/05/12 21:37:38, mark a. foltz wrote: > = MakeUnique<MockDialMediaSinkService>(...) MakeUnique<> cannot take content::BrowserThread::DeleteOnIOThread... https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc:70: base::RunLoop().RunUntilIdle(); On 2017/05/12 21:37:38, mark a. foltz wrote: > Nit: This can be put in TearDown(). Done. https://codereview.chromium.org/2837363002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_delegate_unittest.cc:82: EXPECT_CALL(mock_sink_discovered_cb_, Run(_)); On 2017/05/12 21:37:38, mark a. foltz wrote: > Nit: .Times(1) Done.
https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:16: : MediaSinkService(callback) { This is what I was referring to about making MediaSinkService into a pure virtual interface. The complexity of introducing a subclass seems overkill if all we are doing is storing a callback. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:19: base::Bind(&DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread, I think there might be a deletion race condition here. You might need to have the OnSinksDiscoveredOnIOThread trampoline use a UI threadbound WeakPtr to get back to the UI thread. (seeing base::Unretained() used for cross-thread interactions makes me nervous) https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:52: DCHECK(dial_media_sink_service_); What is the benefit of these methods vs. just posting tasks to MediaSinkService::Start() directly? https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:72: if (sink_discovery_callback_) Can we PostTask to sink_discovery_callback_ directly instead? https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:25: : public MediaSinkService, Can MediaSinkService be made into a 100% pure virtual interface? It is jarring to see members of MediaSinkService implementation appear in the proxy code, like the base class' callback field. Instead of having the proxy responsible for instantiating the concrete service impl object, it can just take the underlying impl object as a ctor argument. Then you don't need a SetForTest() since the product and test code will use the same codepath! https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:32: net::URLRequestContextGetter* request_context); ~DialMediaSinkServiceProxy = default; https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:68: }; nit: DISALLOW_COPY_AND_ASSIGN(DialMediaSinkServiceProxy); https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:69: EXPECT_CALL(*mock_service_, Start()); Verifying that the proxy is invoking a function is really only half of the story for the proxy class. :) Can we have a custom action that verifies that the calls are being run on the I/O thread? I think a custom action might work: EXPECT_CALL(cb, Run(_)) .WillOnce(InvokeWithoutArgs(AssertOnIOThread))
+1 for simplifying the proxy implementation to post tasks directly using the DialMediaSinkService object, versus the onIOThread "trampoline" methods. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:16: : MediaSinkService(callback) { On 2017/05/15 19:56:58, Kevin M wrote: > This is what I was referring to about making MediaSinkService into a pure > virtual interface. The complexity of introducing a subclass seems overkill if > all we are doing is storing a callback. The main purpose is to isolate threading. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:25: : public MediaSinkService, On 2017/05/15 19:56:58, Kevin M wrote: > Can MediaSinkService be made into a 100% pure virtual interface? It is jarring > to see members of MediaSinkService implementation appear in the proxy code, like > the base class' callback field. > > Instead of having the proxy responsible for instantiating the concrete service > impl object, it can just take the underlying impl object as a ctor argument. > Then you don't need a SetForTest() since the product and test code will use the > same codepath! I think refactoring the inheritance is in progress in a different patch. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:32: net::URLRequestContextGetter* request_context); On 2017/05/15 19:56:58, Kevin M wrote: > ~DialMediaSinkServiceProxy = default; That should go in the .cc, as it will invoke the DialMediaSinkService dtor.
https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:16: : MediaSinkService(callback) { On 2017/05/15 19:56:58, Kevin M wrote: > This is what I was referring to about making MediaSinkService into a pure > virtual interface. The complexity of introducing a subclass seems overkill if > all we are doing is storing a callback. Besides, we are going to have CastMediaSinkService in future. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:19: base::Bind(&DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread, On 2017/05/15 19:56:58, Kevin M wrote: > I think there might be a deletion race condition here. You might need to have > the OnSinksDiscoveredOnIOThread trampoline use a UI threadbound WeakPtr to get > back to the UI thread. > > (seeing base::Unretained() used for cross-thread interactions makes me nervous) I think it is safe. |this| owns dial_media_sink_service_, dial_media_sink_service_ just store this callback, does not post it anywhere. If |this| is destroyed, this callback wont be invoked. Inside OnSinksDiscoveredOnIOThread, task is posted with |this|. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:52: DCHECK(dial_media_sink_service_); On 2017/05/15 19:56:58, Kevin M wrote: > What is the benefit of these methods vs. just posting tasks to > MediaSinkService::Start() directly? |this| is RefCountedThreadSafe, MediaSinkService is not. If we post task direclty to MediaSinkService::Start(), |this| and MediaSinkService object could be destroyed before callback is invoked. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:72: if (sink_discovery_callback_) On 2017/05/15 19:56:58, Kevin M wrote: > Can we PostTask to sink_discovery_callback_ directly instead? Similar to above. sink_discovery_callback_ binds to MediaRouterMojoImpl::ProvideSinks(), MediaRouteMojoImpl is not RefCountedThreadSafe. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:25: : public MediaSinkService, On 2017/05/15 19:56:58, Kevin M wrote: > Can MediaSinkService be made into a 100% pure virtual interface? It is jarring > to see members of MediaSinkService implementation appear in the proxy code, like > the base class' callback field. > > Instead of having the proxy responsible for instantiating the concrete service > impl object, it can just take the underlying impl object as a ctor argument. > Then you don't need a SetForTest() since the product and test code will use the > same codepath! Both DialMediaSinkServiceProxy and DialMediaSinkService inherit from MediaServiceSink. They take different sink_discovery_callback_. DialMediaSinkServiceProxy gets MediaRouterMojoImpl::ProvideSinks(), should be invoked on UI thread; DialMediaSinkService gets DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread(), should be invoked on IO thread. This proxy class is RefCountedThreadSafe and handles thread switching between IO and UI so MediaRouterMojoImpl lives on UI thread while DialMediaSinkService lives on IO thread. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:32: net::URLRequestContextGetter* request_context); On 2017/05/15 19:56:58, Kevin M wrote: > ~DialMediaSinkServiceProxy = default; RefCountedThreadSafe class needs private dtor. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:68: }; On 2017/05/15 19:56:58, Kevin M wrote: > nit: DISALLOW_COPY_AND_ASSIGN(DialMediaSinkServiceProxy); Done. https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:69: EXPECT_CALL(*mock_service_, Start()); On 2017/05/15 19:56:58, Kevin M wrote: > Verifying that the proxy is invoking a function is really only half of the story > for the proxy class. :) Can we have a custom action that verifies that the calls > are being run on the I/O thread? > > I think a custom action might work: > > EXPECT_CALL(cb, Run(_)) > .WillOnce(InvokeWithoutArgs(AssertOnIOThread)) Done.
- Changed threading a bit: MediaSinkMojoImpl (mojoImpl) lives on UI thread; DialMediaSinkService (serviceImpl) lives on IO thread; DialMediaSinkServiceProxy (serviceProxy) is created on UI thread, and deleted on IO thread. serviceProxy owns serviceImpl and always outlives serviceImpl. It is safe to pass unretained(this) to serviceImpl. mojoImpl passes weak_ptr to serviceProxy. It is safe if mojoImpl dies before serviceProxy. - StartDiscovery() in mojoImpl if EnableDialLocalDiscovery flag is set.
https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:36: Do you need to reset |sink_discovery_callback_| here? https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:53: base::Unretained(this)), It would be more consistent IMO to pass |this| to base::Bind as it is refcounted. In general passing raw pointers to refcounted objects is discouraged - it subverts the purpose of refcounting which is to manage the lifetime of an object based on every user holding a reference. If the destruction order is as you describe, destroying |dial_media_sink_service| should also delete the last reference held to |this|. However, you would need to explicitly delete DMMS outside the dtor of this class (i.e. in StopOnIOThread()) to avoid a cycle. You could also not make this not refcounted at all. Is there a reason for refcounting? https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:62: dial_media_sink_service_->Stop(); Should you delete dial_media_sink_service_?
https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:36: On 2017/05/17 18:16:20, mark a. foltz wrote: > Do you need to reset |sink_discovery_callback_| here? Leave |sink_discovery_callback| so we can support Start()/Stop()/Start(). Make underlying DialMediaSinkService controls timer in Start()/Stop() so that callback wont be invoked after Stop(). https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:53: base::Unretained(this)), On 2017/05/17 18:16:20, mark a. foltz wrote: > It would be more consistent IMO to pass |this| to base::Bind as it is > refcounted. In general passing raw pointers to refcounted objects is > discouraged - it subverts the purpose of refcounting which is to manage the > lifetime of an object based on every user holding a reference. > > If the destruction order is as you describe, destroying > |dial_media_sink_service| should also delete the last reference held to |this|. > However, you would need to explicitly delete DMMS outside the dtor of this class > (i.e. in StopOnIOThread()) to avoid a cycle. > > You could also not make this not refcounted at all. Is there a reason for > refcounting? Changed to |this| and reset in StopOnIOThread(). https://codereview.chromium.org/2837363002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:62: dial_media_sink_service_->Stop(); On 2017/05/17 18:16:20, mark a. foltz wrote: > Should you delete dial_media_sink_service_? Done.
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM % a couple of minor comments. Nice patch :) https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:76: std::vector<MediaSinkInternal>(sinks.begin(), sinks.end()))); It appears that in MediaServiceBase a copy is already made to pass |sinks| here. It would be better to change the signature for the callback to std::vector<> and use std::move() to transfer internal storage between the callbacks vs. making another copy. https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:10: #include "base/macros.h" You need #includes for base::RefCountedThreadSafe and/or base::DeleteHelper I think https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:68: // to UI thread and invoke |sink_discovery_callback_|; Nit: s/;/./
https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:76: std::vector<MediaSinkInternal>(sinks.begin(), sinks.end()))); On 2017/05/19 18:56:57, mark a. foltz wrote: > It appears that in MediaServiceBase a copy is already made to pass |sinks| here. > It would be better to change the signature for the callback to std::vector<> > and use std::move() to transfer internal storage between the callbacks vs. > making another copy. Done. https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:10: #include "base/macros.h" On 2017/05/19 18:56:57, mark a. foltz wrote: > You need #includes for base::RefCountedThreadSafe and/or base::DeleteHelper I > think Done. https://codereview.chromium.org/2837363002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:68: // to UI thread and invoke |sink_discovery_callback_|; On 2017/05/19 18:56:57, mark a. foltz wrote: > Nit: s/;/./ Done.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
Getting close! One major potential segfaulting issue then it's basically down to nits. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:48: finish_timer_.reset(); This is calling unique_ptr::reset() which will destroy the timer object. Do you mean to call timer->Reset() instead? https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:150: } // namespace media_router Can you add a test case that Starts, Stops, and reStarts the service impl object? That would've caught the unique_ptr segfault bug. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:48: // Start() is cleared. We should document somewhere in this header (maybe at the top?) that Stop() must be called, otherwise the proxy will never die. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:56: const content::TestBrowserThreadBundle thread_bundle_; nit: Why is this const? https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:78: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Nit: use GTest macros in test cases, they are more compatible with the test runner's output. EXPECT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) should work.
https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:48: finish_timer_.reset(); On 2017/05/24 00:52:55, Kevin M wrote: > This is calling unique_ptr::reset() which will destroy the timer object. Do you > mean to call timer->Reset() instead? On purpose. Need to destroy timer so that OnDeviceDescriptionAvailable() after Stop() wont start the timer again. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:150: } // namespace media_router On 2017/05/24 00:52:55, Kevin M wrote: > Can you add a test case that Starts, Stops, and reStarts the service impl > object? That would've caught the unique_ptr segfault bug. Done. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:48: // Start() is cleared. On 2017/05/24 00:52:55, Kevin M wrote: > We should document somewhere in this header (maybe at the top?) that Stop() must > be called, otherwise the proxy will never die. Done. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:56: const content::TestBrowserThreadBundle thread_bundle_; On 2017/05/24 00:52:55, Kevin M wrote: > nit: Why is this const? Done. https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:78: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2017/05/24 00:52:55, Kevin M wrote: > Nit: use GTest macros in test cases, they are more compatible with the test > runner's output. > > EXPECT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) > should work. Done.
https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:48: finish_timer_.reset(); On 2017/05/24 17:45:51, zhaobin wrote: > On 2017/05/24 00:52:55, Kevin M wrote: > > This is calling unique_ptr::reset() which will destroy the timer object. Do > you > > mean to call timer->Reset() instead? > > On purpose. Need to destroy timer so that OnDeviceDescriptionAvailable() after > Stop() wont start the timer again. OnDeviceDescriptionAvailable() will segfault, though - it dereferences the timer pointer which was destroyed by Stop(). This line of code resets the unique_ptr. I think you mean to reset the timer contained by the unique_ptr, so you would use the arrow operator instead: finish_timer_->Reset(); https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:157: media_sink_service_->SetTimerForTest(base::WrapUnique(mock_timer_)); Setting the timer more than once in a test case is substantially different than how the actual production code will behave. I think it covers up the segfaulting bug I referred to earlier in my comments. https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:45: // |this| to underlying DialMediaSinkService and increase ref counter. Needs Nit: that level of detail is unnecessary, you can just say "Caller is responsible for calling Stop() before destroying this object." Writing that, I feel that it is a strange requirement. Were you able to try using WeakPtrs? Did they not work? https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:70: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Please migrate this to EXPECT_TRUE, too
https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc (right): https://codereview.chromium.org/2837363002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:48: finish_timer_.reset(); On 2017/05/24 18:13:29, Kevin M wrote: > On 2017/05/24 17:45:51, zhaobin wrote: > > On 2017/05/24 00:52:55, Kevin M wrote: > > > This is calling unique_ptr::reset() which will destroy the timer object. Do > > you > > > mean to call timer->Reset() instead? > > > > On purpose. Need to destroy timer so that OnDeviceDescriptionAvailable() after > > Stop() wont start the timer again. > > OnDeviceDescriptionAvailable() will segfault, though - it dereferences the timer > pointer which was destroyed by Stop(). > > This line of code resets the unique_ptr. I think you mean to reset the timer > contained by the unique_ptr, so you would use the arrow operator instead: > > finish_timer_->Reset(); No segfault in the latest patch. https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:157: media_sink_service_->SetTimerForTest(base::WrapUnique(mock_timer_)); On 2017/05/24 18:13:29, Kevin M wrote: > Setting the timer more than once in a test case is substantially different than > how the actual production code will behave. I think it covers up the segfaulting > bug I referred to earlier in my comments. We create/destroy timer and registry in real Start()/Stop(). Setting mock objects for them change the actual behavior a little, since we decide to use composit to do test instead of inheritence. I did not see it too terrible to set a mock_timer_ here though. https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:45: // |this| to underlying DialMediaSinkService and increase ref counter. Needs On 2017/05/24 18:13:29, Kevin M wrote: > Nit: that level of detail is unnecessary, you can just say "Caller is > responsible for calling Stop() before destroying this object." > > Writing that, I feel that it is a strange requirement. Were you able to try > using WeakPtrs? Did they not work? Done. https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc (right): https://codereview.chromium.org/2837363002/diff/390001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy_unittest.cc:70: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2017/05/24 18:13:29, Kevin M wrote: > Please migrate this to EXPECT_TRUE, too Done.
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...
lgtm Looks good, thanks Bin!
lgtm Looks good, thanks Bin!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Nice patch! I have a few questions regarding the use of std::move / memory management. Apologies if they were already discussed previously. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:50: if (!dial_media_sink_service_) { Consider adding a DCHECK_CURRENTLY_ON(BrowserThread::IO); here? https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:51: // Need to explicitly delete |dial_media_sink_service_| outside dtor to Does this comment mean that we should always call StopOnIOThread()? It seems dial_media_sink_service_ is being deleted inside the dtor, unless StopOnIOThread() is called. I did not fully understand why we need to use WeakPtr instead of ref-counted in this callback. If |dial_media_sink_service_| is deleted then its reference to DialMediaSinkServiceProxy would be released, which would allow it to be deleted when there are no more refs to it. This can be guaranteed if we always make sure to call StopOnIOThread(). https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:63: if (!dial_media_sink_service_) ditto on DCHECK_CURRENTLY_ON. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:75: base::Bind(sink_discovery_callback_, std::move(sinks))); Why do we need std::move here? base::Bind by default makes a copy of the arguments being passed in to its internal storage. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:78: net::URLRequestContextGetter* request_context_; I think this needs to be a scoped_refptr as well. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:181: std::vector<MediaSinkInternal> sinks) = 0; similar to the other comment about using std::move. This method is used in base::Bind as a base::Callback. So |sinks| is the reference is to the vector that was (safely) copied into the callback's internal storage. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:445: std::move(sinks))); ditto on std::move. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.h:481: content::BrowserContext* context_; content::BrowserContext* const
re: std::move base::Callback now supports rvalue parameters. Since the sink list is simply being passed from callback to callback, moving avoids unnecessary copies. See "Avoiding Copies." https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md
On 2017/05/26 16:24:16, mark a. foltz wrote: > re: std::move > > base::Callback now supports rvalue parameters. Since the sink list is simply > being passed from callback to callback, moving avoids unnecessary copies. See > "Avoiding Copies." > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md I see, thanks. I assume that is also why it was necessary to change the callback signature from const T& to T for the argument being bound, so that std::move actually works when calling base::Bind.
On 2017/05/26 at 18:58:29, imcheng wrote: > On 2017/05/26 16:24:16, mark a. foltz wrote: > > re: std::move > > > > base::Callback now supports rvalue parameters. Since the sink list is simply > > being passed from callback to callback, moving avoids unnecessary copies. See > > "Avoiding Copies." > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md > > I see, thanks. I assume that is also why it was necessary to change the callback signature from const T& to T for the argument being bound, so that std::move actually works when calling base::Bind. Right. Passing as const ref forces a copy to be made at a process/thread/callback boundary.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:50: if (!dial_media_sink_service_) { On 2017/05/26 01:25:59, imcheng wrote: > Consider adding a DCHECK_CURRENTLY_ON(BrowserThread::IO); here? Mark suggests no check for private methods. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:51: // Need to explicitly delete |dial_media_sink_service_| outside dtor to On 2017/05/26 01:25:59, imcheng wrote: > Does this comment mean that we should always call StopOnIOThread()? It seems > dial_media_sink_service_ is being deleted inside the dtor, unless > StopOnIOThread() is called. > > I did not fully understand why we need to use WeakPtr instead of ref-counted in > this callback. If |dial_media_sink_service_| is deleted then its reference to > DialMediaSinkServiceProxy would be released, which would allow it to be deleted > when there are no more refs to it. This can be guaranteed if we always make sure > to call StopOnIOThread(). Done. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:63: if (!dial_media_sink_service_) On 2017/05/26 01:25:59, imcheng wrote: > ditto on DCHECK_CURRENTLY_ON. Mark suggests no check for private methods. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc:75: base::Bind(sink_discovery_callback_, std::move(sinks))); On 2017/05/26 01:25:59, imcheng wrote: > Why do we need std::move here? base::Bind by default makes a copy of the > arguments being passed in to its internal storage. Acknowledged. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h:78: net::URLRequestContextGetter* request_context_; On 2017/05/26 01:26:00, imcheng wrote: > I think this needs to be a scoped_refptr as well. Done. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:181: std::vector<MediaSinkInternal> sinks) = 0; On 2017/05/26 01:26:00, imcheng wrote: > similar to the other comment about using std::move. This method is used in > base::Bind as a base::Callback. So |sinks| is the reference is to the vector > that was (safely) copied into the callback's internal storage. To avoid copy as Mark suggested... https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:445: std::move(sinks))); On 2017/05/26 01:26:00, imcheng wrote: > ditto on std::move. Use std::move to avoid copy as Mark suggested. https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2837363002/diff/410001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.h:481: content::BrowserContext* context_; On 2017/05/26 01:26:00, imcheng wrote: > content::BrowserContext* const 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: 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, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2837363002/#ps430001 (title: "resolve code review comments from Derek")
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": 430001, "attempt_start_ts": 1495854418199110,
"parent_rev": "cf4f838225c132dee06cc21dd6a576347734589b", "commit_rev":
"ff8d8838d10847c5c41b7e7bc6ee488ccebbcda2"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Use DialMediaSinkServiceDelegate in MediaRouterMojoImpl - Added DialMediaSinkServiceDelegate to handle thread hopping between IO/UI thread - Added MediaRouterMojoImpl::StartDiscovery() TODO: - Call StartDiscovery() in MediaRouterMojoImpl::RegisterMediaRouteProvider() BUG=687375 ========== to ========== [Media Router] Use DialMediaSinkServiceDelegate in MediaRouterMojoImpl - Added DialMediaSinkServiceDelegate to handle thread hopping between IO/UI thread - Added MediaRouterMojoImpl::StartDiscovery() TODO: - Call StartDiscovery() in MediaRouterMojoImpl::RegisterMediaRouteProvider() BUG=687375 Review-Url: https://codereview.chromium.org/2837363002 Cr-Commit-Position: refs/heads/master@{#475230} Committed: https://chromium.googlesource.com/chromium/src/+/ff8d8838d10847c5c41b7e7bc6ee... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:430001) as https://chromium.googlesource.com/chromium/src/+/ff8d8838d10847c5c41b7e7bc6ee... |
