Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1415103006:
Non-Local Join for Media Router and Presentation API (Closed)
|
Created:
5 years, 1 month ago by matt.boetger Modified:
4 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, arv+watch_chromium.org, ben+mojo_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, feature-media-reviews_chromium.org, jonnymack, mcasas+watch_chromium.org, media-router+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNon-Local Join for Media Router and Presentation API
The Cast SDK has a concept of non-local joins. This means
that if a Cast session is started on Device A (e.g. an
Android phone), a user should be able to join the session
via a webpage in Chrome on Device B. This requires the
idea of "joinable" routes to enter into the media router.
The route queries need a context for this to make sense,
and that context is the page's |media_source|. If the
route can be joined by the page (given the page's
media_source), then a list of route_ids (that are a subset
of the routes returned in OnRouteUpdated) signify what
routes are joinable. The UI should present a "Join" button
if the currently viewed route is joinable.
Current Screenshots for MediaRoute Dialog
Stop And Join - LTR:
https://screenshot.googleplex.com/X814uW7esBU
Stop Only - LTR:
https://screenshot.googleplex.com/iybKZd1aABh
Stop And Join - RTL:
https://screenshot.googleplex.com/ffKqawTFgTF
Stop Only - RTL:
https://screenshot.googleplex.com/HWbR94H5Qef
Committed: https://crrev.com/5628601c63e07c7aa04d3e62eaeb90b2d60ea53a
Cr-Commit-Position: refs/heads/master@{#368260}
Patch Set 1 : Removing Logging Statements #
Total comments: 4
Patch Set 2 : Auto-Formatting Fixes #
Total comments: 11
Patch Set 3 : Added routes query set #Patch Set 4 : Presentation ID created by Media Router #Patch Set 5 : Added Unit Tests #
Total comments: 34
Patch Set 6 : Android media router interface changes #Patch Set 7 : Refactored to use Route Ids for joinable routes #Patch Set 8 : Ready for Review #
Total comments: 30
Patch Set 9 : Review Fixes #
Total comments: 32
Patch Set 10 : Review Fixes 2 #
Total comments: 70
Patch Set 11 : Review Fixes 3 * NO UI Work #Patch Set 12 : Review Fixes 3 * WITH UI Changes #
Total comments: 79
Patch Set 13 : Review Fixes 4 #Patch Set 14 : UI Fixes from Jonny's Feedback #Patch Set 15 : Removing default parameter for OnRoutesUpdated #
Total comments: 6
Patch Set 16 : Backwards Compatibility #
Total comments: 38
Patch Set 17 : Added WakeReason UMA Analytics and fixed comments #Patch Set 18 : Rebasing #Patch Set 19 : Rebase before commit #Patch Set 20 : Fixing ChromeOS System Tray Test #Messages
Total messages: 91 (42 generated)
Patchset #1 (id:1) has been deleted
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
Overall the communication between the WebUI and C++ code for the canJoin bit feels a bit weird. I wonder if we could somehow obtain this bit for each route during routes query, perhaps by passing in the MediaSource to join with. It will likely require a change on the MRPM side. There are a lot of unrelated formatting changes. They should be reverted. https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:208: canJoin_: { not needed? https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:765: media_router.browserApi.canJoinRoute(route); This flow seems a bit weird to me. It feels like canJoin_ shouldn't be part of the container. I wonder if we can compute this earlier, perhaps as part of the routes query. https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:379: route_response_callbacks.push_back( We shouldn't need this callback. It's only for resolving a start() request which brought up this dialog. Maybe this code could be refactored somehow with CreateRoute()?
https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:379: route_response_callbacks.push_back( On 2015/11/03 19:28:03, imcheng1 wrote: > We shouldn't need this callback. It's only for resolving a start() request which > brought up this dialog. Maybe this code could be refactored somehow with > CreateRoute()? Yeah, this was obviously copy/pasted from CreateRoue. I can take out the entire if statement if we don't need the presentation_service_delegate_ callback too.
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. ***********STILL WORK IN PROGRESS***************** BUG= ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. ***********STILL WORK IN PROGRESS***************** BUG= ==========
vadimgo@chromium.org changed reviewers: + vadimgo@chromium.org
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:40002) has been deleted
boetger@chromium.org changed reviewers: + mfoltz@chromium.org
Looks okay as a first cut. Where is the API for the MRP to give permission to the page to join the route? https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.css (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.css:14: #join-route-button { Where are the mocks for this? Barring that, can you post a screen shot? https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:17: <paper-button raised id="join-route-button" class="button" Where is the conditional that determines when this button is shown? https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... chrome/browser/resources/media_router/media_router.js:95: console.log('media router ui join clicked: ' + JSON.stringify(data.detail)); This probably should be removed before submitting. https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:355: // There are 3 cases. In all cases the MediaRouterUI will need to be notified. This comment block needs to be rewritten to discuss the join case. https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:381: std::string presentation_id("non-local-join_"); I don't think the UI should be manufacturing presentation ids. imcheng@ do you have a suggestion on where this logic should live? https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:382: presentation_id += route_id; What if there are simultaneous requests to join the same route?
This is still a work in progress. I'm currently working on the condition responsible for showing/hiding the join button as agreed on by Derek. UI work will need to be cleaned up. Ideally, I'd like the join button to be to the left of the close route button on the same line. I'd gladly make a mock if you let me know with what tool you'd like it created. https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.css (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.css:14: #join-route-button { On 2015/11/09 19:12:31, mark a. foltz wrote: > Where are the mocks for this? Barring that, can you post a screen shot? I have no mocks and this is crude right now. It just replicates the close route button. This will need to be fixed. I was more concerned with the SDK and Chrome code. https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:17: <paper-button raised id="join-route-button" class="button" On 2015/11/09 19:12:31, mark a. foltz wrote: > Where is the conditional that determines when this button is shown? I currently am working with Derek to make sure the semantics for this condition makes sense as is available when the dialog is instantiated. https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/resource... chrome/browser/resources/media_router/media_router.js:95: console.log('media router ui join clicked: ' + JSON.stringify(data.detail)); On 2015/11/09 19:12:31, mark a. foltz wrote: > This probably should be removed before submitting. Agreed. This was just for debugging. https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:381: std::string presentation_id("non-local-join_"); On 2015/11/09 19:12:31, mark a. foltz wrote: > I don't think the UI should be manufacturing presentation ids. imcheng@ do you > have a suggestion on where this logic should live? If the router is to create the presentation Id, I might have to change the media router interface to have a different, 'non-local' join functionality because the auto-join route method would break if I move the creation into the JoinRoute method.
https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:381: std::string presentation_id("non-local-join_"); On 2015/11/09 19:24:35, matt.boetger wrote: > On 2015/11/09 19:12:31, mark a. foltz wrote: > > I don't think the UI should be manufacturing presentation ids. imcheng@ do > you > > have a suggestion on where this logic should live? > > If the router is to create the presentation Id, I might have to change the media > router interface to have a different, 'non-local' join functionality because the > auto-join route method would break if I move the creation into the JoinRoute > method. I can see 2 ways to make that work: 1) Add a JoinRouteByRouteId method in MediaRouter that takes a MediaRoute::Id instead of presentation id. MediaRouterMojImpl / MediaRouterAndroid will then generate the presentation id with a special prefix ("mr_joinByRouteId_") and call to MRPM with the special presentation id. The MRPs will have to respect the pres id. 2. Or we can be more explicit and add the JoinRouteByRouteId method to both MediaRouter and MRPM. I prefer #2 as it is more explicit and avoids the use of hacks. The JoinRouteByRouteId MRP implementation should not be too complicated either.
Patchset #3 (id:90001) has been deleted
Patchset #4 (id:130001) has been deleted
Overall, I think it looks good. Did you find a reasonable way to implement the new routes query on the MRP side? Couple of high level comments: - Instead of passing a second list of MediaRoute objects back to MR, could we pass route ids instead? It can be assumed that joinable routes are a subset of all routes. - Some UX clarification on what should happen while a join request is in progress, and what should happen after, would be good. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:164: // StopObservingMediaRoutes() is called. Please update comments. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:167: // Stops querying the state of all media routes. ditto https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:218: array<MediaRoute> joinableRoutes); +2 indent https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:218: array<MediaRoute> joinableRoutes); Since joinableRoutes is a subset of routes, would it be possible for it to be just route ids? https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:68: : MediaRoutesObserver(router, source_id), It looks like you can do MediaRoutesObserver(router) here since you have already added a default value for |source_id| in MediaRoutesObserver's constructor. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:141: const MediaSource::Id source_id = std::string("")); |source_id| is not needed? https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:22: const MediaSource::Id source_id = std::string("")); const MediaSource::Id& source_id = MediaSource::Id() https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:25: // Invoked when the list of routes and their associated sinks have been Could you update comments? https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:31: const MediaSource::Id source_id, Is |source_id| needed? MR should ensure that |source_id| == |source_id_|. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:138: joinableRouteList: { I am a bit confused by this. Why do we have a separate joinableRouteList if we added a canJoin bit to media_rotuer.Route? https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:657: route['canJoin'] = false; Prefer adding canJoin as a field in Route class. Default value can be false. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:19: hidden$="[[!route.canJoin]]" Maybe more of a UX question: do we want to show the join button only in DEFAULT cast mode? https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:55: value: loadTimeData.getString('joinCastingButton'), joinButton ? https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:95: console.log('media router ui join clicked: ' + JSON.stringify(data.detail)); nit: remove logging https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:92: container.routeList = routeList; I would prefer an updateRouteList method in |container| that takes routeList and a list of joinable route ids. Then inside that method it will set the canJoin bits on a subset of routes accordingly. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:389: media_router_ui_->JoinRoute(sink_id, route_id); Do we need to check HasPendingRouteRequest() similar to OnCreateRoute above?
Patchset #6 (id:190001) has been deleted
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. ***********STILL WORK IN PROGRESS***************** BUG= ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. ==========
https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:164: // StopObservingMediaRoutes() is called. On 2015/11/19 18:55:08, imcheng1 wrote: > Please update comments. Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:167: // Stops querying the state of all media routes. On 2015/11/19 18:55:08, imcheng1 wrote: > ditto Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:218: array<MediaRoute> joinableRoutes); On 2015/11/19 18:55:08, imcheng1 wrote: > +2 indent Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:218: array<MediaRoute> joinableRoutes); On 2015/11/19 18:55:08, imcheng1 wrote: > Since joinableRoutes is a subset of routes, would it be possible for it to be > just route ids? Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:68: : MediaRoutesObserver(router, source_id), On 2015/11/19 18:55:08, imcheng1 wrote: > It looks like you can do MediaRoutesObserver(router) here since you have already > added a default value for |source_id| in MediaRoutesObserver's constructor. Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:141: const MediaSource::Id source_id = std::string("")); On 2015/11/19 18:55:08, imcheng1 wrote: > |source_id| is not needed? Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:22: const MediaSource::Id source_id = std::string("")); On 2015/11/19 18:55:08, imcheng1 wrote: > const MediaSource::Id& source_id = MediaSource::Id() Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:25: // Invoked when the list of routes and their associated sinks have been On 2015/11/19 18:55:08, imcheng1 wrote: > Could you update comments? Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:31: const MediaSource::Id source_id, On 2015/11/19 18:55:08, imcheng1 wrote: > Is |source_id| needed? MR should ensure that |source_id| == |source_id_|. Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:138: joinableRouteList: { On 2015/11/19 18:55:08, imcheng1 wrote: > I am a bit confused by this. Why do we have a separate joinableRouteList if we > added a canJoin bit to media_rotuer.Route? It depends where and when you want to set the 'canJoin' bit. I'm doing it in this class. I can move this up into the media_router_interface.js if you prefer, but that class seemed like it was strictly an interface between the UI and the Media Router. In the end, I thought the logic for what to do with those route ids should rest with the UI code. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:657: route['canJoin'] = false; On 2015/11/19 18:55:08, imcheng1 wrote: > Prefer adding canJoin as a field in Route class. Default value can be false. Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:19: hidden$="[[!route.canJoin]]" On 2015/11/19 18:55:08, imcheng1 wrote: > Maybe more of a UX question: do we want to show the join button only in DEFAULT > cast mode? In the code, the assumption is made that only the DEFAULT cast mode will be joinable - so I think it's a fair assumption for the UX as well. However, when you click on a route it shows the route details and it may be confusing to not show the join option as well - because that option is available otherwise, but not now - could be confusing to a user. I can see it either way. I still haven't heard back from the UX folks. I'll try to ping them again. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:55: value: loadTimeData.getString('joinCastingButton'), On 2015/11/19 18:55:08, imcheng1 wrote: > joinButton ? Same could be said for the stop button - but I wanted to be consistent. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:95: console.log('media router ui join clicked: ' + JSON.stringify(data.detail)); On 2015/11/19 18:55:08, imcheng1 wrote: > nit: remove logging Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:92: container.routeList = routeList; On 2015/11/19 18:55:08, imcheng1 wrote: > I would prefer an updateRouteList method in |container| that takes routeList and > a list of joinable route ids. Then inside that method it will set the canJoin > bits on a subset of routes accordingly. Done. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:389: media_router_ui_->JoinRoute(sink_id, route_id); On 2015/11/19 18:55:08, imcheng1 wrote: > Do we need to check HasPendingRouteRequest() similar to OnCreateRoute above? Done.
boetger@chromium.org changed reviewers: + avayvod@chromium.org
I reviewed only up to chrome/browser/media/router. Here are my comments so far. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:55: value: loadTimeData.getString('joinCastingButton'), On 2015/11/24 19:45:24, matt.boetger wrote: > On 2015/11/19 18:55:08, imcheng1 wrote: > > joinButton ? > > Same could be said for the stop button - but I wanted to be consistent. For stop button, it is named so because the button says "Stop casting". Since the join button will say "Join", joinButton is the right name for the resource. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:272: base::ObserverList<MediaRoutesObserver>* observer_list = Use auto* so it's consistent with below. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:274: if (!observer_list) { It looks like we don't currently support joinable routes in the android implementation because the implementation to notify observers of joinable routes below is incorrect. There's also no need to support joinable routes on android. I would suggest reverting the change here and then add something like: if (!source_id.empty()) NOTIMPLEMENTED() << "Joinable routes query not implemented." https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:88: router_ = static_cast<MediaRouter*>( Can the static_cast be removed? https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:115: media_source_id, You might want to check with Anton/Mounir if this is correct behavior. It looks like the previous version has some MediaRoutes logic happening even before this function is called. Either way, doesn't look like you need to pass in |media_source_id| since android doesn't need to know joinable routes. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:171: MediaRouterDialogControllerAndroid:: This looks copied from MediaRouterUI. I am not sure if android needs that logic because I am guessing they send back only routes that can be displayed here in the first place. But it would be good to check with Anton/Mounir. Otherwise, it would be good to refactor this into chrome/browser/media/router. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:200: if (std::find(joinable_route_ids.begin(), Presumably the MRP would only consider a route as joinable only if for_display is true? https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.h (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.h:92: MediaRouter* router_; MediaRouter* const router_; https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:167: // StopObservingMediaRoutes() is called. nit: some of this can go in previous line? https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:220: // Called when list of routes has been updated. Could you pleae update documentation? https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:221: DCHECK(!media_source.is_null()); This DCHECK is not needed since its non-nullness is guaranteed by mojo. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:236: for (size_t i = 0; i < joinable_route_ids.size(); ++i) { nit: rm brace in this for loop and above https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:241: setRouteList: function(routeList, joinableRouteIdList) { I feel like we shouldn't have to fill in the canJoin bit here. Instead, it can be done in MediaRouterWebUIMessageHandler. Then you don't need to pass |joinableRouteIdList| into WebUI. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:93: MediaRouter* router, const MediaSource::Id source_id, const MediaSource::Id&
Re: android, I feel that we can't really implement manual join on Android which means I'd rather just leave the android code as it is with minimal changes (like NOTIMPLEMENTED-ing pure virtual methods for compiling). We can revisit adding some behavior there later. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:274: if (!observer_list) { On 2015/11/26 at 00:49:45, imcheng1 wrote: > It looks like we don't currently support joinable routes in the android implementation because the implementation to notify observers of joinable routes below is incorrect. There's also no need to support joinable routes on android. > > I would suggest reverting the change here and then add something like: > > if (!source_id.empty()) > NOTIMPLEMENTED() << "Joinable routes query not implemented." So actually, on Android it doesn't seem like we can identify running sessions although we can try to join each device with a particular application id and if it succeeds it means the route was joinable :) By default, however, the Android SDK tries to join the application if it's running, so we could at least tell the MR if the new route was a fresh session or if it joined an already running application. Not sure if it's something MR needs though. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.h (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.h:69: // Callback to the owning MediaRouterUI instance. We don't have MediaRouterUI on Android.
I'll look at this after you've addressed comments from avayvod@ and imcheng@. Please ping me when it's ready.
Patchset #9 (id:270001) has been deleted
Patchset #9 (id:290001) has been deleted
I reverted a bunch of the Android changes. I moved the location of the 'canJoin' bit. I renamed the join button. https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/170001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:55: value: loadTimeData.getString('joinCastingButton'), On 2015/11/26 00:49:45, imcheng1 wrote: > On 2015/11/24 19:45:24, matt.boetger wrote: > > On 2015/11/19 18:55:08, imcheng1 wrote: > > > joinButton ? > > > > Same could be said for the stop button - but I wanted to be consistent. > > For stop button, it is named so because the button says "Stop casting". Since > the join button will say "Join", joinButton is the right name for the resource. Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:272: base::ObserverList<MediaRoutesObserver>* observer_list = On 2015/11/26 00:49:45, imcheng1 wrote: > Use auto* so it's consistent with below. Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:274: if (!observer_list) { On 2015/11/26 00:49:45, imcheng1 wrote: > It looks like we don't currently support joinable routes in the android > implementation because the implementation to notify observers of joinable routes > below is incorrect. There's also no need to support joinable routes on android. > > I would suggest reverting the change here and then add something like: > > if (!source_id.empty()) > NOTIMPLEMENTED() << "Joinable routes query not implemented." Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:274: if (!observer_list) { On 2015/11/26 14:43:04, whywhat wrote: > On 2015/11/26 at 00:49:45, imcheng1 wrote: > > It looks like we don't currently support joinable routes in the android > implementation because the implementation to notify observers of joinable routes > below is incorrect. There's also no need to support joinable routes on android. > > > > I would suggest reverting the change here and then add something like: > > > > if (!source_id.empty()) > > NOTIMPLEMENTED() << "Joinable routes query not implemented." > > So actually, on Android it doesn't seem like we can identify running sessions > although we can try to join each device with a particular application id and if > it succeeds it means the route was joinable :) > > By default, however, the Android SDK tries to join the application if it's > running, so we could at least tell the MR if the new route was a fresh session > or if it joined an already running application. Not sure if it's something MR > needs though. Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:88: router_ = static_cast<MediaRouter*>( On 2015/11/26 00:49:45, imcheng1 wrote: > Can the static_cast be removed? Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:115: media_source_id, On 2015/11/26 00:49:45, imcheng1 wrote: > You might want to check with Anton/Mounir if this is correct behavior. It looks > like the previous version has some MediaRoutes logic happening even before this > function is called. Either way, doesn't look like you need to pass in > |media_source_id| since android doesn't need to know joinable routes. Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:171: MediaRouterDialogControllerAndroid:: On 2015/11/26 00:49:45, imcheng1 wrote: > This looks copied from MediaRouterUI. I am not sure if android needs that logic > because I am guessing they send back only routes that can be displayed here in > the first place. But it would be good to check with Anton/Mounir. > > Otherwise, it would be good to refactor this into chrome/browser/media/router. Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:200: if (std::find(joinable_route_ids.begin(), On 2015/11/26 00:49:45, imcheng1 wrote: > Presumably the MRP would only consider a route as joinable only if for_display > is true? Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.h (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.h:69: // Callback to the owning MediaRouterUI instance. On 2015/11/26 14:43:04, whywhat wrote: > We don't have MediaRouterUI on Android. Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.h:92: MediaRouter* router_; On 2015/11/26 00:49:45, imcheng1 wrote: > MediaRouter* const router_; Reverting. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:167: // StopObservingMediaRoutes() is called. On 2015/11/26 00:49:45, imcheng1 wrote: > nit: some of this can go in previous line? Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:220: // Called when list of routes has been updated. On 2015/11/26 00:49:45, imcheng1 wrote: > Could you pleae update documentation? Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:221: DCHECK(!media_source.is_null()); On 2015/11/26 00:49:46, imcheng1 wrote: > This DCHECK is not needed since its non-nullness is guaranteed by mojo. Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:236: for (size_t i = 0; i < joinable_route_ids.size(); ++i) { On 2015/11/26 00:49:45, imcheng1 wrote: > nit: rm brace in this for loop and above Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:241: setRouteList: function(routeList, joinableRouteIdList) { On 2015/11/26 00:49:46, imcheng1 wrote: > I feel like we shouldn't have to fill in the canJoin bit here. Instead, it can > be done in MediaRouterWebUIMessageHandler. Then you don't need to pass > |joinableRouteIdList| into WebUI. Done. https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:93: MediaRouter* router, const MediaSource::Id source_id, On 2015/11/26 00:49:46, imcheng1 wrote: > const MediaSource::Id& Done.
*/android/*: lgtm
boetger@chromium.org changed reviewers: + apacible@chromium.org
Thanks for your patience Matt. It's looking much better. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:273: NOTIMPLEMENTED() << "Joinable routes query not implemented."; -2 indent, new line after https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:87: remove empty line https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:235: joinable_routes_converted.push_back(joinable_route_ids[i].get()); .get() not needed - conversion to std::string should be implicit https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:237: auto it = routes_queries_.find(media_source); It should probably be better to do this check before performing the conversions -- same with OnSinksReceived: if (no observers) { DVLOG(...) return; } [std::vector allocation / conversion] FOR_EACH_OBSERVER(...); https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:53: const char kJoinableRouteId[] = "joinableRouteId2"; "joinableRouteId" https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:39: MediaRouter* router_; would it be possible to make this MediaRouter* const router_; ? https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:19: hidden$="[[!route.canJoin]]" instead of using hidden attribute, consider using conditional template: https://www.polymer-project.org/1.0/docs/devguide/templates.html#dom-if https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:54: type: String, readOnly: true https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:81: * Fires a join-route-click event. This is called when the button to to join https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:86: joinRoute_: function() { I wonder if this should somehow go through media-router-container. The reason I say this is because we have some logic there that changes the sink being launched to be a spinner and also prevent concurrent createRoute requests. It would be nice to have that for join as well. But it depends on how the join flow should look acoording to UX. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:90: * @param {{detail: {route: string}}} data Update comment here: we are passing in a media_router.Route object. Same with onCloseRouteClick. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_data.js:125: * @param {boolean=} opt_canJoin True if this route can be joined. why not required? https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:220: router_, default_source.id(), The default source can change over the lifetime of MediaRouterUI. When it changes or is removed, |routes_observer_| should be reset. Another related suggestion is to immediately clear joinable_route_ids_ and update WebUI when default source is removed. You will need to rebase first, since the code here has changed a bit. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:361: DCHECK(origin.is_valid()); Move this DCHECK to L352 https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:100: auto it = std::find(joinable_route_ids.begin(), This can be replaced with: bool canJoin = ContainsValue(joinable_route_ids, route.media_route_id()); https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:132: EXPECT_CALL(*mock_media_router_ui_, GetRouteProviderExtensionId()).Times(1) Revert or update expectation.
I updated the code and addressed Derek's review feedback. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:273: NOTIMPLEMENTED() << "Joinable routes query not implemented."; On 2015/12/01 23:45:06, imcheng1 wrote: > -2 indent, new line after Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:87: On 2015/12/01 23:45:06, imcheng1 wrote: > remove empty line Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:235: joinable_routes_converted.push_back(joinable_route_ids[i].get()); On 2015/12/01 23:45:06, imcheng1 wrote: > .get() not needed - conversion to std::string should be implicit Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:237: auto it = routes_queries_.find(media_source); On 2015/12/01 23:45:06, imcheng1 wrote: > It should probably be better to do this check before performing the conversions > -- same with OnSinksReceived: > > if (no observers) { > DVLOG(...) > return; > } > > [std::vector allocation / conversion] > FOR_EACH_OBSERVER(...); Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:53: const char kJoinableRouteId[] = "joinableRouteId2"; On 2015/12/01 23:45:06, imcheng1 wrote: > "joinableRouteId" Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:39: MediaRouter* router_; On 2015/12/01 23:45:06, imcheng1 wrote: > would it be possible to make this MediaRouter* const router_; ? No. I tried and it started cascading the change down. If we mark this as const, then RegisterMediaRoutesObserver must be marked as const (as called from media_routes_observer.cc:17). If that's marked as const - then the "add" to routes_queries (as called from media_router_mojo_impl.cc:468) must be marked as const - which can't happen since it's base::ScopedPtrHashMap. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:19: hidden$="[[!route.canJoin]]" On 2015/12/01 23:45:06, imcheng1 wrote: > instead of using hidden attribute, consider using conditional template: > https://www.polymer-project.org/1.0/docs/devguide/templates.html#dom-if I put this in, but the unit tests seemed to not properly re-render the html when the route was changed - so I couldn't properly test it. At least it is consistent with the route-information tag using the hidden$ attribute. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:54: type: String, On 2015/12/01 23:45:06, imcheng1 wrote: > readOnly: true Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:81: * Fires a join-route-click event. This is called when the button to On 2015/12/01 23:45:06, imcheng1 wrote: > to join Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:86: joinRoute_: function() { On 2015/12/01 23:45:06, imcheng1 wrote: > I wonder if this should somehow go through media-router-container. The reason I > say this is because we have some logic there that changes the sink being > launched to be a spinner and also prevent concurrent createRoute requests. It > would be nice to have that for join as well. But it depends on how the join flow > should look acoording to UX. Since all the route information is known at the time of the join, I don't think there's any reason to have a spinner - we can immediately show all the necessary information (and basically just hide the join button). https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:90: * @param {{detail: {route: string}}} data On 2015/12/01 23:45:06, imcheng1 wrote: > Update comment here: we are passing in a media_router.Route object. Same with > onCloseRouteClick. Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_data.js:125: * @param {boolean=} opt_canJoin True if this route can be joined. On 2015/12/01 23:45:06, imcheng1 wrote: > why not required? Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:220: router_, default_source.id(), On 2015/12/01 23:45:06, imcheng1 wrote: > The default source can change over the lifetime of MediaRouterUI. When it > changes or is removed, |routes_observer_| should be reset. > > Another related suggestion is to immediately clear joinable_route_ids_ and > update WebUI when default source is removed. > > You will need to rebase first, since the code here has changed a bit. Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:361: DCHECK(origin.is_valid()); On 2015/12/01 23:45:06, imcheng1 wrote: > Move this DCHECK to L352 Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:100: auto it = std::find(joinable_route_ids.begin(), On 2015/12/01 23:45:06, imcheng1 wrote: > This can be replaced with: > > bool canJoin = ContainsValue(joinable_route_ids, route.media_route_id()); Done. https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:132: EXPECT_CALL(*mock_media_router_ui_, GetRouteProviderExtensionId()).Times(1) On 2015/12/01 23:45:06, imcheng1 wrote: > Revert or update expectation. Done.
Some feedback on the API design. If this was a consensus with imcheng@ we should probably discuss as a group. For reviewers, it would help to split this into two patches - one for the media router changes and another for the supporting UI. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:98: const MediaSource::Id& source, source_id for consistency with CreateRoute https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:99: const MediaRoute::Id& route_id, Note, the route_id should include the source_id (or the source could be looked up it). But fine to keep it here for consistency with CreateRoute() and JoinRoute(). https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:114: const std::string& presentation_id, I still think this API is weird, but I think I understand why it is the way it is. Basically the "presentation id" concept is currently not defined across devices in Chrome at present. Instead we allow an MRP to create a non local route and assign it a fake route id which is used here to implement manual join. I think one long term fix here is to define a presentation id for non-local presentations that can be advertised by the MRP. Then we can call JoinRoute with either kind of id. For local presentations we automatically join, for non-local we go through the new flow you're adding. If there's agreement, then we should update crbug.com/510297 and add a TODO here referencing it. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:230: array<string> joinable_route_ids); I would much rather add an is_joinable boolean flag to MediaRoute instead of passing a separate array. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:354: std::string presentation_id("non-local-join_"); What is this for - shouldn't the route_id already have a presentation_id? https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:356: RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoJoinRoute, I'm not sure DoJoinRoute is the right API to call here since we are joining a route without an associated presentation (at least that the caller knows about.) Or is the fake presentation id a way to re-use the existing API? https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:791: auto* routes_query = routes_queries_.get(source_id); This pattern is repeated a couple of times - please factor out methods HasRoutesObserver(source_id) and HasSinksObserver(source_id) https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:320: base::ScopedPtrHashMap<MediaSource::Id, scoped_ptr<MediaRoutesQuery>> Why do we need this map and |local_routes_observers_|?
Caveat: I didn't review the html/css/js in this round. Derek and I discussed this from an API point of view. Here are my thoughts. - Passing a separate list of joinable route ids with the route observer is the cleanest solution for now. - It should still be possible to add a media routes observer without a media source. (Imagine implementing a remote control outside the context of a specific tab/page.) - We need to be extra careful to clean up any observers created in the MRPM. It seems like each source has a separate observer list? Does stop remove one observer or all? The semantics for this need to be specified in media_router.mojom and elsewhere as needed. - IMO it would be cleaner to add ConnectRouteByRouteId() to media_router.mojom. This would mean you don't have to create any fake presentation ids to join a remote route, just have to pass the source and the route id. The calling pattern would look the same as CreateRoute() with the additional joinable_route_id argument. - Eventually I would like to refactor so that the presentation ID becomes something handled by the browser alone and not passed to or from the MRPM. Once that happens we could remove JoinRoute() from the Media Router API and all calls would go through ConnectRouteByRouteId() (with some mapping between presentation id and route id happening in the browser). This may require some new handling for the auto-join functionality, I need to think through the consequences. And this is a longer term cleanup, not in scope for this patch. - Derek suggested adding an optional presentation id to the MediaRoute object, separate from the route id. I don't remember exactly why, but I'm not opposed. - The terminology in the API now favors "Connect" over "Join" which is why I suggest ConnectRouteByRouteId. It would make less work for later renaming, at the expense of temporary inconsistency. I don't feel strongly, maybe Derek has an opinion. Some other comments noted in this pass https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:272: const std::string& source_id = observer->source_id(); Can this be inlined below? https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:156: Maybe DCHECK(joinable_route_ids.empty()) https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:99: const MediaRoute::Id& route_id, On 2015/12/03 at 19:24:23, mark a. foltz wrote: > Note, the route_id should include the source_id (or the source could be looked up it). But fine to keep it here for consistency with CreateRoute() and JoinRoute(). You can ignore this comment. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:166: // Querying will continue until StopObservingMediaRoutes() is called. The media_source should be optional correct? Please document this fact. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:171: StopObservingMediaRoutes(string media_source); What happens if the client calls Start(foo) Stop() or Start() Stop(foo) or Start(foo) Stop(bar) ? It's important that we be able to reliably turn off route updates from the MRPM. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:230: array<string> joinable_route_ids); On 2015/12/03 at 19:24:23, mark a. foltz wrote: > I would much rather add an is_joinable boolean flag to MediaRoute instead of passing a separate array. You can ignore this comment. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:325: StartObservingMediaRoutes(mojo::String(""))); Shouldn't a real media source be passed to StartObservingMediaRoutes for this test case? https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:22: const MediaSource::Id source_id = MediaSource::Id()); Default arguments are not allowed - use a setter or delegated constructor: https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-39... https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:126: if (std::find(joinable_route_ids.begin(), Shorter as ContainsValue from base/stl_util.h https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:247: routes_observer_.reset(new UIMediaRoutesObserver( I assume the UIMediaRoutesObserver dtor handles de-registration of itself. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:362: bool MediaRouterUI::JoinRoute(const MediaSink::Id& sink_id, I would like to name this ConnectRemoteRoute to make this clear this is a different code path from reconnecting to a presentation via the presentation API; i.e., PresentationRequest.connect(id). That has nothing to do with the UI https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:371: MediaSource source = Don't you need to check presentation_request_ as well? It seems like you should refactor to share code between CreateRoute() and ConnectRemoteRoute(), it looks like the main difference is what request is made to router_. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:77: base::Bind(&MediaRouterUITest::OnRoutesUpdated, Aside: I prefer that the mocks be declared in a separate class, to keep mocks exercised by the test cleanly separated from code that implements the test fixture itself. But not necessary to fix in this CL. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:98: SaveArg<1>(&filtered_route_ids)); Don't you want to assert that the joinable_route_ids are passed to OnRoutesUpdated?
Please add screenshots of the route details view (ltr and rtl) with: - Close button only - Close and Join buttons https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.css (left): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.css:6: -webkit-padding-end: 24px; +jonnymack for input on moving the button text from the 'end' (right for LTR, left for RTL) to center. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html"> What's the advantage of using iron-flex-layout rather than flexbox? Since we're also adding a new import, please note performance differences before/after your changes. Usually we check mean/median of 5-10 runs in chrome://tracing. Steps: 1. Press "Record" and select the Media Router option. Everything else can be unchecked. Start recording. 2. Open the Media Router dialog (can be in new tab). 3. Stop recording. 4. Check the "MR UI" block for load time. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:80: * route - route ID. nit: "route - route to close." Thanks for fixing L78! https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:92: * route - route ID. nit: "route - route to join." https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:193: kSetRouteList, *routes_val); nit: Move back to previous line. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:416: if (!media_router_ui_->JoinRoute(sink_id, route_id)) { Do we want to surface an error if we fail to join? This fails silently. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:36: const std::vector<MediaRoute::Id>& joinable_route_ids); nit: align indent with the |routes| parameter in the previous line. https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... File chrome/test/data/webui/media_router/route_details_tests.js (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:81: fakeSinkOne = new media_router.Sink('sink id 1', 'Living Room', What are fakeSinkOne and fakeSinkTwo used for? https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:102: // 'close-route-button' button is clicked. nit: 'join-route-button' https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:115: // unless it's hidden (join is hidden unless the route can be joined) This is a fragment; is this a continuation from the previous comment? If so, please make the comment standalone to not rely on the previous one. https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:115: // unless it's hidden (join is hidden unless the route can be joined) nit: period at the end.
Patchset #11 (id:350001) has been deleted
I brought the code up to date and made the refactorings suggested by Mark. I got some feedback and Jonny Mack and Jennifer that will be in the next patch handling UI critiques. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:272: const std::string& source_id = observer->source_id(); On 2015/12/09 00:48:16, mark a. foltz wrote: > Can this be inlined below? Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:156: On 2015/12/09 00:48:16, mark a. foltz wrote: > Maybe DCHECK(joinable_route_ids.empty()) Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:98: const MediaSource::Id& source, On 2015/12/03 19:24:23, mark a. foltz wrote: > source_id for consistency with CreateRoute Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:99: const MediaRoute::Id& route_id, On 2015/12/03 19:24:23, mark a. foltz wrote: > Note, the route_id should include the source_id (or the source could be looked > up it). But fine to keep it here for consistency with CreateRoute() and > JoinRoute(). Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:99: const MediaRoute::Id& route_id, On 2015/12/03 19:24:23, mark a. foltz wrote: > Note, the route_id should include the source_id (or the source could be looked > up it). But fine to keep it here for consistency with CreateRoute() and > JoinRoute(). Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:114: const std::string& presentation_id, On 2015/12/03 19:24:23, mark a. foltz wrote: > I still think this API is weird, but I think I understand why it is the way it > is. Basically the "presentation id" concept is currently not defined across > devices in Chrome at present. Instead we allow an MRP to create a non local > route and assign it a fake route id which is used here to implement manual join. > > I think one long term fix here is to define a presentation id for non-local > presentations that can be advertised by the MRP. Then we can call JoinRoute with > either kind of id. For local presentations we automatically join, for non-local > we go through the new flow you're adding. > > If there's agreement, then we should update crbug.com/510297 and add a TODO here > referencing it. > If we're going to a new method, 'connectRouteByRouteId' I don't see why we need to define a special presentation id for non-local joins - in fact, we can get rid of the special "non-local-join" fake presentation id. This probably needs to be thought through a little more after we see the refactoring. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:166: // Querying will continue until StopObservingMediaRoutes() is called. On 2015/12/09 00:48:16, mark a. foltz wrote: > The media_source should be optional correct? > Please document this fact. It was not my intention to make this optional. I will make it optional however. With the concern for making it possible to observe routes without a media source, do we need to also create an OnRoutesUpdated that does not take a media source? Should we make a separate overloaded call? Or perhaps a new method call completely. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:171: StopObservingMediaRoutes(string media_source); On 2015/12/09 00:48:16, mark a. foltz wrote: > What happens if the client calls > > Start(foo) > Stop() > > or > > Start() > Stop(foo) > > or > > Start(foo) > Stop(bar) > > ? > > It's important that we be able to reliably turn off route updates from the MRPM. > It was intended that if you call Start(foo) you must call Stop(foo) - exactly how the Start/Stop of the media sink queries work. I'm not sure why you'd expect any other behavior, but I will document it here. Should I add similar documentation to the Media Sink queries? https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:230: array<string> joinable_route_ids); On 2015/12/09 00:48:16, mark a. foltz wrote: > On 2015/12/03 at 19:24:23, mark a. foltz wrote: > > I would much rather add an is_joinable boolean flag to MediaRoute instead of > passing a separate array. > > You can ignore this comment. Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:230: array<string> joinable_route_ids); On 2015/12/03 19:24:23, mark a. foltz wrote: > I would much rather add an is_joinable boolean flag to MediaRoute instead of > passing a separate array. Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:354: std::string presentation_id("non-local-join_"); On 2015/12/03 19:24:23, mark a. foltz wrote: > What is this for - shouldn't the route_id already have a presentation_id? Since this is non-local, the route_id contains the cast session id (since there is no presentation id) - this is used to join. Since we are going to the connectRouteByRouteId we can remove this. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:356: RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoJoinRoute, On 2015/12/03 19:24:23, mark a. foltz wrote: > I'm not sure DoJoinRoute is the right API to call here since we are joining a > route without an associated presentation (at least that the caller knows about.) > Or is the fake presentation id a way to re-use the existing API? Yes, the fake presentation id was a way to re-use the existing API. Since we are going to the "connectRouteByRouteId" method on the media router - we can remove this. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:791: auto* routes_query = routes_queries_.get(source_id); On 2015/12/03 19:24:23, mark a. foltz wrote: > This pattern is repeated a couple of times - please factor out methods > HasRoutesObserver(source_id) and HasSinksObserver(source_id) Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:320: base::ScopedPtrHashMap<MediaSource::Id, scoped_ptr<MediaRoutesQuery>> On 2015/12/03 19:24:23, mark a. foltz wrote: > Why do we need this map and |local_routes_observers_|? |local_routes_observers| only deals with local, displayable routes to change media router icon states. This list existed before along with a list of route_observers. I think refactoring this is outside the scope of this change. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:325: StartObservingMediaRoutes(mojo::String(""))); On 2015/12/09 00:48:17, mark a. foltz wrote: > Shouldn't a real media source be passed to StartObservingMediaRoutes for this > test case? No, because this StartObservingMediaRoutes comes from the MediaRouterMojoImpl::RouteResponseReceived method in which it resets it's own local routes_observer without a media source. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:22: const MediaSource::Id source_id = MediaSource::Id()); On 2015/12/09 00:48:17, mark a. foltz wrote: > Default arguments are not allowed - use a setter or delegated constructor: > > https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-39... Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:126: if (std::find(joinable_route_ids.begin(), On 2015/12/09 00:48:17, mark a. foltz wrote: > Shorter as ContainsValue from base/stl_util.h Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:247: routes_observer_.reset(new UIMediaRoutesObserver( Yes, all route observers handle de-registration of itself via the MediaRoutesObserver destructor. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:362: bool MediaRouterUI::JoinRoute(const MediaSink::Id& sink_id, On 2015/12/09 00:48:17, mark a. foltz wrote: > I would like to name this ConnectRemoteRoute to make this clear this is a > different code path from reconnecting to a presentation via the presentation > API; i.e., PresentationRequest.connect(id). That has nothing to do with the UI Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:371: MediaSource source = On 2015/12/09 00:48:17, mark a. foltz wrote: > Don't you need to check presentation_request_ as well? > > It seems like you should refactor to share code between CreateRoute() and > ConnectRemoteRoute(), it looks like the main difference is what request is made > to router_. Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:77: base::Bind(&MediaRouterUITest::OnRoutesUpdated, On 2015/12/09 00:48:17, mark a. foltz wrote: > Aside: I prefer that the mocks be declared in a separate class, to keep mocks > exercised by the test cleanly separated from code that implements the test > fixture itself. But not necessary to fix in this CL. Acknowledged. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:98: SaveArg<1>(&filtered_route_ids)); On 2015/12/09 00:48:17, mark a. foltz wrote: > Don't you want to assert that the joinable_route_ids are passed to > OnRoutesUpdated? That's what this code is doing.
Performed the UI tweaks suggested by Jennifer and Jonny. Here are the screenshots: Join And Close - LTR: https://screenshot.googleplex.com/nEa4nyekkFp Join and Close - RTL: https://screenshot.googleplex.com/ftjcVatfbTV Close - LTR: https://screenshot.googleplex.com/6poWavGk82K Close - RTL: https://screenshot.googleplex.com/ULLSaKFXjPD https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.css (left): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.css:6: -webkit-padding-end: 24px; On 2015/12/09 19:56:24, apacible wrote: > +jonnymack for input on moving the button text from the 'end' (right for LTR, > left for RTL) to center. I spoke with jonnymack via email - moving the text back to 'end'. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html"> On 2015/12/09 19:56:25, apacible wrote: > What's the advantage of using iron-flex-layout rather than flexbox? > > Since we're also adding a new import, please note performance differences > before/after your changes. Usually we check mean/median of 5-10 runs in > chrome://tracing. > > Steps: > 1. Press "Record" and select the Media Router option. Everything else can be > unchecked. Start recording. > 2. Open the Media Router dialog (can be in new tab). > 3. Stop recording. > 4. Check the "MR UI" block for load time. I had originally used flexbox, but after one of my syncs to use the latest code - that stopped working, so I had put in the iron-flex-layout. It seems to be working again, so I'll remove the iron-flex-layout. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:80: * route - route ID. On 2015/12/09 19:56:25, apacible wrote: > nit: "route - route to close." > > Thanks for fixing L78! Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:92: * route - route ID. On 2015/12/09 19:56:25, apacible wrote: > nit: "route - route to join." Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:193: kSetRouteList, *routes_val); On 2015/12/09 19:56:25, apacible wrote: > nit: Move back to previous line. Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:416: if (!media_router_ui_->JoinRoute(sink_id, route_id)) { On 2015/12/09 19:56:25, apacible wrote: > Do we want to surface an error if we fail to join? This fails silently. There is a TODO for imcheng in the CreateRoute method as well, I'll add a todo here as well. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:36: const std::vector<MediaRoute::Id>& joinable_route_ids); On 2015/12/09 19:56:25, apacible wrote: > nit: align indent with the |routes| parameter in the previous line. Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... File chrome/test/data/webui/media_router/route_details_tests.js (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:81: fakeSinkOne = new media_router.Sink('sink id 1', 'Living Room', On 2015/12/09 19:56:25, apacible wrote: > What are fakeSinkOne and fakeSinkTwo used for? Nothing anymore. Thanks for catching. https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:102: // 'close-route-button' button is clicked. On 2015/12/09 19:56:25, apacible wrote: > nit: 'join-route-button' Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:115: // unless it's hidden (join is hidden unless the route can be joined) On 2015/12/09 19:56:25, apacible wrote: > nit: period at the end. Done. https://codereview.chromium.org/1415103006/diff/330001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_details_tests.js:115: // unless it's hidden (join is hidden unless the route can be joined) On 2015/12/09 19:56:25, apacible wrote: > This is a fragment; is this a continuation from the previous comment? If so, > please make the comment standalone to not rely on the previous one. Done.
Some minor comments remain (although I still am puzzled by a couple of things in the APIs). I need to take a closer look at the unit tests to satisfy myself that there is good coverage. One thing I'm not a big fan of, is that a client (i.e. the media router UI) now has to manage both a sinks query and a routes query each time the default presentation changes for the WebContents. This is not a common case for Cast but it can happen in general. I remember back in the original design we tried to combine the two APIs for observing sinks and routes into one, and it ended up requiring a lot of extra code to detangle the results for presentation. Now that we have both routes and sinks tied to a specific media source, maybe it's time to rethink the pattern again. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:166: // Querying will continue until StopObservingMediaRoutes() is called. On 2015/12/15 at 19:21:20, matt.boetger wrote: > On 2015/12/09 00:48:16, mark a. foltz wrote: > > The media_source should be optional correct? > > Please document this fact. > > It was not my intention to make this optional. I will make it optional however. > > With the concern for making it possible to observe routes without a media source, do we need to also create an OnRoutesUpdated that does not take a media source? Should we make a separate overloaded call? Or perhaps a new method call completely. I don't think so. Since we're passing back the media_source with the route update the client can match it up with the observer object on its side. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:171: StopObservingMediaRoutes(string media_source); On 2015/12/15 at 19:21:20, matt.boetger wrote: > On 2015/12/09 00:48:16, mark a. foltz wrote: > > What happens if the client calls > > > > Start(foo) > > Stop() > > > > or > > > > Start() > > Stop(foo) > > > > or > > > > Start(foo) > > Stop(bar) > > > > ? > > > > It's important that we be able to reliably turn off route updates from the MRPM. > > > > It was intended that if you call Start(foo) you must call Stop(foo) - exactly how the Start/Stop of the media sink queries work. I'm not sure why you'd expect any other behavior, but I will document it here. Should I add similar documentation to the Media Sink queries? Sure, it would be great to make this more explicit where it's not. https://codereview.chromium.org/1415103006/diff/390001/chrome/app/media_route... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/app/media_route... chrome/app/media_router_strings.grdp:77: <message name="IDS_MEDIA_ROUTER_JOIN_BUTTON" desc="Join button, which, on click, will join the current presentation session."> Nit: join the current route. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:35: // Type of callback used in |CreateRoute()| and |JoinRoute()| and Nit: use comma instead of multiple "and"s. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:98: virtual void ConnectRouteByRouteId( It would be better to keep the nomenclature consistent between the API and the documentation, so "Connects to" instead of "Joins", here and elsewhere in the patch. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:156: string presentation_id, Why does the mojo interface take a presentation id while the MediaRouter does not? It looks like you are generating a GUID and passing it in. Is it used by the MRPM or can it be removed? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:181: // The |media_source| should be considered when returning joinable What is the meaning of an empty media_source? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:18: // MediaSinks have been updated. The meaning of the media_source ctor parameter needs to be documented. (Both when set and when omitted.) https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:23: explicit MediaRoutesObserver(MediaRouter* router, explicit is not necessary for a two-arg ctor. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (left): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:223: // Register for MediaRoute updates. How does the dialog get route updates when there is no default presentation request? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:138: joinable_route_ids_for_display.push_back(route.media_route_id()); Since the only way to connect to a remote route is via the dialog, does it make any sense for the MRP to return a joinable route that is not for display? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:277: routes_observer_.reset(); Don't you need to instantiate a routes observer without a source_id to continue to get route updates? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:106: bool CreateRouteOrConnectRoute(const MediaSink::Id& sink_id, CreateOrConnectRoute? https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:620: * @param {!string} sourceUrn Adding a new required parameter is specifically not compatible with older versions of the component extension. So you'll want to ensure there is an extension release before this hits dev channel.
https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:243: joinable_routes_converted.push_back(joinable_route_ids[i]); If you just want to copy the elements of joinable_route_ids then pass it to the ctor of joinable_routes_converted.
https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:157: DCHECK(joinable_route_ids.empty()); This DCHECK is not needed. |joinable_route_ids| is not being used anyway. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:36: // |ConnectRouteByRouteId|. Callback is Also ConnectRouteByRouteId() for consistency. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:181: // The |media_source| should be considered when returning joinable +1. It would be good to document what is the purpose of |media_source| instead. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:236: std::vector<MediaRoute::Id> joinable_routes_converted; nit: This should go after the first for-loop https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:934: return routes_query != NULL && routes_query->observers.might_have_observers(); nit: get rid of != NULL https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:137: if (ContainsValue(joinable_route_ids, route.media_route_id())) { Do we need to hide the join button if the route was creating in this tab and page? We'd need to think of a strategy to do so for both cast and the general case. Note that we already have information on currently active routes in each RenderFrameHost in PresentationServiceDelegateImpl.
Patchset #13 (id:410001) has been deleted
Minor comments for resources/ and some general formatting. I need to take a closer look at the rest of the UI code. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.css (left): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.css:6: -webkit-padding-end: 24px; On 2015/12/16 00:21:08, matt.boetger wrote: > On 2015/12/09 19:56:24, apacible wrote: > > +jonnymack for input on moving the button text from the 'end' (right for LTR, > > left for RTL) to center. > > I spoke with jonnymack via email - moving the text back to 'end'. Acknowledged. https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/330001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:416: if (!media_router_ui_->JoinRoute(sink_id, route_id)) { On 2015/12/16 00:21:08, matt.boetger wrote: > On 2015/12/09 19:56:25, apacible wrote: > > Do we want to surface an error if we fail to join? This fails silently. > > There is a TODO for imcheng in the CreateRoute method as well, I'll add a todo > here as well. Acknowledged. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:381: std::vector<MediaRoute::Id>())); nit: Does this fit on the previous line? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:413: std::vector<MediaRoute::Id>())); nit: Does this fit on the previous line? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:314: mojo::String(kRouteId), _, nit: fix indentation. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:339: mojo::String(kRouteId), _, nit: fix indentation here and L340. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:356: route_response_callbacks); nit: fix indentation. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:645: mojo::String(media_source.id()))).Times(1); nit: fix indentation here and L648. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.css (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. nit: alphabetize selectors. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:13: <div class="flex-button-container"> class => id https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:13: <div class="flex-button-container"> Rename to something more descriptive -- maybe "route-action-buttons"? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:68: joinRoute_: function() { nit: alphabetize after closeRoute_. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:386: route_response_callbacks); nit: fix indentation here and L389. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:395: const MediaRoute::Id& route_id) { nit: fix indentation. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:127: base::Unretained(&mock_callback)))); nit: fix indentation. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:156: nit: remove extra newline. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:521: sourceUrn, routeId, presentationId, origin, tabId) nit: fix indentation for L521-527.
Patchset #13 (id:430001) has been deleted
The next round of fixes addressing Jennifer's, Derek's and Mark's feedback. https://codereview.chromium.org/1415103006/diff/390001/chrome/app/media_route... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/app/media_route... chrome/app/media_router_strings.grdp:77: <message name="IDS_MEDIA_ROUTER_JOIN_BUTTON" desc="Join button, which, on click, will join the current presentation session."> On 2015/12/16 07:17:34, mark a. foltz wrote: > Nit: join the current route. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:381: std::vector<MediaRoute::Id>())); On 2015/12/18 22:34:19, apacible wrote: > nit: Does this fit on the previous line? Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_android.cc:413: std::vector<MediaRoute::Id>())); On 2015/12/18 22:34:19, apacible wrote: > nit: Does this fit on the previous line? Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... File chrome/browser/media/android/router/media_router_dialog_controller_android.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/a... chrome/browser/media/android/router/media_router_dialog_controller_android.cc:157: DCHECK(joinable_route_ids.empty()); On 2015/12/17 02:30:57, imcheng1 wrote: > This DCHECK is not needed. |joinable_route_ids| is not being used anyway. +mfoltz This was asked for by mfoltz@ https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:35: // Type of callback used in |CreateRoute()| and |JoinRoute()| and On 2015/12/16 07:17:34, mark a. foltz wrote: > Nit: use comma instead of multiple "and"s. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:36: // |ConnectRouteByRouteId|. Callback is On 2015/12/17 02:30:57, imcheng1 wrote: > Also ConnectRouteByRouteId() for consistency. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:98: virtual void ConnectRouteByRouteId( On 2015/12/16 07:17:34, mark a. foltz wrote: > It would be better to keep the nomenclature consistent between the API and the > documentation, so "Connects to" instead of "Joins", here and elsewhere in the > patch. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:156: string presentation_id, On 2015/12/16 07:17:34, mark a. foltz wrote: > Why does the mojo interface take a presentation id while the MediaRouter does > not? > It looks like you are generating a GUID and passing it in. Is it used by the > MRPM or can it be removed? It's the same reason CreateRoute is creating a presentation ID. With a non-local join, a presentation session does not actually exist yet, and thus to make sense within the framework, a presentation ID needs to be generated. This presentation ID is passed back to to the MediaRouteResponseCallbacks the same way CreateRoute does. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:181: // The |media_source| should be considered when returning joinable On 2015/12/16 07:17:34, mark a. foltz wrote: > What is the meaning of an empty media_source? Added an explanation in the comments. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:181: // The |media_source| should be considered when returning joinable On 2015/12/17 02:30:57, imcheng1 wrote: > +1. It would be good to document what is the purpose of |media_source| instead. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:236: std::vector<MediaRoute::Id> joinable_routes_converted; On 2015/12/17 02:30:57, imcheng1 wrote: > nit: This should go after the first for-loop Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:243: joinable_routes_converted.push_back(joinable_route_ids[i]); On 2015/12/16 07:28:05, mark a. foltz wrote: > If you just want to copy the elements of joinable_route_ids then pass it to the > ctor of joinable_routes_converted. Compiler error: no matching constructor for initialization of 'std::vector<MediaRoute::Id>' I tried to use the storage() function, but there was still inconsistencies between MediaRoute::Id and StorageType (aka mojo::String): error: no viable conversion from 'const vector<StorageType>' to 'const vector<MediaRoute::Id>' I think a TypeConverter would need to be created. Going to leave this for now, but if I'm missing something, let me know :) https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:934: return routes_query != NULL && routes_query->observers.might_have_observers(); On 2015/12/17 02:30:57, imcheng1 wrote: > nit: get rid of != NULL Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:314: mojo::String(kRouteId), _, On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:339: mojo::String(kRouteId), _, On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation here and L340. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:356: route_response_callbacks); On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl_unittest.cc:645: mojo::String(media_source.id()))).Times(1); On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation here and L648. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:18: // MediaSinks have been updated. On 2015/12/16 07:17:35, mark a. foltz wrote: > The meaning of the media_source ctor parameter needs to be documented. (Both > when set and when omitted.) Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:23: explicit MediaRoutesObserver(MediaRouter* router, On 2015/12/16 07:17:35, mark a. foltz wrote: > explicit is not necessary for a two-arg ctor. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:37: std::vector<MediaRoute::Id>()) {} Are default parameters not allowed everywhere, or just in constructors? Do I need to change this as well? https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.css (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/18 22:34:19, apacible wrote: > nit: alphabetize selectors. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:13: <div class="flex-button-container"> On 2015/12/18 22:34:19, apacible wrote: > class => id Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:13: <div class="flex-button-container"> On 2015/12/18 22:34:19, apacible wrote: > Rename to something more descriptive -- maybe "route-action-buttons"? Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:68: joinRoute_: function() { On 2015/12/18 22:34:19, apacible wrote: > nit: alphabetize after closeRoute_. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (left): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:223: // Register for MediaRoute updates. On 2015/12/16 07:17:35, mark a. foltz wrote: > How does the dialog get route updates when there is no default presentation > request? Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:137: if (ContainsValue(joinable_route_ids, route.media_route_id())) { On 2015/12/17 02:30:57, imcheng1 wrote: > Do we need to hide the join button if the route was creating in this tab and > page? We'd need to think of a strategy to do so for both cast and the general > case. Note that we already have information on currently active routes in each > RenderFrameHost in PresentationServiceDelegateImpl. Perhaps. Right now that is an implementation detail on the provider side - and I can see the argument to keep it that way so that developers have the freedom to decide what joining means in their case. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:138: joinable_route_ids_for_display.push_back(route.media_route_id()); On 2015/12/16 07:17:35, mark a. foltz wrote: > Since the only way to connect to a remote route is via the dialog, does it make > any sense for the MRP to return a joinable route that is not for display? No, the MRP should not, but how are we going to enforce that if people can create there own providers? I think this code at least prevents errors from incorrect implementations on the provider side. I could see the flip-side argument that it hides those errors from developers - but I'm not sure developers will be able to see the errors and make the connection to correct. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:277: routes_observer_.reset(); On 2015/12/16 07:17:35, mark a. foltz wrote: > Don't you need to instantiate a routes observer without a source_id to continue > to get route updates? Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:386: route_response_callbacks); On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation here and L389. Fixed on L389. Not sure what the problem is here though - the parameters line up. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:395: const MediaRoute::Id& route_id) { On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:106: bool CreateRouteOrConnectRoute(const MediaSink::Id& sink_id, On 2015/12/16 07:17:35, mark a. foltz wrote: > CreateOrConnectRoute? Done. Also made it private. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:127: base::Unretained(&mock_callback)))); On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:156: On 2015/12/18 22:34:19, apacible wrote: > nit: remove extra newline. Done. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:521: sourceUrn, routeId, presentationId, origin, tabId) On 2015/12/18 22:34:19, apacible wrote: > nit: fix indentation for L521-527. I fixed the parameter list indentation - not sure what's wrong with the rest. I'm following the formatting that's in the CreateRoute function. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:620: * @param {!string} sourceUrn On 2015/12/16 07:17:35, mark a. foltz wrote: > Adding a new required parameter is specifically not compatible with older > versions of the component extension. So you'll want to ensure there is an > extension release before this hits dev channel. The changes to onRouteUpdated also means that the extension will need to be up to date. Not sure if I can make the extension work with both - so there will be bit of time where things will be broken with the component extension until the patch hits the dev channel.
https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:37: std::vector<MediaRoute::Id>()) {} On 2015/12/18 23:37:15, matt.boetger wrote: > Are default parameters not allowed everywhere, or just in constructors? Do I > need to change this as well? Default arguments are not allowed in functions, but constructors are fine: https://google.github.io/styleguide/cppguide.html#Default_Arguments
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ==========
I removed the default argument from the OnRoutesUpdated method. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_routes_observer.h:37: std::vector<MediaRoute::Id>()) {} On 2015/12/21 20:00:35, imcheng1 wrote: > On 2015/12/18 23:37:15, matt.boetger wrote: > > Are default parameters not allowed everywhere, or just in constructors? Do I > > need to change this as well? > > Default arguments are not allowed in functions, but constructors are fine: > https://google.github.io/styleguide/cppguide.html#Default_Arguments Thanks for the link. I removed the default parameter.
Minor comments in newest patchset and PS 12, otherwise it's looking good. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:243: joinable_routes_converted.push_back(joinable_route_ids[i]); On 2015/12/18 23:37:15, matt.boetger wrote: > On 2015/12/16 07:28:05, mark a. foltz wrote: > > If you just want to copy the elements of joinable_route_ids then pass it to > the > > ctor of joinable_routes_converted. > > Compiler error: no matching constructor for initialization of > 'std::vector<MediaRoute::Id>' > > I tried to use the storage() function, but there was still inconsistencies > between MediaRoute::Id and StorageType (aka mojo::String): > > error: no viable conversion from 'const vector<StorageType>' to 'const > vector<MediaRoute::Id>' > > I think a TypeConverter would need to be created. Going to leave this for now, > but if I'm missing something, let me know :) > I think you could do joinable_route_ids.To<std::vector<std::string>>(); or something similar. See: https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... That said I don't think it's a big deal. The fact that the converter returns a std::vector means there will be some extra copying around. You also can't inline the call in FOR_EACH_OBSERVER because it's a macro that will invoke the Converter for each observer. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:138: joinable_route_ids_for_display.push_back(route.media_route_id()); On 2015/12/18 23:37:15, matt.boetger wrote: > On 2015/12/16 07:17:35, mark a. foltz wrote: > > Since the only way to connect to a remote route is via the dialog, does it > make > > any sense for the MRP to return a joinable route that is not for display? > > No, the MRP should not, but how are we going to enforce that if people can > create there own providers? I think this code at least prevents errors from > incorrect implementations on the provider side. I could see the flip-side > argument that it hides those errors from developers - but I'm not sure > developers will be able to see the errors and make the connection to correct. BTW Vadim and I talked about removing forDisplay from MR. It should be possible once we cleaned up the receiver action logic. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:290: sourceUrn, routes.map(routeToMojo_), joinableRouteIds); You can make this backwards compatible if you check if joinableRouteIds is undefined and if so use an empty array. See comment below. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:620: * @param {!string} sourceUrn On 2015/12/18 23:37:15, matt.boetger wrote: > On 2015/12/16 07:17:35, mark a. foltz wrote: > > Adding a new required parameter is specifically not compatible with older > > versions of the component extension. So you'll want to ensure there is an > > extension release before this hits dev channel. > > The changes to onRouteUpdated also means that the extension will need to be up > to date. Not sure if I can make the extension work with both - so there will be > bit of time where things will be broken with the component extension until the > patch hits the dev channel. It should be able to make this change backwards compatible. Old MR + New MRPM: sourceUrn not provided to MRPM; thus MRPM need to treat undefined as empty joinableRouteIds (which should be empty) will get dropped when it reaches media_router_bindings.js New MR + Old MRPM: sourceUrn will get dropped in MRPM joinableRouteIds not provided to media_router_bindings.js; this file should check if joinableRouteIds is undefined and send an empty array back to browser if that's the case. https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:237: routes_observer_.reset(new UIMediaRoutesObserver( This should go somewhere else now that we set up routes_observer_ in OnDefaultPresentation{Changed,Removed}. The only case where OnDefaultPresentationChanged() isn't invoked during Init*() is if the condition in L209 is false -- so I would move this line to an else block there. https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:418: if (!media_router_ui_->ConnectRemoteRoute(sink_id, route_id)) { on naming: Is there logic in the WebUI that restricts joining to remote routes? i.e., can I start a route on tab 1, then join via the dialog in tab 2?
I took Derek's latest feedback into account and made the interface work with the old version of the SDK to ensure backwards compatibility. In order to to do this, I changed the parameter ordering for OnRoutesUpdated so that the first parameters match between the two versions. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:243: joinable_routes_converted.push_back(joinable_route_ids[i]); On 2015/12/29 00:24:42, imcheng1 wrote: > On 2015/12/18 23:37:15, matt.boetger wrote: > > On 2015/12/16 07:28:05, mark a. foltz wrote: > > > If you just want to copy the elements of joinable_route_ids then pass it to > > the > > > ctor of joinable_routes_converted. > > > > Compiler error: no matching constructor for initialization of > > 'std::vector<MediaRoute::Id>' > > > > I tried to use the storage() function, but there was still inconsistencies > > between MediaRoute::Id and StorageType (aka mojo::String): > > > > error: no viable conversion from 'const vector<StorageType>' to 'const > > vector<MediaRoute::Id>' > > > > I think a TypeConverter would need to be created. Going to leave this for > now, > > but if I'm missing something, let me know :) > > > > I think you could do joinable_route_ids.To<std::vector<std::string>>(); or > something similar. See: > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... > > That said I don't think it's a big deal. The fact that the converter returns a > std::vector means there will be some extra copying around. You also can't inline > the call in FOR_EACH_OBSERVER because it's a macro that will invoke the > Converter for each observer. Done. https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/390001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:138: joinable_route_ids_for_display.push_back(route.media_route_id()); On 2015/12/29 00:24:42, imcheng1 wrote: > On 2015/12/18 23:37:15, matt.boetger wrote: > > On 2015/12/16 07:17:35, mark a. foltz wrote: > > > Since the only way to connect to a remote route is via the dialog, does it > > make > > > any sense for the MRP to return a joinable route that is not for display? > > > > No, the MRP should not, but how are we going to enforce that if people can > > create there own providers? I think this code at least prevents errors from > > incorrect implementations on the provider side. I could see the flip-side > > argument that it hides those errors from developers - but I'm not sure > > developers will be able to see the errors and make the connection to correct. > > > BTW Vadim and I talked about removing forDisplay from MR. It should be possible > once we cleaned up the receiver action logic. Acknowledged. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:290: sourceUrn, routes.map(routeToMojo_), joinableRouteIds); On 2015/12/29 00:24:42, imcheng1 wrote: > You can make this backwards compatible if you check if joinableRouteIds is > undefined and if so use an empty array. See comment below. Done. https://codereview.chromium.org/1415103006/diff/390001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:620: * @param {!string} sourceUrn On 2015/12/29 00:24:42, imcheng1 wrote: > On 2015/12/18 23:37:15, matt.boetger wrote: > > On 2015/12/16 07:17:35, mark a. foltz wrote: > > > Adding a new required parameter is specifically not compatible with older > > > versions of the component extension. So you'll want to ensure there is an > > > extension release before this hits dev channel. > > > > The changes to onRouteUpdated also means that the extension will need to be up > > to date. Not sure if I can make the extension work with both - so there will > be > > bit of time where things will be broken with the component extension until the > > patch hits the dev channel. > > It should be able to make this change backwards compatible. > > Old MR + New MRPM: > sourceUrn not provided to MRPM; thus MRPM need to treat undefined as empty > joinableRouteIds (which should be empty) will get dropped when it reaches > media_router_bindings.js > > New MR + Old MRPM: > sourceUrn will get dropped in MRPM > joinableRouteIds not provided to media_router_bindings.js; this file should > check if joinableRouteIds is undefined and send an empty array back to browser > if that's the case. In order to make this backwards compatible, I needed to change the onRoutesUpdated method signature so that it continues to take the route list as the first parameter. https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:237: routes_observer_.reset(new UIMediaRoutesObserver( On 2015/12/29 00:24:42, imcheng1 wrote: > This should go somewhere else now that we set up routes_observer_ in > OnDefaultPresentation{Changed,Removed}. The only case where > OnDefaultPresentationChanged() isn't invoked during Init*() is if the condition > in L209 is false -- so I would move this line to an else block there. Done. https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:418: if (!media_router_ui_->ConnectRemoteRoute(sink_id, route_id)) { On 2015/12/29 00:24:42, imcheng1 wrote: > on naming: Is there logic in the WebUI that restricts joining to remote routes? > i.e., can I start a route on tab 1, then join via the dialog in tab 2 No, there is currently nothing preventing a provider from telling us a local route is joinable and then us showing it. I was defaulting to allow the providers to manage this. I can put a check in media_router_bindings to make sure they only non-local (remote) routes are returned in the joinable_route_ids array.
lgtm lgtm after comment is addressed. Also, does this need to wait for the MRPM fix to remove sink list from onRoutesUpdated to be pushed first? https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:418: if (!media_router_ui_->ConnectRemoteRoute(sink_id, route_id)) { On 2016/01/05 00:19:48, matt.boetger wrote: > On 2015/12/29 00:24:42, imcheng1 wrote: > > on naming: Is there logic in the WebUI that restricts joining to remote > routes? > > i.e., can I start a route on tab 1, then join via the dialog in tab 2 > > No, there is currently nothing preventing a provider from telling us a local > route is joinable and then us showing it. I was defaulting to allow the > providers to manage this. I can put a check in media_router_bindings to make > sure they only non-local (remote) routes are returned in the joinable_route_ids > array. Well I think it's reasonable to allow join to local route. In that case we don't need to call the function ConnectRemoteRoute. https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:283: * @param {string=} opt_sourceUrn The sourUrn associated with this route nit: fix comments. Also comment that it can be empty.
LGTM with nit picks around comments addressed. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:90: // |route_id|. This is used for non-local routes since no Presentation ID Maybe: "|route_id| must refer to a non-local route, since..." https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:147: // |media_source|. Extra newline. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:151: // |error_text| will be null. Extra whitespace after // https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:153: // will be set. Ditto https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:156: string presentation_id, Document use of |presentation_id| https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:184: // headless observer (e.g. a remote control) that has no interest in maybe "this should be considered an observer that is not associated with a media source, and thus cannot connect to a remote route showing a source." https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:197: // if it's emtpy. Thus, |StartObservingMediaRoutes(media_source)| must be typo in empty https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:198: // matched with |StopObservingMediaRoutes(media_soruce)|. typo in source https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:199: // Calling |StopObservingMediaRoutes()| without a media_source will stop The style I like is to use pipes around parameters but not function names, so just |media_source| here and elsewhere. The style guide doesn't seem to be specific though. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:350: RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoConnectRouteByRouteId, You'll want to call SetWakeReason() so we update the UMA counter for event page wakeups.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
Please update the CL description to have a max line length of 72 characters.
Just reviewed c/b/ui https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_media_router.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_media_router.cc:90: const MediaRouteIds& joinable_route_ids) { unused_joinable_route_ids https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:163: const RoutesUpdatedCallback& callback); We avoid multiple constructors in Chrome. Either update existing calls to UIMediaRoutesObserver() or provide a setter for |source_id|. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:214: // non-local route. This is used for connecting to a non-local route. s/ / / https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:217: // completed. 'completes' or 'is completed' https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:416: << ", route id: " << route_id; Reomve?
lgtm https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:240: joinable_route_ids.To<std::vector<std::string>>(); nit: indent 4 spaces rather than 2 https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:28: container.addEventListener('join-route-click', onJoinRouteClick); nit: alphabetize after 'issue-action-click' event listener. https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:285: * @param {Array<string>=} opt_joinableRoutes The active set of joinable media op_joinableRouteIds?
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ==========
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ==========
Aside from fixing the comment nits, I also added the SetWakeReason with a new UMA analytic for ConnectRouteByRouteId. https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/490001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:418: if (!media_router_ui_->ConnectRemoteRoute(sink_id, route_id)) { On 2016/01/05 02:12:47, imcheng1 wrote: > On 2016/01/05 00:19:48, matt.boetger wrote: > > On 2015/12/29 00:24:42, imcheng1 wrote: > > > on naming: Is there logic in the WebUI that restricts joining to remote > > routes? > > > i.e., can I start a route on tab 1, then join via the dialog in tab 2 > > > > No, there is currently nothing preventing a provider from telling us a local > > route is joinable and then us showing it. I was defaulting to allow the > > providers to manage this. I can put a check in media_router_bindings to make > > sure they only non-local (remote) routes are returned in the > joinable_route_ids > > array. > > Well I think it's reasonable to allow join to local route. In that case we don't > need to call the function ConnectRemoteRoute. Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:90: // |route_id|. This is used for non-local routes since no Presentation ID On 2016/01/05 18:39:40, mark a. foltz wrote: > Maybe: "|route_id| must refer to a non-local route, since..." Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router.mojom (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:147: // |media_source|. On 2016/01/05 18:39:40, mark a. foltz wrote: > Extra newline. Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:151: // |error_text| will be null. On 2016/01/05 18:39:40, mark a. foltz wrote: > Extra whitespace after // Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:153: // will be set. On 2016/01/05 18:39:40, mark a. foltz wrote: > Ditto Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:156: string presentation_id, On 2016/01/05 18:39:40, mark a. foltz wrote: > Document use of |presentation_id| Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:184: // headless observer (e.g. a remote control) that has no interest in On 2016/01/05 18:39:40, mark a. foltz wrote: > maybe "this should be considered an observer that is not associated with a media > source, and thus cannot connect to a remote route showing a source." Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:197: // if it's emtpy. Thus, |StartObservingMediaRoutes(media_source)| must be On 2016/01/05 18:39:40, mark a. foltz wrote: > typo in empty Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:198: // matched with |StopObservingMediaRoutes(media_soruce)|. On 2016/01/05 18:39:40, mark a. foltz wrote: > typo in source Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router.mojom:199: // Calling |StopObservingMediaRoutes()| without a media_source will stop On 2016/01/05 18:39:40, mark a. foltz wrote: > The style I like is to use pipes around parameters but not function names, so > just |media_source| here and elsewhere. The style guide doesn't seem to be > specific though. Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:240: joinable_route_ids.To<std::vector<std::string>>(); On 2016/01/05 21:41:58, apacible wrote: > nit: indent 4 spaces rather than 2 Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.cc:350: RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoConnectRouteByRouteId, On 2016/01/05 18:39:40, mark a. foltz wrote: > You'll want to call SetWakeReason() so we update the UMA counter for event page > wakeups. Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:28: container.addEventListener('join-route-click', onJoinRouteClick); On 2016/01/05 21:41:58, apacible wrote: > nit: alphabetize after 'issue-action-click' event listener. Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_media_router.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_media_router.cc:90: const MediaRouteIds& joinable_route_ids) { On 2016/01/05 20:10:23, stevenjb wrote: > unused_joinable_route_ids Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:163: const RoutesUpdatedCallback& callback); On 2016/01/05 20:10:23, stevenjb wrote: > We avoid multiple constructors in Chrome. Either update existing calls to > UIMediaRoutesObserver() or provide a setter for |source_id|. Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:214: // non-local route. This is used for connecting to a non-local route. On 2016/01/05 20:10:23, stevenjb wrote: > s/ / / Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.h:217: // completed. On 2016/01/05 20:10:23, stevenjb wrote: > 'completes' or 'is completed' > Done. https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1415103006/diff/510001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:416: << ", route id: " << route_id; On 2016/01/05 20:10:23, stevenjb wrote: > Reomve? Done. https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:283: * @param {string=} opt_sourceUrn The sourUrn associated with this route On 2016/01/05 02:12:47, imcheng1 wrote: > nit: fix comments. Also comment that it can be empty. Done. https://codereview.chromium.org/1415103006/diff/510001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:285: * @param {Array<string>=} opt_joinableRoutes The active set of joinable media On 2016/01/05 21:41:58, apacible wrote: > op_joinableRouteIds? Done.
FWIW, I think this has become complex enough that it should be broken up (e.g. make the WebUI changes separate). I would be happy to re-review the separated WebUI CL. That said, c/b/ui lgtm
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm
The CQ bit was checked by boetger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mfoltz@chromium.org, apacible@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1415103006/#ps550001 (title: "Rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415103006/550001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415103006/550001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by boetger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, mfoltz@chromium.org, apacible@chromium.org, asvitkine@chromium.org, avayvod@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1415103006/#ps570001 (title: "Rebase before commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415103006/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415103006/570001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boetger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, mfoltz@chromium.org, apacible@chromium.org, asvitkine@chromium.org, avayvod@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1415103006/#ps590001 (title: "Fixing ChromeOS System Tray Tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415103006/590001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415103006/590001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boetger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415103006/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415103006/610001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #21 (id:610001) has been deleted
Patchset #20 (id:590001) has been deleted
The CQ bit was checked by boetger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415103006/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415103006/620001
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 boetger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, mfoltz@chromium.org, apacible@chromium.org, asvitkine@chromium.org, avayvod@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1415103006/#ps620001 (title: "Fixing ChromeOS System Tray Test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415103006/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415103006/620001
Message was sent while issue was closed.
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ==========
Message was sent while issue was closed.
Committed patchset #20 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef ========== to ========== Non-Local Join for Media Router and Presentation API The Cast SDK has a concept of non-local joins. This means that if a Cast session is started on Device A (e.g. an Android phone), a user should be able to join the session via a webpage in Chrome on Device B. This requires the idea of "joinable" routes to enter into the media router. The route queries need a context for this to make sense, and that context is the page's |media_source|. If the route can be joined by the page (given the page's media_source), then a list of route_ids (that are a subset of the routes returned in OnRouteUpdated) signify what routes are joinable. The UI should present a "Join" button if the currently viewed route is joinable. Current Screenshots for MediaRoute Dialog Stop And Join - LTR: https://screenshot.googleplex.com/X814uW7esBU Stop Only - LTR: https://screenshot.googleplex.com/iybKZd1aABh Stop And Join - RTL: https://screenshot.googleplex.com/ffKqawTFgTF Stop Only - RTL: https://screenshot.googleplex.com/HWbR94H5Qef Committed: https://crrev.com/5628601c63e07c7aa04d3e62eaeb90b2d60ea53a Cr-Commit-Position: refs/heads/master@{#368260} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/5628601c63e07c7aa04d3e62eaeb90b2d60ea53a Cr-Commit-Position: refs/heads/master@{#368260} |
