Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2264153002:
[Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side (Closed)
|
Created:
4 years, 4 months ago by takumif Modified:
4 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side
Relevant CL:
https://codereview.chromium.org/2174693004/
This CL:
- MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager
- QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode
- QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode
- QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode
- QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode
- Add/fix unit tests for QueryResultManager and MediaRouterUI
- Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it).
BUG=627655
Committed: https://crrev.com/4f8671de022b8908686fc5667039c4288a7eeb70
Cr-Commit-Position: refs/heads/master@{#421412}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Create CastModesWithMediaSources #
Total comments: 29
Patch Set 3 : Address Mark's comments, add unit test for CMWMS #
Total comments: 22
Patch Set 4 : Address Mark's comments #
Total comments: 52
Patch Set 5 : Address Mark and Derek's comments #
Total comments: 3
Patch Set 6 : rebase #Patch Set 7 : move ctor for CastModesWithMediaSources #
Total comments: 4
Patch Set 8 : use base::MakeUnique, name changes #Patch Set 9 : Modify MediaRouterDialogControllerAndroid, format #Messages
Total messages: 71 (35 generated)
Description was changed from ========== . BUG= ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed... to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI BUG=627655 ==========
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed... to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed... to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI BUG=627655 ==========
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed... to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI BUG=627655 ==========
takumif@chromium.org changed reviewers: + mfoltz@chromium.org
Hey Mark, could you take a look? Thanks!
This is a really tricky change with lots of refactoring, thanks for taking it on. I think this follows the pattern of the existing code pretty closely. It seems like there are multiple data structures maintained in query_result_manager that contain different slices of the (cast mode, source, sink) tuples. Ideally I would like to see fewer data structures and simpler resulting code. Keep in mind that the number of items in each of these lists is going to be very small and not updated frequently so performance should not be an overriding concern. What would QueryResultManager look like with the following three structures? A) A map of (source, MediaSinksObserver) to keep track of the per-source sink observers B) A vector of CastMode where CastMode keeps track of the source list for that mode (in priority order) C) A map of (sinkId, MediaSinkWithCastModes) where MSWCM keeps track of the per-sink preferred source for each supported mode Adding sources with a cast mode updates (A) and (B) Getting a sink list updates (C) Let me know what you think, and we'll go from there. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:55: const MediaCastMode cast_mode_; This assumes that a source is associated with one cast mode. This likely remains true for now as the default mode will have presentation URLs and the desktop/tab modes will have capture sources. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:81: void QueryResultManager::StartSinksQuery( This might be better named UpdateSourcesForCastMode? https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:93: RemoveObserversForCastMode(cast_mode); Shouldn't this be computing the difference between the old set of sources and the new ones and removing/creating observers accordingly? https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:101: sinks_observers_[cast_mode].push_back(std::move(observer)); It would seem more logical to have MediaSource::Id as the key for sinks_observers_, then no need to keep a vector in the value. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:123: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Please call DCHECK_CURRENTLY_ON consistently for public methods in this API. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:115: void SetSourcesForCastMode( Doesn't this depend on the sink? Or is this to keep track of which sources we need to register observers for? https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:135: MediaSource GetFirstSourceSupportedBySink( Doesn't this depend on the cast mode? Also, this could be a method on MediaSinkWithCastModes. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:139: bool SinkSupportsSource(MediaSink sink, MediaSource source) const; This could be a method on MediaSinkWithCastModes. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:143: MediaSinkWithCastModes GetMediaSinkWithCastModes(const MediaSink& sink); return const ref https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:155: std::map<MediaSink, std::map<MediaCastMode, These nested data structures are really hard to understand. It seems like this value could be folded into MediaSinkWithCastModes by having MSWCM be an ordered map of CastMode -> MediaSource.
On 2016/08/23 20:46:47, mark a. foltz wrote: > This is a really tricky change with lots of refactoring, thanks for taking it > on. > > I think this follows the pattern of the existing code pretty closely. It seems > like there are multiple data structures maintained in query_result_manager that > contain different slices of the (cast mode, source, sink) tuples. Ideally I > would like to see fewer data structures and simpler resulting code. Keep in > mind that the number of items in each of these lists is going to be very small > and not updated frequently so performance should not be an overriding concern. > > What would QueryResultManager look like with the following three structures? > > A) A map of (source, MediaSinksObserver) to keep track of the per-source sink > observers > B) A vector of CastMode where CastMode keeps track of the source list for that > mode (in priority order) > C) A map of (sinkId, MediaSinkWithCastModes) where MSWCM keeps track of the > per-sink preferred source for each supported mode > > Adding sources with a cast mode updates (A) and (B) > Getting a sink list updates (C) > > Let me know what you think, and we'll go from there. > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.cc:55: const > MediaCastMode cast_mode_; > This assumes that a source is associated with one cast mode. This likely > remains true for now as the default mode will have presentation URLs and the > desktop/tab modes will have capture sources. > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.cc:81: void > QueryResultManager::StartSinksQuery( > This might be better named UpdateSourcesForCastMode? > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.cc:93: > RemoveObserversForCastMode(cast_mode); > Shouldn't this be computing the difference between the old set of sources and > the new ones and removing/creating observers accordingly? > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.cc:101: > sinks_observers_[cast_mode].push_back(std::move(observer)); > It would seem more logical to have MediaSource::Id as the key for > sinks_observers_, then no need to keep a vector in the value. > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.cc:123: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > Please call DCHECK_CURRENTLY_ON consistently for public methods in this API. > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > File chrome/browser/ui/webui/media_router/query_result_manager.h (right): > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.h:115: void > SetSourcesForCastMode( > Doesn't this depend on the sink? Or is this to keep track of which sources we > need to register observers for? > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.h:135: MediaSource > GetFirstSourceSupportedBySink( > Doesn't this depend on the cast mode? > > Also, this could be a method on MediaSinkWithCastModes. > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.h:139: bool > SinkSupportsSource(MediaSink sink, MediaSource source) const; > This could be a method on MediaSinkWithCastModes. > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.h:143: > MediaSinkWithCastModes GetMediaSinkWithCastModes(const MediaSink& sink); > return const ref > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > chrome/browser/ui/webui/media_router/query_result_manager.h:155: > std::map<MediaSink, std::map<MediaCastMode, > These nested data structures are really hard to understand. It seems like this > value could be folded into MediaSinkWithCastModes by having MSWCM be an ordered > map of CastMode -> MediaSource. Regarding structure (C) that you proposed, if the preferred source for a sink becomes no longer available, there would be no way to determine if there are other sources for the same cast mode that the sink supports. Wouldn't that be a problem? Do we expect e.g. a cast mode to have sources x, y, and z, then switch to having y, z, and w?
https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:93: RemoveObserversForCastMode(cast_mode); On 2016/08/23 20:46:46, mark a. foltz wrote: > Shouldn't this be computing the difference between the old set of sources and > the new ones and removing/creating observers accordingly? That would be more complex, but would make sense if we want to reduce the number of queries we make to the MRMojoImpl. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:101: sinks_observers_[cast_mode].push_back(std::move(observer)); On 2016/08/23 20:46:46, mark a. foltz wrote: > It would seem more logical to have MediaSource::Id as the key for > sinks_observers_, then no need to keep a vector in the value. Keeping them in a vector made it simpler to get rid of all the observers for a cast mode in RemoveObserversForCastMode(), but if we'll be removing observers source-by-source, that'd make more sense. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:115: void SetSourcesForCastMode( On 2016/08/23 20:46:47, mark a. foltz wrote: > Doesn't this depend on the sink? Or is this to keep track of which sources we > need to register observers for? It's the latter. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.h:155: std::map<MediaSink, std::map<MediaCastMode, On 2016/08/23 20:46:47, mark a. foltz wrote: > These nested data structures are really hard to understand. It seems like this > value could be folded into MediaSinkWithCastModes by having MSWCM be an ordered > map of CastMode -> MediaSource. This could be a map from sink to unordered set of sources. I believe we'll still need per-sink set(s) of supported sources, as I mentioned in my reply.
Hey Mark, when you have time could you take a look at my comments above and the question regarding structure (C) that you proposed? Thanks, Takumi On Tue, Aug 23, 2016 at 3:13 PM, <takumif@chromium.org> wrote: > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > browser/ui/webui/media_router/query_result_manager.cc > File chrome/browser/ui/webui/media_router/query_result_manager.cc > (right): > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > browser/ui/webui/media_router/query_result_manager.cc#newcode93 > chrome/browser/ui/webui/media_router/query_result_manager.cc:93: > RemoveObserversForCastMode(cast_mode); > On 2016/08/23 20:46:46, mark a. foltz wrote: > > Shouldn't this be computing the difference between the old set of > sources and > > the new ones and removing/creating observers accordingly? > > That would be more complex, but would make sense if we want to reduce > the number of queries we make to the MRMojoImpl. > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > browser/ui/webui/media_router/query_result_manager.cc#newcode101 > chrome/browser/ui/webui/media_router/query_result_manager.cc:101: > sinks_observers_[cast_mode].push_back(std::move(observer)); > On 2016/08/23 20:46:46, mark a. foltz wrote: > > It would seem more logical to have MediaSource::Id as the key for > > sinks_observers_, then no need to keep a vector in the value. > > Keeping them in a vector made it simpler to get rid of all the observers > for a cast mode in RemoveObserversForCastMode(), but if we'll be > removing observers source-by-source, that'd make more sense. > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > browser/ui/webui/media_router/query_result_manager.h > File chrome/browser/ui/webui/media_router/query_result_manager.h > (right): > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > browser/ui/webui/media_router/query_result_manager.h#newcode115 > chrome/browser/ui/webui/media_router/query_result_manager.h:115: void > SetSourcesForCastMode( > On 2016/08/23 20:46:47, mark a. foltz wrote: > > Doesn't this depend on the sink? Or is this to keep track of which > sources we > > need to register observers for? > > It's the latter. > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > browser/ui/webui/media_router/query_result_manager.h#newcode155 > chrome/browser/ui/webui/media_router/query_result_manager.h:155: > std::map<MediaSink, std::map<MediaCastMode, > On 2016/08/23 20:46:47, mark a. foltz wrote: > > These nested data structures are really hard to understand. It seems > like this > > value could be folded into MediaSinkWithCastModes by having MSWCM be > an ordered > > map of CastMode -> MediaSource. > > This could be a map from sink to unordered set of sources. I believe > we'll still need per-sink set(s) of supported sources, as I mentioned in > my reply. > > https://codereview.chromium.org/2264153002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/24 at 21:56:57, chromium-reviews wrote: > Hey Mark, when you have time could you take a look at my comments above and > the question regarding structure (C) that you proposed? > > Thanks, > Takumi > > On Tue, Aug 23, 2016 at 3:13 PM, <takumif@chromium.org> wrote: > > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > > browser/ui/webui/media_router/query_result_manager.cc > > File chrome/browser/ui/webui/media_router/query_result_manager.cc > > (right): > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > > browser/ui/webui/media_router/query_result_manager.cc#newcode93 > > chrome/browser/ui/webui/media_router/query_result_manager.cc:93: > > RemoveObserversForCastMode(cast_mode); > > On 2016/08/23 20:46:46, mark a. foltz wrote: > > > Shouldn't this be computing the difference between the old set of > > sources and > > > the new ones and removing/creating observers accordingly? > > > > That would be more complex, but would make sense if we want to reduce > > the number of queries we make to the MRMojoImpl. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > > browser/ui/webui/media_router/query_result_manager.cc#newcode101 > > chrome/browser/ui/webui/media_router/query_result_manager.cc:101: > > sinks_observers_[cast_mode].push_back(std::move(observer)); > > On 2016/08/23 20:46:46, mark a. foltz wrote: > > > It would seem more logical to have MediaSource::Id as the key for > > > sinks_observers_, then no need to keep a vector in the value. > > > > Keeping them in a vector made it simpler to get rid of all the observers > > for a cast mode in RemoveObserversForCastMode(), but if we'll be > > removing observers source-by-source, that'd make more sense. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > > browser/ui/webui/media_router/query_result_manager.h > > File chrome/browser/ui/webui/media_router/query_result_manager.h > > (right): > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > > browser/ui/webui/media_router/query_result_manager.h#newcode115 > > chrome/browser/ui/webui/media_router/query_result_manager.h:115: void > > SetSourcesForCastMode( > > On 2016/08/23 20:46:47, mark a. foltz wrote: > > > Doesn't this depend on the sink? Or is this to keep track of which > > sources we > > > need to register observers for? > > > > It's the latter. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/ > > browser/ui/webui/media_router/query_result_manager.h#newcode155 > > chrome/browser/ui/webui/media_router/query_result_manager.h:155: > > std::map<MediaSink, std::map<MediaCastMode, > > On 2016/08/23 20:46:47, mark a. foltz wrote: > > > These nested data structures are really hard to understand. It seems > > like this > > > value could be folded into MediaSinkWithCastModes by having MSWCM be > > an ordered > > > map of CastMode -> MediaSource. > > > > This could be a map from sink to unordered set of sources. I believe > > we'll still need per-sink set(s) of supported sources, as I mentioned in > > my reply. > > > > https://codereview.chromium.org/2264153002/ > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > I'll try to get to this today.
On 2016/08/23 at 22:12:56, takumif wrote: > On 2016/08/23 20:46:47, mark a. foltz wrote: > > This is a really tricky change with lots of refactoring, thanks for taking it > > on. > > > > I think this follows the pattern of the existing code pretty closely. It seems > > like there are multiple data structures maintained in query_result_manager that > > contain different slices of the (cast mode, source, sink) tuples. Ideally I > > would like to see fewer data structures and simpler resulting code. Keep in > > mind that the number of items in each of these lists is going to be very small > > and not updated frequently so performance should not be an overriding concern. > > > > What would QueryResultManager look like with the following three structures? > > > > A) A map of (source, MediaSinksObserver) to keep track of the per-source sink > > observers > > B) A vector of CastMode where CastMode keeps track of the source list for that > > mode (in priority order) > > C) A map of (sinkId, MediaSinkWithCastModes) where MSWCM keeps track of the > > per-sink preferred source for each supported mode > > > > Adding sources with a cast mode updates (A) and (B) > > Getting a sink list updates (C) > > > > Let me know what you think, and we'll go from there. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.cc:55: const > > MediaCastMode cast_mode_; > > This assumes that a source is associated with one cast mode. This likely > > remains true for now as the default mode will have presentation URLs and the > > desktop/tab modes will have capture sources. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.cc:81: void > > QueryResultManager::StartSinksQuery( > > This might be better named UpdateSourcesForCastMode? > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.cc:93: > > RemoveObserversForCastMode(cast_mode); > > Shouldn't this be computing the difference between the old set of sources and > > the new ones and removing/creating observers accordingly? > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.cc:101: > > sinks_observers_[cast_mode].push_back(std::move(observer)); > > It would seem more logical to have MediaSource::Id as the key for > > sinks_observers_, then no need to keep a vector in the value. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.cc:123: > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > Please call DCHECK_CURRENTLY_ON consistently for public methods in this API. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > File chrome/browser/ui/webui/media_router/query_result_manager.h (right): > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.h:115: void > > SetSourcesForCastMode( > > Doesn't this depend on the sink? Or is this to keep track of which sources we > > need to register observers for? > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.h:135: MediaSource > > GetFirstSourceSupportedBySink( > > Doesn't this depend on the cast mode? > > > > Also, this could be a method on MediaSinkWithCastModes. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.h:139: bool > > SinkSupportsSource(MediaSink sink, MediaSource source) const; > > This could be a method on MediaSinkWithCastModes. > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.h:143: > > MediaSinkWithCastModes GetMediaSinkWithCastModes(const MediaSink& sink); > > return const ref > > > > https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... > > chrome/browser/ui/webui/media_router/query_result_manager.h:155: > > std::map<MediaSink, std::map<MediaCastMode, > > These nested data structures are really hard to understand. It seems like this > > value could be folded into MediaSinkWithCastModes by having MSWCM be an ordered > > map of CastMode -> MediaSource. > > Regarding structure (C) that you proposed, if the preferred source for a sink > becomes no longer available, there would be no way to determine if there are > other sources for the same cast mode that the sink supports. Wouldn't that be a > problem? Do we expect e.g. a cast mode to have sources x, y, and z, then switch > to having y, z, and w? I'm not sure of the exact scenario you describe. However it is possible for a page to set presentation URLS [a, b, c] then change to [b, c, d], which would trigger a new set of sinks observers for the default cast mode. As you say when source a gets removed, you'll have to eliminate it from any MSWCM entries until the sinks observers come back with new sink lists to repopulate MSWCM.
On 2016/08/26 05:03:19, mark a. foltz wrote: > I'm not sure of the exact scenario you describe. However it is possible for a > page to set presentation URLS [a, b, c] then change to [b, c, d], which would > trigger a new set of sinks observers for the default cast mode. As you say when > source a gets removed, you'll have to eliminate it from any MSWCM entries until > the sinks observers come back with new sink lists to repopulate MSWCM. Let me try to explain my thinking better. When the URLs change from [a, b, c] to [b, c, d], you can keep the original observers for b and c, as well as their compatibility information with sinks (assuming that doesn't change. Does it?). That way you can reduce the number of queries made to the media router, because you're not querying for info on b and c again. However, in order to store this information we need per-sink sets of supported sources. Is it more important to reduce the number of queries made to the media router, or keep things simple? One way to store the information mentioned above would be to make MSWCM contain a set of media sources as its third property. This would get a bit complex when we're updating the list of sources for a cast mode, and trying to determine whether a sink still supports the cast mode, as the sources supported by the sink would be mixed in a set containing sources from other cast modes. Another way would be to split up that set of sources by cast mode. MSWCM would contain a sink and a map from cast modes to sets of sources. This would be easier to update and to determine whether a sink supports a cast mode, so I would prefer to do this. What do you think?
On 2016/08/26 at 18:06:08, takumif wrote: > On 2016/08/26 05:03:19, mark a. foltz wrote: > > I'm not sure of the exact scenario you describe. However it is possible for a > > page to set presentation URLS [a, b, c] then change to [b, c, d], which would > > trigger a new set of sinks observers for the default cast mode. As you say when > > source a gets removed, you'll have to eliminate it from any MSWCM entries until > > the sinks observers come back with new sink lists to repopulate MSWCM. > > Let me try to explain my thinking better. > > When the URLs change from [a, b, c] to [b, c, d], you can keep the original observers for b and c, as well as their compatibility information with sinks (assuming that doesn't change. Does it?). That way you can reduce the number of queries made to the media router, because you're not querying for info on b and c again. However, in order to store this information we need per-sink sets of supported sources. Is it more important to reduce the number of queries made to the media router, or keep things simple? If there's an opportunity to reduce the number of queries that is better. Adding or removing a sinks observer may wake up the component event page and we want to minimize the number of times we want to do that. > > One way to store the information mentioned above would be to make MSWCM contain a set of media sources as its third property. This would get a bit complex when we're updating the list of sources for a cast mode, and trying to determine whether a sink still supports the cast mode, as the sources supported by the sink would be mixed in a set containing sources from other cast modes. > > Another way would be to split up that set of sources by cast mode. MSWCM would contain a sink and a map from cast modes to sets of sources. This would be easier to update and to determine whether a sink supports a cast mode, so I would prefer to do this. > > What do you think? It's hard to assess without actually coding it up. I think drafting a patch along one of the two proposals above and seeing how it looks would be instructive. The latter sounds like a good start, even if we do need to keep multiple sources per sink in MSWCM. Stepping back, my overall preference that motivated the original feedback is to avoid having data structures nested two or three deep. Instead of having a map of sets or a map of vectors, create a struct or object to hold the map value and then have an API to manipulate the value when needed. This is particularly the case when there are multiple data structures that need to be in sync. For example, in PresentationServiceDelegateImpl [1], we have a top level object that keeps track of the overall state of the PresentationAPI for a WebContents (tab), then child objects for a set of frames (PresentationFrameManager) and individual frames (PresentationFrame). This could have been done in a single object with nested maps but then you end up writing iterators or lookups two or three levels deep and it gets more difficult to understand where you are and what you are doing (IMO). [1] https://cs-staging.chromium.org/chromium/src/chrome/browser/media/router/pres... This patch may fit this pattern by moving some of the per-sink bookkeeping into MSWCM.
Sorry, forgot to publish a draft. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:101: sinks_observers_[cast_mode].push_back(std::move(observer)); On 2016/08/23 at 22:13:09, takumif wrote: > On 2016/08/23 20:46:46, mark a. foltz wrote: > > It would seem more logical to have MediaSource::Id as the key for > > sinks_observers_, then no need to keep a vector in the value. > > Keeping them in a vector made it simpler to get rid of all the observers for a cast mode in RemoveObserversForCastMode(), but if we'll be removing observers source-by-source, that'd make more sense. If that's the common case (adding/removing by cast mode) then this data structure seems ok.
I decided to create a new class, CastModesWithMediaSources, rather than making MediaSinkWithCastModes contain sources as well. Adding sources to MSWCM was making it too complex, and MRUI and MRWebUIMessageHandler don't need MSWCM to contain information about the sources. I also made the registration of MediaSinksObserver per-source rather than per-cast mode to reduce the number of queries to the MR. CMWMS should make QueryResultManager easier to read. If CMWMS looks good, I'll be writing unit tests for it. https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:81: void QueryResultManager::StartSinksQuery( On 2016/08/23 20:46:46, mark a. foltz wrote: > This might be better named UpdateSourcesForCastMode? Isn't it nicer to have StartSinksQuery() and StopSinksQuery() have matching names? https://codereview.chromium.org/2264153002/diff/1/chrome/browser/ui/webui/med... chrome/browser/ui/webui/media_router/query_result_manager.cc:123: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2016/08/23 20:46:46, mark a. foltz wrote: > Please call DCHECK_CURRENTLY_ON consistently for public methods in this API. Done.
Thanks, overall I feel this is easier to follow, and most of my comments are minor suggestions on coding style. Thanks also for adding a unit test for sink priority order in query_result_manager. I'll want to do a final pass to make sure I didn't miss anything. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:32: bool operator==(const MediaSource& other) const; Why do we need both == and Equals? https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_request.h (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_request.h:22: // TODO(crbug.com/627655): Handle multiple URLs. Would it be possible to change the ctor to take const std::vector<std::string>& presentation_urls? There should be only a couple of invocations of this. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:10: CastModesWithMediaSources::CastModesWithMediaSources( Is the copy ctor necessary? https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:28: if (cast_modes_.find(cast_mode) == cast_modes_.end()) Assign cast_modes_.find(cast_mode) to a local variable to avoid repeated lookups. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:34: void CastModesWithMediaSources::RemoveCastMode(MediaCastMode cast_mode) { It seems like this should only be done if the set of sources is also empty. Does this need to be part of the API? https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:16: // Finish class level comment https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:31: bool IsEmpty() const; Does this mean the cast mode set is empty? Please document. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:324: // TODO(crbug.com/627655): Use multiple URLs. Sigh, this is not straightforward. The MediaRoutesObserver needs to be redesigned to remove the need to take a MediaSource and we need to find another way to pass the "joinable route ids". https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:117: if (sink_pair.first.id() == sink_id) { You could create a MediaSink(sink_id) and lookup with that. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:128: return cast_mode_sources_.find(cast_mode) == cast_mode_sources_.end() ? base::ContainsKey here and elsewhere to test map membership https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:134: cast_mode_sources_[cast_mode] = sources; Nit: - What does it mean if |sources| is empty? Would slightly prefer that the entry be removed. - Is |sources| expected to be unique - what if there are duplicates? https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:139: const std::vector<MediaSource> old_sources = Slight preference for just an early return if there is no entry for cast_mode (versus creating an empty vector and iterating over it). https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:197: if (cast_mode_sources_.find(cast_mode) == cast_mode_sources_.end()) Store the result from find() and use it below https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:198: return MediaSource(); What does it mean to return an empty source here? An empty source is not really defined. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:204: return MediaSource(); Ditto
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Hi Mark, thanks for the comments. I've made changes accordingly. Please take a look, thanks! https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:32: bool operator==(const MediaSource& other) const; On 2016/08/31 05:18:54, mark a. foltz wrote: > Why do we need both == and Equals? Got rid of Equals(). https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_request.h (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_request.h:22: // TODO(crbug.com/627655): Handle multiple URLs. On 2016/08/31 05:18:54, mark a. foltz wrote: > Would it be possible to change the ctor to take const std::vector<std::string>& > presentation_urls? There should be only a couple of invocations of this. Done. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:10: CastModesWithMediaSources::CastModesWithMediaSources( On 2016/08/31 05:18:54, mark a. foltz wrote: > Is the copy ctor necessary? We need it for using the [] operator on |all_sinks_| in QueryResultManager, in which CMWMS is used as the values in the map. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:28: if (cast_modes_.find(cast_mode) == cast_modes_.end()) On 2016/08/31 05:18:54, mark a. foltz wrote: > Assign cast_modes_.find(cast_mode) to a local variable to avoid repeated > lookups. Done. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:34: void CastModesWithMediaSources::RemoveCastMode(MediaCastMode cast_mode) { On 2016/08/31 05:18:54, mark a. foltz wrote: > It seems like this should only be done if the set of sources is also empty. > Does this need to be part of the API? No, this is no longer necessary. Removing. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:16: // On 2016/08/31 05:18:55, mark a. foltz wrote: > Finish class level comment Oops. Done. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:31: bool IsEmpty() const; On 2016/08/31 05:18:54, mark a. foltz wrote: > Does this mean the cast mode set is empty? Please document. Done. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:324: // TODO(crbug.com/627655): Use multiple URLs. On 2016/08/31 05:18:55, mark a. foltz wrote: > Sigh, this is not straightforward. The MediaRoutesObserver needs to be > redesigned to remove the need to take a MediaSource and we need to find another > way to pass the "joinable route ids". Yeah, I wasn't sure what to do here. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:117: if (sink_pair.first.id() == sink_id) { On 2016/08/31 05:18:55, mark a. foltz wrote: > You could create a MediaSink(sink_id) and lookup with that. Creating a dummy sink with fake description and icon for lookup relies on the fact that sink comparison only looks at the sink ID, and feels a bit hacky to me. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:128: return cast_mode_sources_.find(cast_mode) == cast_mode_sources_.end() ? On 2016/08/31 05:18:55, mark a. foltz wrote: > base::ContainsKey here and elsewhere to test map membership Done. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:134: cast_mode_sources_[cast_mode] = sources; On 2016/08/31 05:18:55, mark a. foltz wrote: > Nit: > - What does it mean if |sources| is empty? Would slightly prefer that the entry > be removed. > - Is |sources| expected to be unique - what if there are duplicates? Removed. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:139: const std::vector<MediaSource> old_sources = On 2016/08/31 05:18:55, mark a. foltz wrote: > Slight preference for just an early return if there is no entry for cast_mode > (versus creating an empty vector and iterating over it). Done. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:197: if (cast_mode_sources_.find(cast_mode) == cast_mode_sources_.end()) On 2016/08/31 05:18:55, mark a. foltz wrote: > Store the result from find() and use it below I used base::ContainsKey here instead. https://codereview.chromium.org/2264153002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:198: return MediaSource(); On 2016/08/31 05:18:55, mark a. foltz wrote: > What does it mean to return an empty source here? An empty source is not really > defined. I got rid of empty sources (and the default ctor) altogether, since in most cases we can now represent the lack of sources with an empty vector.
Overall design looks good. Mostly minor comments and naming suggestions https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/create_presentation_connection_request.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/create_presentation_connection_request.cc:7: #include <memory> This should go in the .h https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_media_sinks_observer.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_media_sinks_observer.cc:7: #include <vector> Put in .h https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_request.h (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_request.h:9: #include <vector> Ditto https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:31: const auto& sources_for_cast_mode = it->second; base::ContainsKey? https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:25: void AddSource(const MediaSource& source, MediaCastMode cast_mode); Nit: the mapping is from cast mode to source(s). I would reverse the order of the arguments here and below. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:37: // Returns true if there are no cast modes contained. Not sure this adds much versus GetCastModes().empty(). https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:41: std::map<MediaCastMode, std::unordered_set<MediaSource, MediaSource::Hash>> A std::multimap may be simpler but would not inherently enforce uniqueness of MediaSource per MediaCastMode, which seems like a good property. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:195: return &source; Do you really want to return a pointer here? What if a later update removes this source? You may want to return a std::unique_ptr that can be filled with a copy. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.h:115: // longer exist, and creates observers and starts queries for new sources. The comments seem out of sync with the API. Maybe the API should look like this? UpdateObserversForCastMode(cast_mode, sources) Calls: RemoveObserversForCastMode(cast_mode, sources) AddObserversForCastMode(cast_mode, sources) https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.h:127: const std::vector<MediaSink>& result_sinks); Or |current_sinks|? https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.h:148: std::map<MediaCastMode, std::vector<MediaSource>> cast_mode_sources_; std::multimap?
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Please take a look, thanks! https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/create_presentation_connection_request.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/create_presentation_connection_request.cc:7: #include <memory> On 2016/09/02 23:00:37, mark a. foltz wrote: > This should go in the .h Done. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_media_sinks_observer.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_media_sinks_observer.cc:7: #include <vector> On 2016/09/02 23:00:37, mark a. foltz wrote: > Put in .h Done. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_request.h (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_request.h:9: #include <vector> On 2016/09/02 23:00:37, mark a. foltz wrote: > Ditto This one already is :P https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:31: const auto& sources_for_cast_mode = it->second; On 2016/09/02 23:00:37, mark a. foltz wrote: > base::ContainsKey? Done. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:25: void AddSource(const MediaSource& source, MediaCastMode cast_mode); On 2016/09/02 23:00:37, mark a. foltz wrote: > Nit: the mapping is from cast mode to source(s). I would reverse the order of > the arguments here and below. Done. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:37: // Returns true if there are no cast modes contained. On 2016/09/02 23:00:37, mark a. foltz wrote: > Not sure this adds much versus GetCastModes().empty(). It's more obvious than GetCastModes().empty(), and we'll only have to fix it here if we ever decide to stop using CastModeSet::empty(). https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:41: std::map<MediaCastMode, std::unordered_set<MediaSource, MediaSource::Hash>> On 2016/09/02 23:00:37, mark a. foltz wrote: > A std::multimap may be simpler but would not inherently enforce uniqueness of > MediaSource per MediaCastMode, which seems like a good property. Yeah, it's nice not having to check for duplicates when adding. And removing a source would be a bit more complicated. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.cc:195: return &source; On 2016/09/02 23:00:37, mark a. foltz wrote: > Do you really want to return a pointer here? What if a later update removes > this source? You may want to return a std::unique_ptr that can be filled with a > copy. Changed to return a unique_ptr. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.h:115: // longer exist, and creates observers and starts queries for new sources. On 2016/09/02 23:00:37, mark a. foltz wrote: > The comments seem out of sync with the API. > > Maybe the API should look like this? > > UpdateObserversForCastMode(cast_mode, sources) > > Calls: > > RemoveObserversForCastMode(cast_mode, sources) > AddObserversForCastMode(cast_mode, sources) I'm not sure if UpdateObserversForCastMode() is necessary, as StartSinksQuery() is the only method that wants to add observers. Also, when we remove observers we also want to remove the sources they're associated with, so I want to keep the current naming for that. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.h:127: const std::vector<MediaSink>& result_sinks); On 2016/09/02 23:00:37, mark a. foltz wrote: > Or |current_sinks|? I think it's more like |new_sinks|. https://codereview.chromium.org/2264153002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/query_result_manager.h:148: std::map<MediaCastMode, std::vector<MediaSource>> cast_mode_sources_; On 2016/09/02 23:00:37, mark a. foltz wrote: > std::multimap? I think it makes sense to keep this as-is, since |this| obtains the sources for a cast mode as a vector.
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 ==========
Pretty close. Most of my comments are nit-picky. One substantial comment on query_result_manager. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_request.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_request.cc:34: for (const std::string& presentation_url : presentation_urls_) nit: const auto& https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:22: cast_modes_[cast_mode].erase(source); I would use .find() here so you don't have to allocate and immediately erase an empty entry when there's no |cast_mode| in the map. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:29: return base::ContainsKey(cast_modes_, cast_mode) Using .find() would avoid a double lookup in cast_modes_. e.g. something like: const auto cast_modes_it = cast_modes_.find(cast_mode); return cast_modes_it != cast_modes_.end() ? ContainsKey(cast_modes_it->second, source) : false; https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:24: // Add a source for the cast mode. Super nit: Method comments should begin "Adds", "Removes", etc. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:42: cast_modes_; Indentation https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:100: for (auto* observer : media_sinks_observers_) { nit: Usually Chrome doesn't use {} for two-line for statements; but be consistent with the surrounding code. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:109: modes.insert(observer_pair.first); Blank line after for statement https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:117: if (sink_pair.first.id() == sink_id) { std::find_if ? https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:128: return base::ContainsKey(cast_mode_sources_, cast_mode) ? cast_mode_sources_.find() to avoid double lookup https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:139: sinks_observers_.erase(source); This assumes that two different modes can't add the same source. That should be documented (and enforced). https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:85: // queries for, and stops queries for sources that no longer exist. Maybe: stops queries for sources no longer associated with any cast mode. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:88: void StartSinksQuery(MediaCastMode cast_mode, IIUC, the set of sources and set of cast modes are now distinct, so this might not start a sinks query if the sources were already associated with a different cast mode. It might be slightly better to rename this API SetSourcesForCastMode()/RemoveSourcesForCastMode(), but I don't feel strongly. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:117: // |cast_mode| used to support, but isn't in |new_sources|. What if the sources were registered for a different cast mode? https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:118: void RemoveOldSourcesForCastMode( RemoveObserversForCastMode sounds better to me, but I don't feel strongly. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:124: const std::vector<MediaSource>& sources, |new_sources| ? https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:130: const MediaSource source, const MediaSource& https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:155: MediaSource defaultSource1 = MediaSourceForPresentationUrl("http://barUrl"); Nit: please use obviously valid URLs as test data, as this will mean less test cleanup when we switch to GURL/KURL. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:266: TEST_F(QueryResultManagerTest, MultipleUrls) { Can you update this test case to include at least one non-URL source, e.g. MediaSourceForTab(1). https://codereview.chromium.org/2264153002/diff/140001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:1566: 'browser/ui/webui/media_router/cast_modes_with_media_sources.cc', I think chrome/ .gypi have been deleted.
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
Drive-by comments. This design is a little different from what I had in mind. In particular I was thinking the UI will displaying each URL of the PresentationRequest in the cast mode picker, and the user will be able to select a specific URL to cast to. To support this however will require a more complicated design, so I can understand if we are not going this route (e.g., the added options do not add much benefit / may confuse the user). So just want to confirm that we are going with the current design (user selects cast mode, browser code maps it to the most preferred source) https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:21: CastModesWithMediaSources(const CastModesWithMediaSources& other); This object looks a bit expensive to copy. Is copy ctor really needed, or can we disallow copy assignment/construction instead? https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:192: const CastModesWithMediaSources& sources_for_sink = all_sinks_.at(sink); I don't think we should be using the at() methods on associative containers since they throw exceptions if the value is not found, and exceptions are banned in Chromium. In this case in particular we are also doing a double lookup in |all_sinks_|. Also, it doesn't seem necessary to break this into a separate function. I think we should just inline this into GetSourceForCastModeAndSink() and see if we can get rid of the double lookup. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:148: std::map<MediaSource, std::unique_ptr<MediaSinksObserver>, Is there any reason we need to maintain order for these sources/observers? It looks like we added the Compare struct specifically for this purpose but I am not sure if we actually need it. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:155: std::map<MediaSink, CastModesWithMediaSources, MediaSink::Compare> all_sinks_; Ditto on ordering/Compare struct.
On 2016/09/12 at 19:26:29, imcheng wrote: > Drive-by comments. This design is a little different from what I had in mind. In particular I was thinking the UI will displaying each URL of the PresentationRequest in the cast mode picker, and the user will be able to select a specific URL to cast to. To support this however will require a more complicated design, so I can understand if we are not going this route (e.g., the added options do not add much benefit / may confuse the user). So just want to confirm that we are going with the current design (user selects cast mode, browser code maps it to the most preferred source) Yeah, sorry for any confusion. The idea in the spec is that the user agent will choose the first URL in the list that is compatible with the chosen presentation display. So that the user is not required to choose the URL to present, only the display. We could add this feature if there is demand (after all, the site should expect presentation of any URL that is provided).
On 2016/09/12 at 20:54:39, mark a. foltz wrote: > On 2016/09/12 at 19:26:29, imcheng wrote: > > Drive-by comments. This design is a little different from what I had in mind. In particular I was thinking the UI will displaying each URL of the PresentationRequest in the cast mode picker, and the user will be able to select a specific URL to cast to. To support this however will require a more complicated design, so I can understand if we are not going this route (e.g., the added options do not add much benefit / may confuse the user). So just want to confirm that we are going with the current design (user selects cast mode, browser code maps it to the most preferred source) > > Yeah, sorry for any confusion. The idea in the spec is that the user agent will choose the first URL in the list that is compatible with the chosen presentation display. So that the user is not required to choose the URL to present, only the display. We could add this feature if there is demand (after all, the site should expect presentation of any URL that is provided). Separately we should show some details in the UX about the URL being presented: this is for security, so that the user knows what site is being shown on the display.
Patchset #5 (id:160001) has been deleted
Thanks for the comments. Please take a look. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_request.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_request.cc:34: for (const std::string& presentation_url : presentation_urls_) On 2016/09/09 22:22:26, mark a. foltz wrote: > nit: const auto& Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:22: cast_modes_[cast_mode].erase(source); On 2016/09/09 22:22:26, mark a. foltz wrote: > I would use .find() here so you don't have to allocate and immediately erase > an empty entry when there's no |cast_mode| in the map. Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:29: return base::ContainsKey(cast_modes_, cast_mode) On 2016/09/09 22:22:26, mark a. foltz wrote: > Using .find() would avoid a double lookup in cast_modes_. > > e.g. something like: > > const auto cast_modes_it = cast_modes_.find(cast_mode); > return cast_modes_it != cast_modes_.end() ? ContainsKey(cast_modes_it->second, > source) : false; Given the low number of cast modes, I think I prefer using ContainsKey twice for readability. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:21: CastModesWithMediaSources(const CastModesWithMediaSources& other); On 2016/09/12 19:26:28, imcheng wrote: > This object looks a bit expensive to copy. Is copy ctor really needed, or can we > disallow copy assignment/construction instead? It's needed for implicitly instantiating with the [] operator for unordered_map in QueryResultManager. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:24: // Add a source for the cast mode. On 2016/09/09 22:22:26, mark a. foltz wrote: > Super nit: Method comments should begin "Adds", "Removes", etc. Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:42: cast_modes_; On 2016/09/09 22:22:26, mark a. foltz wrote: > Indentation Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:100: for (auto* observer : media_sinks_observers_) { On 2016/09/09 22:22:26, mark a. foltz wrote: > nit: Usually Chrome doesn't use {} for two-line for statements; but be > consistent with the surrounding code. Got it, removed the braces. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:109: modes.insert(observer_pair.first); On 2016/09/09 22:22:27, mark a. foltz wrote: > Blank line after for statement Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:117: if (sink_pair.first.id() == sink_id) { On 2016/09/09 22:22:27, mark a. foltz wrote: > std::find_if ? find_if would look messier, since we can't use auto for the parameter for the lambda function passed into it. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:128: return base::ContainsKey(cast_mode_sources_, cast_mode) ? On 2016/09/09 22:22:27, mark a. foltz wrote: > cast_mode_sources_.find() to avoid double lookup Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:139: sinks_observers_.erase(source); On 2016/09/09 22:22:27, mark a. foltz wrote: > This assumes that two different modes can't add the same source. That should be > documented (and enforced). Added a method AreSourcesValidForCastMode() for checking that a source doesn't get associated with two cast modes. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:192: const CastModesWithMediaSources& sources_for_sink = all_sinks_.at(sink); On 2016/09/12 19:26:28, imcheng wrote: > I don't think we should be using the at() methods on associative containers > since they throw exceptions if the value is not found, and exceptions are banned > in Chromium. > > In this case in particular we are also doing a double lookup in |all_sinks_|. > Also, it doesn't seem necessary to break this into a separate function. I think > we should just inline this into GetSourceForCastModeAndSink() and see if we can > get rid of the double lookup. I'd like to keep this a separate method to avoid having too many nested for/ifs. I can avoid the double lookup by passing in CastModesWithMediaSources instead of MediaSink to this method. Got rid of at() to avoid double lookup, but at() should be safe as long as we check for membership beforehand. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:85: // queries for, and stops queries for sources that no longer exist. On 2016/09/09 22:22:27, mark a. foltz wrote: > Maybe: > > stops queries for sources no longer associated with any cast mode. Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:88: void StartSinksQuery(MediaCastMode cast_mode, On 2016/09/09 22:22:27, mark a. foltz wrote: > IIUC, the set of sources and set of cast modes are now distinct, so this might > not start a sinks query if the sources were already associated with a different > cast mode. > > It might be slightly better to rename this API > SetSourcesForCastMode()/RemoveSourcesForCastMode(), but I don't feel strongly. Added a method AreSourcesValidForCastMode() for checking that a source doesn't get associated with two cast modes. Renamed the two methods. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:117: // |cast_mode| used to support, but isn't in |new_sources|. On 2016/09/09 22:22:27, mark a. foltz wrote: > What if the sources were registered for a different cast mode? Added a method AreSourcesValidForCastMode() for checking that a source doesn't get associated with two cast modes. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:118: void RemoveOldSourcesForCastMode( On 2016/09/09 22:22:27, mark a. foltz wrote: > RemoveObserversForCastMode sounds better to me, but I don't feel strongly. Not only do we remove observers, but also disassociate old sources from sinks in this method. Reflected that in the comment. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:124: const std::vector<MediaSource>& sources, On 2016/09/09 22:22:27, mark a. foltz wrote: > |new_sources| ? Not all sources are necessarily new. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:130: const MediaSource source, On 2016/09/09 22:22:27, mark a. foltz wrote: > const MediaSource& Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:148: std::map<MediaSource, std::unique_ptr<MediaSinksObserver>, On 2016/09/12 19:26:29, imcheng wrote: > Is there any reason we need to maintain order for these sources/observers? It > looks like we added the Compare struct specifically for this purpose but I am > not sure if we actually need it. Replaced with unordered_map. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:155: std::map<MediaSink, CastModesWithMediaSources, MediaSink::Compare> all_sinks_; On 2016/09/12 19:26:29, imcheng wrote: > Ditto on ordering/Compare struct. Using map rather than unordered_map here allows us to compare the parameters to OnResultsUpdated() with a vector in tests. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:155: MediaSource defaultSource1 = MediaSourceForPresentationUrl("http://barUrl"); On 2016/09/09 22:22:27, mark a. foltz wrote: > Nit: please use obviously valid URLs as test data, as this will mean less test > cleanup when we switch to GURL/KURL. Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:266: TEST_F(QueryResultManagerTest, MultipleUrls) { On 2016/09/09 22:22:27, mark a. foltz wrote: > Can you update this test case to include at least one non-URL source, e.g. > MediaSourceForTab(1). Done. https://codereview.chromium.org/2264153002/diff/140001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:1566: 'browser/ui/webui/media_router/cast_modes_with_media_sources.cc', On 2016/09/09 22:22:27, mark a. foltz wrote: > I think chrome/ .gypi have been deleted. Editing BUILD.gn instead.
https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:29: return base::ContainsKey(cast_modes_, cast_mode) On 2016/09/13 at 03:48:21, takumif wrote: > On 2016/09/09 22:22:26, mark a. foltz wrote: > > Using .find() would avoid a double lookup in cast_modes_. > > > > e.g. something like: > > > > const auto cast_modes_it = cast_modes_.find(cast_mode); > > return cast_modes_it != cast_modes_.end() ? ContainsKey(cast_modes_it->second, > > source) : false; > > Given the low number of cast modes, I think I prefer using ContainsKey twice for readability. There are at most 3, IIUC. It would be just as fast if not faster for |cast_modes_| to be a vector, but then you'd need to define a struct to hold the MediaCastMode. Doesn't seem worth adding code and allocating extra objects for a small optimization. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:21: CastModesWithMediaSources(const CastModesWithMediaSources& other); On 2016/09/13 at 03:48:21, takumif wrote: > On 2016/09/12 19:26:28, imcheng wrote: > > This object looks a bit expensive to copy. Is copy ctor really needed, or can we > > disallow copy assignment/construction instead? > > It's needed for implicitly instantiating with the [] operator for unordered_map in QueryResultManager. I usually code to avoid the [] operator since it results in the side effect of instantiation with lookup, and map[foo] = bar() results in both calling the default ctor, the ctor for bar(), and the assignment operator. In this case, I am also not sure it makes sense to copy a CMWMS: there should be only one instance in |cast_mode_sources_| per MediaSource. I recommend: - Disallowing copy and assign. - Changing the unordered_map to store unique_ptr *or* declare a move ctor (slight preference for latter). References: https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types https://google.github.io/styleguide/cppguide.html#Rvalue_references https://codereview.chromium.org/2264153002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:214: const auto& cast_mode_it = cast_mode_sources_.find(cast_mode); Nit: combine this with statement below
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.cc:29: return base::ContainsKey(cast_modes_, cast_mode) On 2016/09/13 17:30:15, mark a. foltz wrote: > On 2016/09/13 at 03:48:21, takumif wrote: > > On 2016/09/09 22:22:26, mark a. foltz wrote: > > > Using .find() would avoid a double lookup in cast_modes_. > > > > > > e.g. something like: > > > > > > const auto cast_modes_it = cast_modes_.find(cast_mode); > > > return cast_modes_it != cast_modes_.end() ? > ContainsKey(cast_modes_it->second, > > > source) : false; > > > > Given the low number of cast modes, I think I prefer using ContainsKey twice > for readability. > > There are at most 3, IIUC. It would be just as fast if not faster for > |cast_modes_| to be a vector, but then you'd need to define a struct to hold the > MediaCastMode. Doesn't seem worth adding code and allocating extra objects for > a small optimization. Acknowledged. https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/cast_modes_with_media_sources.h:21: CastModesWithMediaSources(const CastModesWithMediaSources& other); On 2016/09/13 17:30:15, mark a. foltz wrote: > On 2016/09/13 at 03:48:21, takumif wrote: > > On 2016/09/12 19:26:28, imcheng wrote: > > > This object looks a bit expensive to copy. Is copy ctor really needed, or > can we > > > disallow copy assignment/construction instead? > > > > It's needed for implicitly instantiating with the [] operator for > unordered_map in QueryResultManager. > > I usually code to avoid the [] operator since it results in the side effect of > instantiation with lookup, and map[foo] = bar() results in both calling the > default ctor, the ctor for bar(), and the assignment operator. > > In this case, I am also not sure it makes sense to copy a CMWMS: there should be > only one instance in |cast_mode_sources_| per MediaSource. > > I recommend: > - Disallowing copy and assign. > - Changing the unordered_map to store unique_ptr *or* declare a move ctor > (slight preference for latter). > > References: > https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types > https://google.github.io/styleguide/cppguide.html#Rvalue_references Got it. Declaring a move ctor instead. https://codereview.chromium.org/2264153002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:214: const auto& cast_mode_it = cast_mode_sources_.find(cast_mode); On 2016/09/13 17:30:15, mark a. foltz wrote: > Nit: combine this with statement below |cast_mode_it| is reused in the lambda function below, so it's needed. Or is there a better way to do this?
LGTM % one more suggestion. This looks pretty good, a fresh pair of eyes might find additional simplifications in query_result_manager.cc. https://codereview.chromium.org/2264153002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:214: const auto& cast_mode_it = cast_mode_sources_.find(cast_mode); On 2016/09/13 at 19:20:34, takumif wrote: > On 2016/09/13 17:30:15, mark a. foltz wrote: > > Nit: combine this with statement below > > |cast_mode_it| is reused in the lambda function below, so it's needed. Or is there a better way to do this? I missed that; NM. https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:238: sink_with_cast_modes.cast_modes = all_sinks_[sink].GetCastModes(); You already have a MSWCM from iterating over |all_sinks_| above. Just inline this into NotifyOnResultsUpdated as it doesn't seem to be used elsewhere?
lgtm https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:155: std::map<MediaSink, CastModesWithMediaSources, MediaSink::Compare> all_sinks_; On 2016/09/13 03:48:22, takumif wrote: > On 2016/09/12 19:26:29, imcheng wrote: > > Ditto on ordering/Compare struct. > > Using map rather than unordered_map here allows us to compare the parameters to > OnResultsUpdated() with a vector in tests. It isn't ideal to add the Compare solely because of tests. Since we don't expect QRM to be returning results in a specific order, maybe the test could use the UnorderedElementsAreArray matcher instead? Though it may be a bit tricky to use without MediaSink::operator== (which the style guideline prefers over Equals() method). Maybe we can do that in a separate patch. https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:206: return std::unique_ptr<MediaSource>(new MediaSource(source.id())); nit: return base::MakeUnique<MediaSource>(source); (or source.id())
Patchset #8 (id:250001) has been deleted
Patchset #8 (id:270001) has been deleted
takumif@chromium.org changed reviewers: + msw@chromium.org, wfh@chromium.org
+wfh@, please take a look at media_router_type_converters_unittest.cc (security owner). +msw@, please take a look at c/b/ui/BUILD.gn. Thank you! https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.h (right): https://codereview.chromium.org/2264153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.h:155: std::map<MediaSink, CastModesWithMediaSources, MediaSink::Compare> all_sinks_; On 2016/09/14 01:51:23, imcheng wrote: > On 2016/09/13 03:48:22, takumif wrote: > > On 2016/09/12 19:26:29, imcheng wrote: > > > Ditto on ordering/Compare struct. > > > > Using map rather than unordered_map here allows us to compare the parameters > to > > OnResultsUpdated() with a vector in tests. > > It isn't ideal to add the Compare solely because of tests. Since we don't expect > QRM to be returning results in a specific order, maybe the test could use the > UnorderedElementsAreArray matcher instead? Though it may be a bit tricky to use > without MediaSink::operator== (which the style guideline prefers over Equals() > method). Maybe we can do that in a separate patch. Acknowledged. https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/query_result_manager.cc (right): https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:206: return std::unique_ptr<MediaSource>(new MediaSource(source.id())); On 2016/09/14 01:51:23, imcheng wrote: > nit: return base::MakeUnique<MediaSource>(source); (or source.id()) Done. https://codereview.chromium.org/2264153002/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/query_result_manager.cc:238: sink_with_cast_modes.cast_modes = all_sinks_[sink].GetCastModes(); On 2016/09/13 20:38:18, mark a. foltz wrote: > You already have a MSWCM from iterating over |all_sinks_| above. Just inline > this into NotifyOnResultsUpdated as it doesn't seem to be used elsewhere? Done.
chrome/browser/ui/BUILD.gn lgtm
A friendly ping -- wfh@, please take a look at chrome/browser/media/router/mojo/media_router_type_converters_unittest.cc. Thank you!
The CQ bit was checked by takumif@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #9 (id:310001) has been deleted
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@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...
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::CastModeMediaSinksObserver (should be renamed to something like MediaSourceMediaSinksObserver?) observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::StartSinksQuery() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). TBR=wfh@chromium.org BUG=627655 ==========
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). TBR=wfh@chromium.org BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 ==========
takumif@chromium.org changed reviewers: + tsepez@chromium.org
tsepez/wfh, please take a look at chrome/browser/media/router/mojo/media_router_type_converters_unittest.cc imcheng/mfoltz, please take a look at chrome/browser/media/android/router/media_router_dialog_controller_android.cc Thank you!
chrome/browser/media/android/router/media_router_dialog_controller_android.cc lgtm
chrome/browser/media/router/mojo/media_router_type_converters_unittest.cc lgtm
On 2016/09/27 22:37:59, Will Harris wrote: > chrome/browser/media/router/mojo/media_router_type_converters_unittest.cc lgtm Deferring to WFH.
lgtm Please check and update any unittests for media_route_dialog_controller_android.cc. Will need to sync with avayvod@ on what behavior is feasible in the Android controller. It seems to assume every sink is compatible with every source.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/27 22:52:38, mfoltz_ooo_until_9_27 wrote: > lgtm > > Please check and update any unittests for > media_route_dialog_controller_android.cc. > > Will need to sync with avayvod@ on what behavior is feasible in the Android > controller. It seems to assume every sink is compatible with every source. There are no unittests for media_route_dialog_controller_android.cc :( Manually tested YouTube, Netflix, CastHelloVideo, and tab mirroring.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2264153002/#ps330001 (title: "Modify MediaRouterDialogControllerAndroid, format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 ========== to ========== [Presentation API] Add support for multiple URLs in PresentationRequest on Media Router UI side Relevant CL: https://codereview.chromium.org/2174693004/ This CL: - MediaRouterUI now gets multiple sources from PresentationRequest and passes them onto QueryResultManager - QueryResultManager keeps track of not only which cast modes a sink supports, but also which sources for each cast mode - QueryResultManager::MediaSourceMediaSinksObserver observes changes in sinks compatibility of a source rather than a cast mode - QueryResultManager::SetSourcesForCastMode() takes multiple media sources per cast mode - QueryResultManager::GetSourceForCastModeAndSink() (was GetSourceForCastMode()) requires you to specify the sink, because different sinks may support different sources for the same cast mode - Add/fix unit tests for QueryResultManager and MediaRouterUI - Add a class CastModesWithMediaSources used by QueryResultManager for storing cast modes and sources (and unit tests for it). BUG=627655 Committed: https://crrev.com/4f8671de022b8908686fc5667039c4288a7eeb70 Cr-Commit-Position: refs/heads/master@{#421412} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4f8671de022b8908686fc5667039c4288a7eeb70 Cr-Commit-Position: refs/heads/master@{#421412} |
