Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
Committed: https://crrev.com/21ad18d7d165fe60792c5c5e3c56b0647324c03c
Cr-Commit-Position: refs/heads/master@{#436366}

[takumif, 2016-11-30 02:29:33]

Description was changed from

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

BUG=
==========

to

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
==========

[takumif, 2016-11-30 02:29:44]

Description was changed from

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
==========

to

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
==========

[takumif, 2016-11-30 02:34:40]

takumif@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[takumif, 2016-11-30 02:34:40]

This is an implementation of the dialog initialization bug fix that we discussed
today. If the general direction looks good, I'll add comments and tests.

The CL for the previous fix is here: https://codereview.chromium.org/2518083002/

[imcheng, 2016-11-30 19:54:07]

This looks good and is much simpler than the previous approach.

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:184: virtual std::vector<MediaRoute>
GetCurrentRoutes() const = 0;
1) return const ref?
2) Add comments

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:682:
routes_observer_->OnRoutesUpdated(router_->GetCurrentRoutes(),
This feels a bit strange. You are calling the observer so that it will call back
to MRUI to update |routes_|. It looks like what you can do instead is to call
GetCurrentRoutes() in InitCommon() to populate |routes_|. Then you don't need
this method and |initial_routes_received_|.

[mark a. foltz, 2016-11-30 23:52:10]

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:682:
routes_observer_->OnRoutesUpdated(router_->GetCurrentRoutes(),
Will this be the same routes list as UIMediaRoutesObserver?

[takumif, 2016-12-01 00:51:13]

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:682:
routes_observer_->OnRoutesUpdated(router_->GetCurrentRoutes(),
On 2016/11/30 23:52:10, mark a. foltz wrote:
> Will this be the same routes list as UIMediaRoutesObserver?

UIMediaRoutesObserver::OnRoutesUpdated() filters out non-display routes. We
could avoid passing the routes through UIMediaRoutesObserver by moving the
filtering functionality to MediaRouterUI::OnRoutesUpdated().

[takumif, 2016-12-01 03:26:37]

Please take a look, thanks!

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2540773005/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:184: virtual std::vector<MediaRoute>
GetCurrentRoutes() const = 0;
On 2016/11/30 19:54:06, imcheng wrote:
> 1) return const ref?
> 2) Add comments

Done.

[imcheng, 2016-12-01 20:06:53]

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:279:
OnRoutesUpdated(router_->GetCurrentRoutes(), std::vector<MediaRoute::Id>());
Please add a comment about why we are doing this. e.g., to ensure we have the
most recent list of routes to initialize the WebUI.

Note that there might still be some artifacts, namely the Cast/Join button (if
applicable) and sink name may show up later. This is because those depend on
query results that might arrive after the initial view is established. This is
probably fine, but please manually verify that they do show up eventually.

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:279:
OnRoutesUpdated(router_->GetCurrentRoutes(), std::vector<MediaRoute::Id>());
On second thought you might want to call this after |cast_modes_| is initialized
since there are some computation that uses it in OnRoutesUpdated.

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:333: void
MediaRouterUI::UpdateCurrentCastModes() {
This should probably be renamed UpdateRouteToCastModesMapping or something
similar to avoid confusing it with UpdateCastModes. Same with
|current_cast_modes_|.

[takumif, 2016-12-02 19:11:29]

Patchset #3 (id:40001) has been deleted

[takumif, 2016-12-02 19:23:23]

Thanks for reviewing!

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:279:
OnRoutesUpdated(router_->GetCurrentRoutes(), std::vector<MediaRoute::Id>());
On 2016/12/01 20:06:53, imcheng wrote:
> On second thought you might want to call this after |cast_modes_| is
initialized
> since there are some computation that uses it in OnRoutesUpdated.

Done.

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:279:
OnRoutesUpdated(router_->GetCurrentRoutes(), std::vector<MediaRoute::Id>());
On 2016/12/01 20:06:53, imcheng wrote:
> Please add a comment about why we are doing this. e.g., to ensure we have the
> most recent list of routes to initialize the WebUI.
> 
> Note that there might still be some artifacts, namely the Cast/Join button (if
> applicable) and sink name may show up later. This is because those depend on
> query results that might arrive after the initial view is established. This is
> probably fine, but please manually verify that they do show up eventually.

Done.

https://codereview.chromium.org/2540773005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:333: void
MediaRouterUI::UpdateCurrentCastModes() {
On 2016/12/01 20:06:53, imcheng wrote:
> This should probably be renamed UpdateRouteToCastModesMapping or something
> similar to avoid confusing it with UpdateCastModes. Same with
> |current_cast_modes_|.

Done.

[imcheng, 2016-12-02 19:47:53]

lgtm

[takumif, 2016-12-02 20:54:19]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 20:54:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 21:49:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 21:49:39]

Dry run: This issue passed the CQ dry run.

[mark a. foltz, 2016-12-02 23:29:44]

LGTM

I've honestly lost track of how the data model we use for the UI works.  It
seems very complicated to maintain :-(

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:77: const
std::vector<MediaRoute>& MediaRouterBase::GetCurrentRoutes() const {
It would be better to return a copy here.  |current_routes| may get modified by
an update to the internal routes observer while a caller is iterating over it.

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:353:
routes_and_cast_modes_.insert(
I see this is existing code refactored, I just don't really understand what it
means.

A cast mode selects a subset of media sinks and their associated media sources
(if they were to be used to start a media route).  I don't see how they are
related to media routes.

[takumif, 2016-12-03 00:11:06]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[takumif, 2016-12-03 00:11:25]

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:77: const
std::vector<MediaRoute>& MediaRouterBase::GetCurrentRoutes() const {
On 2016/12/02 23:29:44, mark a. foltz wrote:
> It would be better to return a copy here.  |current_routes| may get modified
by
> an update to the internal routes observer while a caller is iterating over it.

Done.

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2540773005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:353:
routes_and_cast_modes_.insert(
On 2016/12/02 23:29:44, mark a. foltz wrote:
> I see this is existing code refactored, I just don't really understand what it
> means.
> 
> A cast mode selects a subset of media sinks and their associated media sources
> (if they were to be used to start a media route).  I don't see how they are
> related to media routes.
> 

It's for hiding the "Cast" button in dialog if clicking it would terminate a
route and start an identical one: https://codereview.chromium.org/2040883002

[commit-bot: I haz the power, 2016-12-03 00:12:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mark a. foltz, 2016-12-03 00:25:08]

On Fri, Dec 2, 2016 at 4:11 PM, <takumif@chromium.org> wrote:

>
> https://codereview.chromium.org/2540773005/diff/80001/
> chrome/browser/media/router/media_router_base.cc
> File chrome/browser/media/router/media_router_base.cc (right):
>
> https://codereview.chromium.org/2540773005/diff/80001/
> chrome/browser/media/router/media_router_base.cc#newcode77
> chrome/browser/media/router/media_router_base.cc:77: const
> std::vector<MediaRoute>& MediaRouterBase::GetCurrentRoutes() const {
> On 2016/12/02 23:29:44, mark a. foltz wrote:
> > It would be better to return a copy here. |current_routes| may get
> modified by
> > an update to the internal routes observer while a caller is iterating
> over it.
>
> Done.
>
> https://codereview.chromium.org/2540773005/diff/80001/
> chrome/browser/ui/webui/media_router/media_router_ui.cc
> File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):
>
> https://codereview.chromium.org/2540773005/diff/80001/
> chrome/browser/ui/webui/media_router/media_router_ui.cc#newcode353
> chrome/browser/ui/webui/media_router/media_router_ui.cc:353:
> routes_and_cast_modes_.insert(
> On 2016/12/02 23:29:44, mark a. foltz wrote:
> > I see this is existing code refactored, I just don't really understand
> what it
> > means.
> >
> > A cast mode selects a subset of media sinks and their associated media
> sources
> > (if they were to be used to start a media route). I don't see how
> they are
> > related to media routes.
> >
>
> It's for hiding the "Cast" button in dialog if clicking it would
> terminate a route and start an identical one:
> https://codereview.chromium.org/2040883002
>

OK.  That makes sense, although I think it would only apply to the
mirroring cast modes.  I am generally concerned about the amount of
bookkeeping required for our UI and the resulting code complexity.

m.

-- 
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.

[commit-bot: I haz the power, 2016-12-03 01:42:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 01:42:18]

Dry run: This issue passed the CQ dry run.

[takumif, 2016-12-05 18:01:59]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 18:02:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 18:43:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 18:43:10]

Dry run: This issue passed the CQ dry run.

[takumif, 2016-12-05 18:44:44]

The CQ bit was checked by takumif@chromium.org

[takumif, 2016-12-05 18:44:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2540773005/#ps100001
(title: "Address Mark's comment")

[commit-bot: I haz the power, 2016-12-05 18:45:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 18:50:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480963484750270,
"parent_rev": "192c40599aca48754ae2c35d0b3951f3e3199e92", "commit_rev":
"e0476ee192a3069b63373b0f5facc4de4f56a8d0"}

[commit-bot: I haz the power, 2016-12-05 18:50:35]

Description was changed from

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
==========

to

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
==========

[commit-bot: I haz the power, 2016-12-05 18:50:36]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-05 18:52:17]

Description was changed from

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361
==========

to

==========
Add GetCurrentRoutes() to MediaRouter API, ensure dialog has routes at init

This CL adds a method to the MediaRouter interface to get the current media
routes synchronously. We call this if MediaRouterUI hasn't observed routes
updates yet when the dialog WebUI asks for initial data, to ensure that WebUI
has routes information at initialization.

BUG=667361

Committed: https://crrev.com/21ad18d7d165fe60792c5c5e3c56b0647324c03c
Cr-Commit-Position: refs/heads/master@{#436366}
==========

[commit-bot: I haz the power, 2016-12-05 18:52:18]

Patchset 5 (id:??) landed as
https://crrev.com/21ad18d7d165fe60792c5c5e3c56b0647324c03c
Cr-Commit-Position: refs/heads/master@{#436366}