chrome/browser/resources/media_router/elements/route_details/route_details.html - Issue 2725503002: [Media Router] Custom Controls 4 - Implement details view WebUI

Side by Side Diff: chrome/browser/resources/media_router/elements/route_details/route_details.html

Issue 2725503002: [Media Router] Custom Controls 4 - Implement details view WebUI (Closed)

Patch Set: . Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js ('K') | « chrome/browser/resources/media_router/elements/route_details/route_details.css ('k') | chrome/browser/resources/media_router/elements/route_details/route_details.js » ('j') | chrome/browser/resources/media_router/media_router.js » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <link rel="import" href="chrome://resources/html/polymer.html">	1 <link rel="import" href="chrome://resources/html/polymer.html">

2 <link rel="import" href="chrome://resources/html/i18n_behavior.html">	2 <link rel="import" href="chrome://resources/html/i18n_behavior.html">

	3 <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/av-icons.htm l">

3 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-butt on.html">	4 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-butt on.html">

	5 <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper -icon-button.html">

	6 <link rel="import" href="chrome://resources/polymer/v1_0/paper-slider/paper-slid er.html">

4 <dom-module id="route-details">	7 <dom-module id="route-details">

5 <link rel="import" type="css" href="../../media_router_common.css">	8 <link rel="import" type="css" href="../../media_router_common.css">

6 <link rel="import" type="css" href="route_details.css">	9 <link rel="import" type="css" href="route_details.css">

7 <template>	10 <template>

8 <div id="route-information" hidden$="[[!isCustomControllerHidden_]]">	11 <div id="route-controls">
	imcheng 2017/03/02 02:25:47 - Is this used for both mirroring and Cast routes? - Is this used for both mirroring and Cast routes? How much common logic is shared by the two types, and would the code benefit from splitting this into a distinct template for each one? - For media routes for which controls / media status is not supported, how does the route details look like? - Would it make sense, from maintainability's POV, to split the controller into its own set of resources? takumif 2017/03/02 19:21:27 Yes, I was thinking of using one template for all Show quoted text On 2017/03/02 02:25:47, imcheng wrote: > - Is this used for both mirroring and Cast routes? How much common logic is > shared by the two types, and would the code benefit from splitting this into a > distinct template for each one? Yes, I was thinking of using one template for all of Cast/mirroring/Hangouts, and show/hide elements depending on can_seek/can_mute/etc attributes of the media status. This means that the WebUI won't need to be aware of the route type. Without Hangout's high bandwidth mode, mirroring and Hangouts templates are just subsets of the Cast template, so I think having one template makes sense. Even when we're adding the high-bandwidth-mode checkbox, I think we can add it to this template and hide it for Cast/mirroring. It might make sense to make Cast have its own template once we're supporting customizations though. I've added a table to the design doc showing the controls each route type supports: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... Show quoted text > - For media routes for which controls / media status is not supported, how does > the route details look like? When would media status not be supported? I think we can cover those cases by initializing this template with title/status taken from the Route object, and all the controls hidden by setting can_* attributes false. Show quoted text > - Would it make sense, from maintainability's POV, to split the controller into > its own set of resources? Yes, it might be better if we put them in their own js/css files. I'll do that. imcheng 2017/03/06 23:36:35 Ok, sounds good. Thanks for figuring this out and Show quoted text On 2017/03/02 19:21:27, takumif wrote: > On 2017/03/02 02:25:47, imcheng wrote: > > - Is this used for both mirroring and Cast routes? How much common logic is > > shared by the two types, and would the code benefit from splitting this into a > > distinct template for each one? > > Yes, I was thinking of using one template for all of Cast/mirroring/Hangouts, > and show/hide elements depending on can_seek/can_mute/etc attributes of the > media status. This means that the WebUI won't need to be aware of the route > type. Without Hangout's high bandwidth mode, mirroring and Hangouts templates > are just subsets of the Cast template, so I think having one template makes > sense. > > Even when we're adding the high-bandwidth-mode checkbox, I think we can add it > to this template and hide it for Cast/mirroring. It might make sense to make > Cast have its own template once we're supporting customizations though. > > I've added a table to the design doc showing the controls each route type > supports: > https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... > Ok, sounds good. Thanks for figuring this out and updating the doc! Show quoted text > > - For media routes for which controls / media status is not supported, how > does > > the route details look like? > > When would media status not be supported? I think we can cover those cases by > initializing this template with title/status taken from the Route object, and > all the controls hidden by setting can_* attributes false. > DIAL MediaRoutes for example do not have any custom controls or status beyond the route description. IIRC, there's no custom controller path for DIAL, so the custom controller view just gets hidden. Show quoted text > > - Would it make sense, from maintainability's POV, to split the controller > into > > its own set of resources? > > Yes, it might be better if we put them in their own js/css files. I'll do that. takumif 2017/03/08 23:34:58 I thought about splitting the controller into its Show quoted text On 2017/03/02 02:25:47, imcheng wrote: > - Is this used for both mirroring and Cast routes? How much common logic is > shared by the two types, and would the code benefit from splitting this into a > distinct template for each one? > - For media routes for which controls / media status is not supported, how does > the route details look like? > - Would it make sense, from maintainability's POV, to split the controller into > its own set of resources? I thought about splitting the controller into its own files, but all that the controller would do is call browserApi methods, so the route_details would be calling controller methods that call browserApi methods, and I feel that's just another layer of indirection. So I'm thinking of not having a separate class for the controller. takumif 2017/03/08 23:34:58 DIAL currently just shows just the title that says Show quoted text On 2017/03/06 23:36:35, imcheng wrote: > On 2017/03/02 19:21:27, takumif wrote: > > On 2017/03/02 02:25:47, imcheng wrote: > > > - Is this used for both mirroring and Cast routes? How much common logic is > > > shared by the two types, and would the code benefit from splitting this into > a > > > distinct template for each one? > > > > Yes, I was thinking of using one template for all of Cast/mirroring/Hangouts, > > and show/hide elements depending on can_seek/can_mute/etc attributes of the > > media status. This means that the WebUI won't need to be aware of the route > > type. Without Hangout's high bandwidth mode, mirroring and Hangouts templates > > are just subsets of the Cast template, so I think having one template makes > > sense. > > > > Even when we're adding the high-bandwidth-mode checkbox, I think we can add it > > to this template and hide it for Cast/mirroring. It might make sense to make > > Cast have its own template once we're supporting customizations though. > > > > I've added a table to the design doc showing the controls each route type > > supports: > > > https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... > > > > Ok, sounds good. Thanks for figuring this out and updating the doc! > > > > > > - For media routes for which controls / media status is not supported, how > > does > > > the route details look like? > > > > When would media status not be supported? I think we can cover those cases by > > initializing this template with title/status taken from the Route object, and > > all the controls hidden by setting can_* attributes false. > > > > DIAL MediaRoutes for example do not have any custom controls or status beyond > the route description. IIRC, there's no custom controller path for DIAL, so the > custom controller view just gets hidden. DIAL currently just shows just the title that says something like "Casting to YouTube". We can replicate that by initializing the custom controls with an empty media status with just the title set. Show quoted text > > > > > - Would it make sense, from maintainability's POV, to split the controller > > into > > > its own set of resources? > > > > Yes, it might be better if we put them in their own js/css files. I'll do > that. > imcheng 2017/03/11 21:57:38 If you interact with the controller element then w Show quoted text On 2017/03/08 23:34:58, takumif wrote: > On 2017/03/02 02:25:47, imcheng wrote: > > - Is this used for both mirroring and Cast routes? How much common logic is > > shared by the two types, and would the code benefit from splitting this into a > > distinct template for each one? > > - For media routes for which controls / media status is not supported, how > does > > the route details look like? > > - Would it make sense, from maintainability's POV, to split the controller > into > > its own set of resources? > > I thought about splitting the controller into its own files, but all that the > controller would do is call browserApi methods, so the route_details would be > calling controller methods that call browserApi methods, and I feel that's just > another layer of indirection. So I'm thinking of not having a separate class for > the controller. If you interact with the controller element then why would it go through route-details first? imcheng 2017/03/11 21:57:39 Ok. Is the idea that the C++ code will try to crea Show quoted text On 2017/03/08 23:34:58, takumif wrote: > On 2017/03/06 23:36:35, imcheng wrote: > > On 2017/03/02 19:21:27, takumif wrote: > > > On 2017/03/02 02:25:47, imcheng wrote: > > > > - Is this used for both mirroring and Cast routes? How much common logic > is > > > > shared by the two types, and would the code benefit from splitting this > into > > a > > > > distinct template for each one? > > > > > > Yes, I was thinking of using one template for all of > Cast/mirroring/Hangouts, > > > and show/hide elements depending on can_seek/can_mute/etc attributes of the > > > media status. This means that the WebUI won't need to be aware of the route > > > type. Without Hangout's high bandwidth mode, mirroring and Hangouts > templates > > > are just subsets of the Cast template, so I think having one template makes > > > sense. > > > > > > Even when we're adding the high-bandwidth-mode checkbox, I think we can add > it > > > to this template and hide it for Cast/mirroring. It might make sense to make > > > Cast have its own template once we're supporting customizations though. > > > > > > I've added a table to the design doc showing the controls each route type > > > supports: > > > > > > https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... > > > > > > > Ok, sounds good. Thanks for figuring this out and updating the doc! > > > > > > > > > > - For media routes for which controls / media status is not supported, how > > > does > > > > the route details look like? > > > > > > When would media status not be supported? I think we can cover those cases > by > > > initializing this template with title/status taken from the Route object, > and > > > all the controls hidden by setting can_* attributes false. > > > > > > > DIAL MediaRoutes for example do not have any custom controls or status beyond > > the route description. IIRC, there's no custom controller path for DIAL, so > the > > custom controller view just gets hidden. > > DIAL currently just shows just the title that says something like "Casting to > YouTube". We can replicate that by initializing the custom controls with an > empty media status with just the title set. > > > > > > > > > - Would it make sense, from maintainability's POV, to split the controller > > > into > > > > its own set of resources? > > > > > > Yes, it might be better if we put them in their own js/css files. I'll do > > that. > > > Ok. Is the idea that the C++ code will try to create a MediaController for the route, and fallback to some default setup if it fails? takumif 2017/03/16 20:16:02 Factored out the controls into their own Polymer m Show quoted text On 2017/03/11 21:57:38, imcheng wrote: > On 2017/03/08 23:34:58, takumif wrote: > > On 2017/03/02 02:25:47, imcheng wrote: > > > - Is this used for both mirroring and Cast routes? How much common logic is > > > shared by the two types, and would the code benefit from splitting this into > a > > > distinct template for each one? > > > - For media routes for which controls / media status is not supported, how > > does > > > the route details look like? > > > - Would it make sense, from maintainability's POV, to split the controller > > into > > > its own set of resources? > > > > I thought about splitting the controller into its own files, but all that the > > controller would do is call browserApi methods, so the route_details would be > > calling controller methods that call browserApi methods, and I feel that's > just > > another layer of indirection. So I'm thinking of not having a separate class > for > > the controller. > > If you interact with the controller element then why would it go through > route-details first? Factored out the controls into their own Polymer module. takumif 2017/03/16 20:16:02 Yes. The Dial MRP can return false for the CreateM Show quoted text On 2017/03/11 21:57:39, imcheng wrote: > On 2017/03/08 23:34:58, takumif wrote: > > On 2017/03/06 23:36:35, imcheng wrote: > > > On 2017/03/02 19:21:27, takumif wrote: > > > > On 2017/03/02 02:25:47, imcheng wrote: > > > > > - Is this used for both mirroring and Cast routes? How much common logic > > is > > > > > shared by the two types, and would the code benefit from splitting this > > into > > > a > > > > > distinct template for each one? > > > > > > > > Yes, I was thinking of using one template for all of > > Cast/mirroring/Hangouts, > > > > and show/hide elements depending on can_seek/can_mute/etc attributes of > the > > > > media status. This means that the WebUI won't need to be aware of the > route > > > > type. Without Hangout's high bandwidth mode, mirroring and Hangouts > > templates > > > > are just subsets of the Cast template, so I think having one template > makes > > > > sense. > > > > > > > > Even when we're adding the high-bandwidth-mode checkbox, I think we can > add > > it > > > > to this template and hide it for Cast/mirroring. It might make sense to > make > > > > Cast have its own template once we're supporting customizations though. > > > > > > > > I've added a table to the design doc showing the controls each route type > > > > supports: > > > > > > > > > > https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... > > > > > > > > > > Ok, sounds good. Thanks for figuring this out and updating the doc! > > > > > > > > > > > > > > - For media routes for which controls / media status is not supported, > how > > > > does > > > > > the route details look like? > > > > > > > > When would media status not be supported? I think we can cover those cases > > by > > > > initializing this template with title/status taken from the Route object, > > and > > > > all the controls hidden by setting can_* attributes false. > > > > > > > > > > DIAL MediaRoutes for example do not have any custom controls or status > beyond > > > the route description. IIRC, there's no custom controller path for DIAL, so > > the > > > custom controller view just gets hidden. > > > > DIAL currently just shows just the title that says something like "Casting to > > YouTube". We can replicate that by initializing the custom controls with an > > empty media status with just the title set. > > > > > > > > > > > > > - Would it make sense, from maintainability's POV, to split the > controller > > > > into > > > > > its own set of resources? > > > > > > > > Yes, it might be better if we put them in their own js/css files. I'll do > > > that. > > > > > > > Ok. Is the idea that the C++ code will try to create a MediaController for the > route, and fallback to some default setup if it fails? Yes. The Dial MRP can return false for the CreateMediaRouteController() call, and the MRUI will be notified that a controller wasn't instantiated, and won't be sending any MediaStatus updates to the WebUI. The WebUI will initialize the route details view with the title from Route.description like we do now, with all the controls hidden. This will be overridden by status updates from the browser (except in the case of Dial).
9 <span>[[activityStatus_]]</span>	12 <div class="display-name ellipsis">[[routeStatus.title]]</div>

	13 <div class="description ellipsis">[[routeStatus.status]]</div>

	14 <div>

	15 <div>

	16 <div class="time-controls"

	17 hidden="[[!routeStatus.canSeek]]">

	18 <paper-slider class="time-slider"

	19 on-change="onTimeSliderChange_"

	20 on-immediate-value-change="onImmediateTimeSliderChange_"

	21 min="0" max="100"

	22 title="[[i18n('seekTitle')]]"

	23 value="[[timeSliderValue_]]"></paper-slider>

	24 <div class="timeline">

	25 <span class="current-time">

	26 [[getFormattedTime_(displayedCurrentTime_)]]</span>

	27 <span class="duration">[[getDuration_(routeStatus)]]</span>

	28 </div>

	29 </div>

	30 <div class="media-controls">

	31 <span class="button-holder">

	32 <paper-icon-button

	33 hidden="[[!routeStatus.canPlayPause]]"

	34 disabled="[[!routeStatus.canPlayPause]]"

	35 icon="[[getPlayPauseIcon_(routeStatus)]]"

	36 title="[[getPlayPauseTitle_(routeStatus.isPaused)]]"

	37 on-click="onPlayPause_"></paper-icon-button>

	38 <paper-icon-button class="volume-button"

	39 hidden="[[!routeStatus.canMute]]"

	40 icon="[[getMuteUnmuteIcon_(routeStatus)]]"

	41 title="[[getMuteUnmuteTitle_(routeStatus.isMuted)]]"

	42 on-click="onMuteUnmute_"></paper-icon-button>

	43 </span>

	44 <span class="volume-holder">

	45 <paper-slider class="volume-slider"

	46 hidden="[[!routeStatus.canSetVolume]]"

	47 on-change="onVolumeSliderChange_"

	48 on-immediate-value-change="onImmediateVolumeSliderChange_"

	49 title="[[i18n('volumeTitle')]]"

	50 value="[[volumeSliderValue_]]"

	51 min="0" max="1" step="0.01"></paper-slider>

	52 </span>

	53 </div>

	54 </div>

	55 </div>

10 </div>	56 </div>

11 <extensionview id="custom-controller"

12 hidden$="[[isCustomControllerHidden_]]">

13 </extensionview>

14 <div id="route-action-buttons" class="layout">	57 <div id="route-action-buttons" class="layout">

15 <paper-button flat class="route-button button"	58 <paper-button flat class="route-button button"

16 id="start-casting-to-route-button"	59 id="start-casting-to-route-button"

17 hidden$="[[computeCastButtonHidden_(route, changeRouteSourceAvailable_ )]]"	60 hidden$="[[computeCastButtonHidden_(route, changeRouteSourceAvailable_ )]]"

18 on-tap="startCastingToRoute_">	61 on-tap="startCastingToRoute_">

19 <span>[[i18n('startCastingButtonText')]]</span>	62 <span>[[i18n('startCastingButtonText')]]</span>

20 </paper-button>	63 </paper-button>

21 <paper-button flat class="route-button button"	64 <paper-button flat class="route-button button"

22 id="close-route-button"	65 id="close-route-button"

23 on-tap="closeRoute_">	66 on-tap="closeRoute_">

24 <span>[[i18n('stopCastingButtonText')]]</span>	67 <span>[[i18n('stopCastingButtonText')]]</span>

25 </paper-button>	68 </paper-button>

26 <div>	69 <div>

27 </template>	70 </template>

28 <script src="route_details.js"></script>	71 <script src="route_details.js"></script>

29 </dom-module>	72 </dom-module>

OLD	NEW