Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2002293003:
[Media Router] Allow casting new media to sink with existing route. (Closed)
Description[Media Router] Allow casting new media to sink with existing route.
This change allows for one button push to stop casting to a sink and
then immediately create a new route to the same sink with the selected
source. The makes the user experience smoother by not making them stop
the current cast and then start a new one manually in two stops.
Some shortcomings of this change that will be addressed in the future:
- At least for mirroring, it's possible to avoid stopping the route and
just switch the stream sources. This probably requires adding a new
API to the extension.
- The button will currently allow users to re-cast the current source,
stopping the current route and starting a new one, even though this
isn't necessary. When both the old and new sources are tabs, the tab
IDs could be checked, but other cases would have to be handled in MR
or the extension.
BUG=614144
R=apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/4680358215ff217eb6b0073c4be2243738369957
Cr-Commit-Position: refs/heads/master@{#397457}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Disable cast-to-route when sink has no available cast modes #Patch Set 3 : Alphabetize renamed method #
Total comments: 11
Patch Set 4 : Responding to apacible's comments #
Total comments: 9
Patch Set 5 : Combine close-route events, new first-action enum, changed comment #
Total comments: 2
Patch Set 6 : Add missing C++ metric value #Patch Set 7 : Rebase for imcheng@'s off-the-record change #
Total comments: 12
Patch Set 8 : Responding to mfoltz's comments #Patch Set 9 : Enable route details cast button only when no currently launching sink #Patch Set 10 : Add route creation TODO and factor out logical NOT #Messages
Total messages: 30 (7 generated)
Description was changed from ========== [Media Router] Allow casting new media to sink with existing route. This change allows for one button push to stop casting to a sink and then immediately create a new route to the same sink with the selected source. The makes the user experience smoother by not making them stop the current cast and then start a new one manually in two stops. Some shortcomings of this change that will be addressed in the future: - At least for mirroring, it's possible to avoid stopping the route and just switch the stream sources. This probably requires adding a new API to the extension. - The button will currently allow users to re-cast the current source, stopping the current route and starting a new one, even though this isn't necessary. When both the old and new sources are tabs, the tab IDs could be checked, but other cases would have to be handled in MR or the extension. BUG=614144 R=apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org ========== to ========== [Media Router] Allow casting new media to sink with existing route. This change allows for one button push to stop casting to a sink and then immediately create a new route to the same sink with the selected source. The makes the user experience smoother by not making them stop the current cast and then start a new one manually in two stops. Some shortcomings of this change that will be addressed in the future: - At least for mirroring, it's possible to avoid stopping the route and just switch the stream sources. This probably requires adding a new API to the extension. - The button will currently allow users to re-cast the current source, stopping the current route and starting a new one, even though this isn't necessary. When both the old and new sources are tabs, the tab IDs could be checked, but other cases would have to be handled in MR or the extension. BUG=614144 R=apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
This is a quick, UI-only solution to casting to sinks that already have routes. I wanted to gauge whether this is worthwhile to improve the user experience now before working on the more complete stream-swapping solution, which may involve adding a new API to the extension. PTAL, thanks!
https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/route_details/route_details.html (left): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/route_details/route_details.html:17: hidden$="[[!route.canJoin]]" It would be nice to update this logic to make it hidden if the current sourceUrn (of the page which the MR UI has been opened on) matches the sourceUrn of the existing route. That would require passing around more info though. What sets the route.canJoin property? Maybe whatever does that can update the logic to check for the new scenarios. https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/media_router.js:41: onCloseRouteForNewMedia); Can't this reuse the onCloseRouteClick handler as well? The code looks identical. Or would that not conform to some style guide?
https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/route_details/route_details.html (left): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/route_details/route_details.html:17: hidden$="[[!route.canJoin]]" On 2016/05/23 21:42:35, amp wrote: > It would be nice to update this logic to make it hidden if the current sourceUrn > (of the page which the MR UI has been opened on) matches the sourceUrn of the > existing route. > > That would require passing around more info though. What sets the route.canJoin > property? Maybe whatever does that can update the logic to check for the new > scenarios. The UI sets route.canJoin by matching a list of joinable route IDs from the extension to the list of routes. The route ID is indeed in a normal URN format that contains the source URN. However, the URN prefix and format is currently only known to the component extension. Without an extension change, we'd have to add route URN parsing somewhere either in MR to pass the information up to the UI or in the UI itself. Since this should be a relatively temporary change, I don't feel strongly either way. WDYT? https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/media_router.js:41: onCloseRouteForNewMedia); On 2016/05/23 21:42:35, amp wrote: > Can't this reuse the onCloseRouteClick handler as well? The code looks > identical. > > Or would that not conform to some style guide? It is identical, but I wasn't sure if it would be bad style to use onCloseRouteClick. If others are ok with reusing it, I'm fine with it too.
This is cool but I find the design a bit hard to follow: I don't know the event flow in the UX very well and it looks like swapping sources is implemented by firing Polymer events, instead of calling stop() and start() functions. I think you might need to check whether there is a current cast mode for the sink before offering to swap sources. https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/route_details/route_details.html (left): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/route_details/route_details.html:17: hidden$="[[!route.canJoin]]" Does this remove the entire logic about when to show the "CAST" button for individual routes? Fascinating. Assuming that there's no other use for route.canJoin, we could remove all the plumbing in the MR and MR UX that determines which routes are joinable from which tabs. Always showing the "CAST" button means there must be a default cast mode for every page and every sink, otherwise clicking it would be undefined. I think the only time this might be a problem is with a DIAL device that only supports e.g. YouTube and not mirroring.
There was indeed a problem with handling DIAL TVs and having undefined cast modes. PTAL, thanks! https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/route_details/route_details.html (left): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/route_details/route_details.html:17: hidden$="[[!route.canJoin]]" On 2016/05/23 23:53:38, mark a. foltz wrote: > Does this remove the entire logic about when to show the "CAST" button for > individual routes? Fascinating. Assuming that there's no other use for > route.canJoin, we could remove all the plumbing in the MR and MR UX that > determines which routes are joinable from which tabs. > > Always showing the "CAST" button means there must be a default cast mode for > every page and every sink, otherwise clicking it would be undefined. I think > the only time this might be a problem is with a DIAL device that only supports > e.g. YouTube and not mirroring. route.canJoin would still be necessary, but in any case I was too quick to remove the button hidden state. It turns out we actually don't even get the DIAL sinks in the allSinks, even when we get the route. So casting YouTube to a DIAL TV and then opening the MR dialog in a new tab will show the route but the sink isn't actually there to even check the cast modes. I added logic to hide the button again when there's no compatible cast modes for the route's sink, which includes if the user selects a cast mode manually first. I also snuck in a small fix for the header text that relates to the same DIAL condition.
https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2002293003/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/media_router.js:41: onCloseRouteForNewMedia); On 2016/05/23 22:41:10, btolsch wrote: > On 2016/05/23 21:42:35, amp wrote: > > Can't this reuse the onCloseRouteClick handler as well? The code looks > > identical. > > > > Or would that not conform to some style guide? > > It is identical, but I wasn't sure if it would be bad style to use > onCloseRouteClick. If others are ok with reusing it, I'm fine with it too. Reusing onCloseRouteClick is fine. https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:367: sinkIdAwaitingNewRoute_: { Could we generalize |currentLaunchingSinkId_| for this? https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1612: onCastNewMediaClick_: function(event) { We should keep track if this is the first user action. There are some examples in this file, e.g. in onCloseRouteClick_(). https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1615: this.sinkIdAwaitingNewRoute_ = detail.route.sinkId; UX question: Do we expect the dialog to close after 3s of the new route resolving, similar to if we're casting on an otherwise inactive sink? https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.js:91: this.fire('start-casting-to-route-click', {route: this.route}); Rename these two events to be more distinct between joining and the more general, new media.
https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:367: sinkIdAwaitingNewRoute_: { On 2016/05/25 00:56:34, apacible wrote: > Could we generalize |currentLaunchingSinkId_| for this? Done. I think we still need a boolean indicating which use it currently has to avoid extra create-route events from rebuildRouteMaps_(), but the sink spinner would work automatically while we're waiting for the route to connect. https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1612: onCastNewMediaClick_: function(event) { On 2016/05/25 00:56:34, apacible wrote: > We should keep track if this is the first user action. There are some examples > in this file, e.g. in onCloseRouteClick_(). Done. I reused START_LOCAL for this, but let me know if you think it should be a new event, possibly JOIN_ROUTE or CAST_TO_ROUTE. https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1615: this.sinkIdAwaitingNewRoute_ = detail.route.sinkId; On 2016/05/25 00:56:34, apacible wrote: > UX question: Do we expect the dialog to close after 3s of the new route > resolving, similar to if we're casting on an otherwise inactive sink? That's what I would expect. Do you think it should be kept open unconditionally? https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.js:91: this.fire('start-casting-to-route-click', {route: this.route}); On 2016/05/25 00:56:34, apacible wrote: > Rename these two events to be more distinct between joining and the more > general, new media. Done.
https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1612: onCastNewMediaClick_: function(event) { On 2016/05/25 20:53:44, btolsch wrote: > On 2016/05/25 00:56:34, apacible wrote: > > We should keep track if this is the first user action. There are some examples > > in this file, e.g. in onCloseRouteClick_(). > > Done. I reused START_LOCAL for this, but let me know if you think it should be a > new event, possibly JOIN_ROUTE or CAST_TO_ROUTE. I'd prefer a new event, since it may be useful to differentiate between the two scenarios. https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1615: this.sinkIdAwaitingNewRoute_ = detail.route.sinkId; On 2016/05/25 20:53:44, btolsch wrote: > On 2016/05/25 00:56:34, apacible wrote: > > UX question: Do we expect the dialog to close after 3s of the new route > > resolving, similar to if we're casting on an otherwise inactive sink? > > That's what I would expect. Do you think it should be kept open > unconditionally? No, that sg. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:743: return this.currentRoute_ && this.sinkMap_[this.currentRoute_.sinkId] ? Is this relevant to your main changes? I was looking at this earlier: If there is a |currentRoute_|, the dialog closes, you navigate to another tab where the cast modes are a mismatch, then open the dialog again, you'll see the route details but the default "Cast to" header. It may be better to check allSinks for the existing sink rather than just sinkMap. If the sink just doesn't exist anymore, we shouldn't show the route. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1618: this.fire('close-route-for-new-media', detail); Fire 'close-route-click' (or 'close-route' and rename it in the current usage), since the event handler does the same thing. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.js:84: * it fires a cast-new-media-click event. This is called when the button to nit: "... a cast-new-media-click event, which stops the current route and immediately launches a new route to the same sink..." (something like that) https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.js:93: this.fire('cast-new-media-click', {route: this.route}); What happens if there's already a pending created route in the media-router-container?
btolsch@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms.xml https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1612: onCastNewMediaClick_: function(event) { On 2016/05/25 22:37:01, apacible wrote: > On 2016/05/25 20:53:44, btolsch wrote: > > On 2016/05/25 00:56:34, apacible wrote: > > > We should keep track if this is the first user action. There are some > examples > > > in this file, e.g. in onCloseRouteClick_(). > > > > Done. I reused START_LOCAL for this, but let me know if you think it should be > a > > new event, possibly JOIN_ROUTE or CAST_TO_ROUTE. > > I'd prefer a new event, since it may be useful to differentiate between the two > scenarios. Done. Used CAST_TO_ROUTE. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:743: return this.currentRoute_ && this.sinkMap_[this.currentRoute_.sinkId] ? On 2016/05/25 22:37:01, apacible wrote: > Is this relevant to your main changes? I was looking at this earlier: > If there is a |currentRoute_|, the dialog closes, you navigate to another tab > where the cast modes are a mismatch, then open the dialog again, you'll see the > route details but the default "Cast to" header. > > It may be better to check allSinks for the existing sink rather than just > sinkMap. If the sink just doesn't exist anymore, we shouldn't show the route. This change doesn't have to be made here, but I thought it was semi-relevant. It addresses the scenario you described. I'm not sure what you mean by "if the sink just doesn't exist anymore." A sink can be missing from allSinks just because it doesn't support the available cast modes, even though MR might still be aware of it. Are you suggesting that maybeShowRouteDetailsOnOpen() shouldn't show route details when the sink isn't present in allSinks? Lastly, what does checking allSinks vs sinkMap accomplish? I thought they were kept synchronized, except for pseudo sinks. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1618: this.fire('close-route-for-new-media', detail); On 2016/05/25 22:37:01, apacible wrote: > Fire 'close-route-click' (or 'close-route' and rename it in the current usage), > since the event handler does the same thing. Done. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.js:84: * it fires a cast-new-media-click event. This is called when the button to On 2016/05/25 22:37:01, apacible wrote: > nit: "... a cast-new-media-click event, which stops the current route and > immediately launches a new route to the same sink..." (something like that) Done. https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.js:93: this.fire('cast-new-media-click', {route: this.route}); On 2016/05/25 22:37:01, apacible wrote: > What happens if there's already a pending created route in the > media-router-container? Currently, only the sink for this route would show a spinner and the previously pending route wouldn't be displayed in route details when it's returned. If this is undesirable, the container could either do nothing when we get the event or maybe set availableCastModes to 0 to prevent the cast button from being shown in route details in the first place. One problem with the latter option is that the cast button would still appear and be usable if the route is joinable. We could also add or change the parameters of route details to let the container completely disable the cast button in this case.
histograms.xml lgtm
https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:743: return this.currentRoute_ && this.sinkMap_[this.currentRoute_.sinkId] ? On 2016/05/26 00:31:12, btolsch wrote: > On 2016/05/25 22:37:01, apacible wrote: > > Is this relevant to your main changes? I was looking at this earlier: > > If there is a |currentRoute_|, the dialog closes, you navigate to another tab > > where the cast modes are a mismatch, then open the dialog again, you'll see > the > > route details but the default "Cast to" header. > > > > It may be better to check allSinks for the existing sink rather than just > > sinkMap. If the sink just doesn't exist anymore, we shouldn't show the route. > > This change doesn't have to be made here, but I thought it was semi-relevant. > It addresses the scenario you described. > > I'm not sure what you mean by "if the sink just doesn't exist anymore." A sink > can be missing from allSinks just because it doesn't support the available cast > modes, even though MR might still be aware of it. Are you suggesting that > maybeShowRouteDetailsOnOpen() shouldn't show route details when the sink isn't > present in allSinks? > > Lastly, what does checking allSinks vs sinkMap accomplish? I thought they were > kept synchronized, except for pseudo sinks. Ah, I thought the WebUI filtered out sinks by cast modes rather than the MR side. https://codereview.chromium.org/2002293003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/2002293003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:53: CAST_TO_ROUTE: 5, Add this to the C++ side too.
https://codereview.chromium.org/2002293003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/2002293003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:53: CAST_TO_ROUTE: 5, On 2016/05/26 17:20:46, apacible wrote: > Add this to the C++ side too. Done.
lgtm
Friendly ping. Any other issues with getting this in?
lgtm
Mostly some naming suggestions, nothing major. I'm a little concerned that the data model driving the UX seems to be modal, i.e. at first glance it won't handle simultaneous requests to create, replace, or join routes. This might be okay given the current assumptions but seems brittle. Long term it might be better to refactor this to keep a per-sink status (idle|create|join|replace) instead of global variables with the "launching mode" of the dialog. Let me know what you think. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_router_metrics.h (right): https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_router_metrics.h:49: CAST_TO_ROUTE = 5, It's not obvious (to me) what user gesture is implied here since there are other ways to cast related to an existing route (for example, by connecting to a remote route). Maybe this could be REPLACE_LOCAL_ROUTE ? https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:194: * Whether the currently launching sink, whose ID is given by The term "launching sink" sounds a bit funny to me (I imagine a kitchen sink flying through the air), but I see it's used elsewhere in this module. I would have maybe gone with "Whether the sink with a pending route creation, ..." https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1608: this.currentLaunchingSinkId_ = detail.route.sinkId; Is it possible to get this event when there is already a pending route creation request (i.e., currentLaunchingSinkId_ != null)? https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1609: this.launchingSinkAwaitingRouteClose_ = true; What if this was already true? https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:71: return !(route && route.canJoin) && !availableCastModes; Can you use || here instead? https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:86: this.fire('cast-new-media-click', {route: this.route}); Slight preference for replace-route-click, but I don't feel strongly. Jennifer should probably have the final say :)
I agree with your long term refactor suggestion, but for now I made the route details cast button only visible when no other sink is launching. There are other options here but I feel this is the cleanest UX for this change. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_router_metrics.h (right): https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_router_metrics.h:49: CAST_TO_ROUTE = 5, On 2016/05/31 22:30:06, mark a. foltz wrote: > It's not obvious (to me) what user gesture is implied here since there are other > ways to cast related to an existing route (for example, by connecting to a > remote route). > > Maybe this could be REPLACE_LOCAL_ROUTE ? Done. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:194: * Whether the currently launching sink, whose ID is given by On 2016/05/31 22:30:06, mark a. foltz wrote: > The term "launching sink" sounds a bit funny to me (I imagine a kitchen sink > flying through the air), but I see it's used elsewhere in this module. > > I would have maybe gone with "Whether the sink with a pending route creation, > ..." Done. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1608: this.currentLaunchingSinkId_ = detail.route.sinkId; On 2016/05/31 22:30:06, mark a. foltz wrote: > Is it possible to get this event when there is already a pending route creation > request (i.e., currentLaunchingSinkId_ != null)? Jennifer asked the same question earlier (https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource...) but the answer is that this sink (with its route being replaced) would show a spinner in the sink list and the previously pending route won't be displayed when it is returned (meaning its details won't be displayed). I think there might still be a possible issue if both currentLaunchingSinkId_ and pendingCreateRouteId_ are set, because this would put them out of sync. The bigger problem would still be the next line. I agree with your long-term suggestion, but for now I can disable the cast button in the route details view if there is a currently-launching sink. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1609: this.launchingSinkAwaitingRouteClose_ = true; On 2016/05/31 22:30:06, mark a. foltz wrote: > What if this was already true? That would cause the first sink (the one originally waiting for its route-close to complete) to never create its new route. The fix I added for now is to disable the cast button in the route details view if there is a currently-launching sink, which would include a previous 'replace route' call. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:71: return !(route && route.canJoin) && !availableCastModes; On 2016/05/31 22:30:06, mark a. foltz wrote: > Can you use || here instead? Maybe I'm misunderstanding the suggestion but saying (!canJoin || !availableCastModes) would basically just preserve the current behavior of only showing the button for joinable routes. https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:86: this.fire('cast-new-media-click', {route: this.route}); On 2016/05/31 22:30:06, mark a. foltz wrote: > Slight preference for replace-route-click, but I don't feel strongly. Jennifer > should probably have the final say :) Done.
LGTM On 2016/06/01 at 01:58:16, btolsch wrote: > I agree with your long term refactor suggestion, but for now I made the route details cast button only visible when no other sink is launching. There are other options here but I feel this is the cleanest UX for this change. Okay, adding a TODO(crbug.com/XXX) with a summary of the thread would record this conversation :) > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/media/r... > File chrome/browser/media/router/media_router_metrics.h (right): > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/media/r... > chrome/browser/media/router/media_router_metrics.h:49: CAST_TO_ROUTE = 5, > On 2016/05/31 22:30:06, mark a. foltz wrote: > > It's not obvious (to me) what user gesture is implied here since there are other > > ways to cast related to an existing route (for example, by connecting to a > > remote route). > > > > Maybe this could be REPLACE_LOCAL_ROUTE ? > > Done. > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:194: * Whether the currently launching sink, whose ID is given by > On 2016/05/31 22:30:06, mark a. foltz wrote: > > The term "launching sink" sounds a bit funny to me (I imagine a kitchen sink > > flying through the air), but I see it's used elsewhere in this module. > > > > I would have maybe gone with "Whether the sink with a pending route creation, > > ..." > > Done. > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1608: this.currentLaunchingSinkId_ = detail.route.sinkId; > On 2016/05/31 22:30:06, mark a. foltz wrote: > > Is it possible to get this event when there is already a pending route creation > > request (i.e., currentLaunchingSinkId_ != null)? > > Jennifer asked the same question earlier (https://codereview.chromium.org/2002293003/diff/60001/chrome/browser/resource...) but the answer is that this sink (with its route being replaced) would show a spinner in the sink list and the previously pending route won't be displayed when it is returned (meaning its details won't be displayed). I think there might still be a possible issue if both currentLaunchingSinkId_ and pendingCreateRouteId_ are set, because this would put them out of sync. The bigger problem would still be the next line. > > I agree with your long-term suggestion, but for now I can disable the cast button in the route details view if there is a currently-launching sink. > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1609: this.launchingSinkAwaitingRouteClose_ = true; > On 2016/05/31 22:30:06, mark a. foltz wrote: > > What if this was already true? > > That would cause the first sink (the one originally waiting for its route-close to complete) to never create its new route. The fix I added for now is to disable the cast button in the route details view if there is a currently-launching sink, which would include a previous 'replace route' call. > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > chrome/browser/resources/media_router/elements/route_details/route_details.js:71: return !(route && route.canJoin) && !availableCastModes; > On 2016/05/31 22:30:06, mark a. foltz wrote: > > Can you use || here instead? > > Maybe I'm misunderstanding the suggestion but saying (!canJoin || !availableCastModes) would basically just preserve the current behavior of only showing the button for joinable routes. I was suggesting something along the lines of !((route && route.canJoin) || (availableCastModes && replaceRouteAvailable)), as positive cases are easier to reason about (hide the button unless either of those two conditions are true). I don't feel strongly though. > > https://codereview.chromium.org/2002293003/diff/120001/chrome/browser/resourc... > chrome/browser/resources/media_router/elements/route_details/route_details.js:86: this.fire('cast-new-media-click', {route: this.route}); > On 2016/05/31 22:30:06, mark a. foltz wrote: > > Slight preference for replace-route-click, but I don't feel strongly. Jennifer > > should probably have the final say :) > > Done.
The CQ bit was checked by btolsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2002293003/#ps180001 (title: "Add route creation TODO and factor out logical NOT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002293003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002293003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_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 btolsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002293003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002293003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Allow casting new media to sink with existing route. This change allows for one button push to stop casting to a sink and then immediately create a new route to the same sink with the selected source. The makes the user experience smoother by not making them stop the current cast and then start a new one manually in two stops. Some shortcomings of this change that will be addressed in the future: - At least for mirroring, it's possible to avoid stopping the route and just switch the stream sources. This probably requires adding a new API to the extension. - The button will currently allow users to re-cast the current source, stopping the current route and starting a new one, even though this isn't necessary. When both the old and new sources are tabs, the tab IDs could be checked, but other cases would have to be handled in MR or the extension. BUG=614144 R=apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Allow casting new media to sink with existing route. This change allows for one button push to stop casting to a sink and then immediately create a new route to the same sink with the selected source. The makes the user experience smoother by not making them stop the current cast and then start a new one manually in two stops. Some shortcomings of this change that will be addressed in the future: - At least for mirroring, it's possible to avoid stopping the route and just switch the stream sources. This probably requires adding a new API to the extension. - The button will currently allow users to re-cast the current source, stopping the current route and starting a new one, even though this isn't necessary. When both the old and new sources are tabs, the tab IDs could be checked, but other cases would have to be handled in MR or the extension. BUG=614144 R=apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/4680358215ff217eb6b0073c4be2243738369957 Cr-Commit-Position: refs/heads/master@{#397457} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/4680358215ff217eb6b0073c4be2243738369957 Cr-Commit-Position: refs/heads/master@{#397457} |
