Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725503002:
[Media Router] Custom Controls 4 - Implement details view WebUI (Closed)
Description[Media Router] Custom Controls 4 - Implement details view WebUI
This CL adds a built-in alternative to the extensionview controller in the route
details view of the media router dialog WebUI. The WebUI route controller is
functionally equivalent to the extensionview controller.
Whenever the WebUI is notified by the browser process of updates to the media
status of the shown route, the details view is updated. Whenever the user
interacts with the media controls on the details view, the media commands are
sent to the browser process.
The switch to the WebUI controller is managed by a Finch experiment that is
currently disabled, so this CL has no behavioral change. Its functionality can
be checked out with the flag --enable-features=MediaRouterUIRouteController.
Before-and-after screenshots:
https://docs.google.com/document/d/1v8jGV33FiFtH_qs2_Ui0UNubt1nHRN_CBrHLt1wRXF0
The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002
2. MediaRouter::GetRouteController(): http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002
4. Custom controls WebUI: this patch
Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4G0/edit
[1] https://codereview.chromium.org/2724513002/
BUG=684633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2725503002
Cr-Commit-Position: refs/heads/master@{#475667}
Committed: https://chromium.googlesource.com/chromium/src/+/3f7923888b023461435906b4b1488a5a857cdc32
Patch Set 1 : . #
Total comments: 17
Patch Set 2 : Address Derek's comments #Patch Set 3 : Support switching between extensionview and new controls #Patch Set 4 : Update #Patch Set 5 : Fix a test #
Total comments: 73
Patch Set 6 : Address Mark's comments #
Total comments: 14
Patch Set 7 : Address Derek's comments, don't load extensionview unnecessarily #
Total comments: 42
Patch Set 8 : Address Mark's comments #
Total comments: 26
Patch Set 9 : Address Jennifer's comments #Patch Set 10 : Remove uses of 'new' in comments #Patch Set 11 : Factor out browserApi, rebase #Patch Set 12 : Try to fix gyp errors #Patch Set 13 : Try to appease Closure #Patch Set 14 : Make Closure happy #
Total comments: 2
Patch Set 15 : Add braces to @implements {Interface} #Messages
Total messages: 102 (75 generated)
Description was changed from ========== Custom Controls UI BUG= ========== to ========== Custom Controls UI BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Custom Controls UI BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Implement Custom Controls WebUI BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI To see the WebUI display dummy data sent from the C++ side, patch this CL [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Implement Custom Controls WebUI To see the WebUI display dummy data sent from the C++ side, patch this CL [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Media Router] Implement Custom Controls WebUI To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
takumif@chromium.org changed reviewers: + imcheng@chromium.org
Hi Derek, this is what I currently have for the WebUI side. You can see it show dummy data by patching https://codereview.chromium.org/2724513002/ as well. If the overall design looks alright to you, I'll start adding tests. Thanks!
Some preliminary comments. Look forward to seeing how this works with the browser side. https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2386: updateRouteStatus: function(status) { Can the media status be passed to the route details element from the entry point without going through the container first? https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> - 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? https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router.js:50: container.addEventListener('pause-route', onPauseRoute); While I appreciate the isolation of browser API from the Polymer elements, I am bit concerned about the amount boilerplate code being introduced, and the indirection makes it a bit difficult to trace the calls. I wonder if we can invoke chrome.send directly. Barring that, can we have the controller invoke media_router.browserApi directly instead of firing event?
https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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... > - 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. > - 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.
Description was changed from ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Implement Custom Controls WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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. > > - 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.
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2386: updateRouteStatus: function(status) { On 2017/03/02 02:25:47, imcheng wrote: > Can the media status be passed to the route details element from the entry point > without going through the container first? To do that, the route details must register/unregister itself with media_router.ui whenever it's created/destroyed. I think it's simpler for the container to go inbetween. https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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. https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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. > https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router.js:50: container.addEventListener('pause-route', onPauseRoute); On 2017/03/02 02:25:47, imcheng wrote: > While I appreciate the isolation of browser API from the Polymer elements, I am > bit concerned about the amount boilerplate code being introduced, and the > indirection makes it a bit difficult to trace the calls. I wonder if we can > invoke chrome.send directly. Barring that, can we have the controller invoke > media_router.browserApi directly instead of firing event? It looks like the layer of event firing is there for testing purposes, to observe the calls happening. I think we should keep it unless we can mock the browserApi.
https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2386: updateRouteStatus: function(status) { On 2017/03/08 23:34:58, takumif wrote: > On 2017/03/02 02:25:47, imcheng wrote: > > Can the media status be passed to the route details element from the entry > point > > without going through the container first? > > To do that, the route details must register/unregister itself with > media_router.ui whenever it's created/destroyed. I think it's simpler for the > container to go inbetween. My thinking is that the WebUI needs to signal to the browser that the route-details/controller is being opened, so that's when it can pass the element itself to the top level WebUI where media commands can be directed to. https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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? https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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? https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router.js:50: container.addEventListener('pause-route', onPauseRoute); On 2017/03/08 23:34:58, takumif wrote: > On 2017/03/02 02:25:47, imcheng wrote: > > While I appreciate the isolation of browser API from the Polymer elements, I > am > > bit concerned about the amount boilerplate code being introduced, and the > > indirection makes it a bit difficult to trace the calls. I wonder if we can > > invoke chrome.send directly. Barring that, can we have the controller invoke > > media_router.browserApi directly instead of firing event? > > It looks like the layer of event firing is there for testing purposes, to > observe the calls happening. I think we should keep it unless we can mock the > browserApi. It looks like we should be able to define chrome.browserApi to point to mock/test versions during testing.
Thanks for reviewing. Please feel free to prioritize other reviews/implementation over this one, since as you know this CL depends on the other three C++ CLs and the extension side CLs. Major changes in this patch: - Factored out route controls into their own Polymer module - Added browser tests for route controls - Made the controls call the browserApi directly rather than firing events, and mocked the browserApi in tests https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2386: updateRouteStatus: function(status) { 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: > > > Can the media status be passed to the route details element from the entry > > point > > > without going through the container first? > > > > To do that, the route details must register/unregister itself with > > media_router.ui whenever it's created/destroyed. I think it's simpler for the > > container to go inbetween. > > My thinking is that the WebUI needs to signal to the browser that the > route-details/controller is being opened, so that's when it can pass the element > itself to the top level WebUI where media commands can be directed to. Okay, giving media_router.ui a |routeControls| attribute that gets set/unset when the details view is opened/closed. https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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). https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/route_details/route_details.html:11: <div id="route-controls"> 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. https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2725503002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router.js:50: container.addEventListener('pause-route', onPauseRoute); On 2017/03/11 21:57:39, imcheng wrote: > On 2017/03/08 23:34:58, takumif wrote: > > On 2017/03/02 02:25:47, imcheng wrote: > > > While I appreciate the isolation of browser API from the Polymer elements, I > > am > > > bit concerned about the amount boilerplate code being introduced, and the > > > indirection makes it a bit difficult to trace the calls. I wonder if we can > > > invoke chrome.send directly. Barring that, can we have the controller invoke > > > media_router.browserApi directly instead of firing event? > > > > It looks like the layer of event firing is there for testing purposes, to > > observe the calls happening. I think we should keep it unless we can mock the > > browserApi. > > It looks like we should be able to define chrome.browserApi to point to > mock/test versions during testing. Done.
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
takumif@chromium.org changed reviewers: + mfoltz@chromium.org
+mfoltz Please take a look, thanks!
Patchset #3 (id:100001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. To see the WebUI display dummy data sent from the C++ side, apply this patch [1] as well. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
FYI, you can now try this out with Derek's Hangouts route controller (http://cl/150905615). Patch Custom Control CLs #3 and #4 onto Chromium, and run with the flag --enable-features=MediaRouterUIRouteController.
Patchset #4 (id:140001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL replaces the <extensionview> in the route details view of the media router dialog WebUI with media controls that have been migrated from the Cast media route provider in the media router component extension. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
takumif@chromium.org changed reviewers: + apacible@chromium.org
+apacible@: Please take a look at the usage of Polymer in route_controls.*, route_details.*, and media_router_container.*. Thanks!
Overall looks good and most of my comments were bikeshedding names. A couple of high level things: - If elements can be constructed with required objects (even if they have dummy values) that would remove a number of null checks. Of course that means you have to find appropriate dummy values to initialize the element. - The 'isExtensionViewHidden' flag was confusing to read - does it mean it is supposed to exist but is currently hidden, or should not exist at all? Can names/flags be clarified to indicate the intention vs the state? Maybe the route details should have an enum CONTROLLER_NONE, CONTROLLER_EXTENSION, CONTROLLER_WEBUI to indicate the desired state of the controller. - Can we avoid instantiating the <extensionview> altogether unless it's going to be populated/shown? - What title do we show for the controller: the route title/description or the media status title/description? Does one override the other? Does the MRP just end up copying one to the other? I was a little unclear from reading the patch. Thanks, m. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:394: useNewRouteControls: { Nit: s/New/WebUI/ to be more specific (as "new" doesn't tell you what we're switching to) https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1097: this.$$('route-details').onClosed(); Is a close-controller call sent when the route details was closed via timeout or close-dialog gesture? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2289: media_router.browserApi.onMediaControllerAvailable(route.id); What if there are no media controls for a route? Instantiating a media controller in the browser seems like a waste then. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.html (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:23: min="0" max="1" step="0.01" Should this be in seconds reflecting the media duration, or do only the relative values matter? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:26: <div id="timeline"> Presumably, non-seekable live media can still have a current_time (reflecting the lifetime of the stream) even if can_seek is false. If hiding the current_time is the existing behavior, fine to keep things this way though. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:39: title="[[getPlayPauseTitle_(routeStatus_.isPaused)]]" Should this be i18n('play') or i18n('pause')? Can you use a conditional expression here? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:43: hidden="[[!routeStatus_.canMute]]" disable this control as well for consistency with play-pause-button https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:45: title="[[getMuteUnmuteTitle_(routeStatus_.isMuted)]]" Similar comment here https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:51: hidden="[[!routeStatus_.canSetVolume]]" Also disable if hidden https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:25: ignoreExternalTimeUpdates_: { This might be clearer as isSeeking_, but I don't feel strongly. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:35: ignoreExternalVolumeUpdates_: { isVolumeChanging_ https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:43: * route name. I don't follow how a media status update is related to the route name. It seems like those are separate things. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:53: * @type {?media_router.Route} Must this be null on construction? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:63: * updateRouteStatus() to discern from internal updates. s/status/media status/ Can you explain "internal" versus "external" updates? I assume an "internal" update happens when the user manipulates the controls, and an "external" update happens when the browser sends a MediaStatus object to the WebUI. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:73: * The value of the time slider, between 0 and 1. s/time slider/seek bar/ Shouldn't this be in seconds? Or does the polymer element only accept fractional values? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:76: timeSliderValue_: { currentTimeFraction_ ? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:82: * The value of the volume slider, between 0 and 1. s/slider/control/ https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:119: var hours = Math.floor(timeInSec / 3600); s/var/const/ https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:126: return timeParts.join(':'); This can be combined with the previous line. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:137: return (routeStatus && routeStatus.isMuted) ? 'av:volume-off' : Can we initialize the element with a routeStatus to avoid all of these null checks? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:181: getSeekPosition_: function(seekPositionRatio, duration) { getSeekTime_(seekPosition, duration) ? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:182: return duration ? Math.floor(seekPositionRatio * duration) : 0; I don't think this extra check is necessary; Math.floor(0) = 0 https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:193: getSeekPositionRatio_: function(seekPosition, duration) { getSeekPosition_(seekTime, duration) ? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:203: onImmediateTimeSliderChange_: function(e) { onSeekStart_ ? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:218: onImmediateVolumeSliderChange_: function(e) { onVolumeChangeStart_? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:226: * Sends a mute or unmute command to the browser. Called when the user toggles the mute status of the media https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:235: * Sends a play or pause command to the browser. Called when the user toggles between playing and pausing the media https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:257: this.routeStatus_ = new media_router.RouteStatus( Would this be simpler as just a <div> with text content? Can it share style with the route description in the sink list? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:299: onTimeSliderChange_: function(e) { onSeekComplete_ https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:318: onVolumeSliderChange_: function(e) { onVolumeChangeComplete_ https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:342: this.receivedStatusUpdates_ = true; Is this the same as testing this.routeStatus_ ? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:14: hidden$="[[!shouldShowExtensionView_(useNewRouteControls, isExtensionViewHidden_)]]"> Does <hidden> prevent instantiation of the <extensionview> element or only adjust its CSS display property? It would be preferable to not instantiate the <extensionview> at all so we can accurately measure the performance impact of switching to WebUI controls. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:240: return !useNewRouteControls && !isExtensionViewHidden; If !isExtensionViewHidden is it already being shown? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:250: shouldShowRouteInfo_: function(useNewRouteControls, isExtensionViewHidden) { Is this for routes that have no media controls? It might be clearer as shouldShowRouteInfoOnly_() https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:264: if (this.route.canJoin) { "Joining" or "changing source" should just be another cast mode at the end of the day, but that requires migrating a bunch of logic to the browser. Some day with the native rewrite... https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:219: /** @type {{sinkId: string, selectedCastModeValue: number}} */ Good catch https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_common.css (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_common.css:8: --dialog-width: 340px; Why this change?
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:394: useNewRouteControls: { On 2017/05/03 21:26:59, mark a. foltz wrote: > Nit: s/New/WebUI/ to be more specific (as "new" doesn't tell you what we're > switching to) Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1097: this.$$('route-details').onClosed(); On 2017/05/03 21:26:59, mark a. foltz wrote: > Is a close-controller call sent when the route details was closed via timeout or > close-dialog gesture? In those cases no it doesn't get sent, but it's not necessary because the C++ side MRUI instance gets destroyed as well. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2289: media_router.browserApi.onMediaControllerAvailable(route.id); On 2017/05/03 21:26:59, mark a. foltz wrote: > What if there are no media controls for a route? Instantiating a media > controller in the browser seems like a waste then. Do you mean a route that doesn't accept any media controls? In that case the controller will simply be a provider of media status updates, which I suppose would contain just title/description. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.html (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:23: min="0" max="1" step="0.01" On 2017/05/03 21:26:59, mark a. foltz wrote: > Should this be in seconds reflecting the media duration, or do only the relative > values matter? Yup, doing seconds here would be a good idea. Changed. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:26: <div id="timeline"> On 2017/05/03 21:26:59, mark a. foltz wrote: > Presumably, non-seekable live media can still have a current_time (reflecting > the lifetime of the stream) even if can_seek is false. > > If hiding the current_time is the existing behavior, fine to keep things this > way though. This is the existing behavior. We can change it in the future if there's a need to support non-seekable media with current_time. We'd want to show a disabled slider if it has duration too. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:39: title="[[getPlayPauseTitle_(routeStatus_.isPaused)]]" On 2017/05/03 21:26:59, mark a. foltz wrote: > Should this be i18n('play') or i18n('pause')? Can you use a conditional > expression here? getPlayPauseTitle_() defined in route_controls.js returns the i18n-ized text. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:43: hidden="[[!routeStatus_.canMute]]" On 2017/05/03 21:26:59, mark a. foltz wrote: > disable this control as well for consistency with play-pause-button Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:45: title="[[getMuteUnmuteTitle_(routeStatus_.isMuted)]]" On 2017/05/03 21:26:59, mark a. foltz wrote: > Similar comment here Same, i18n-ized text is returned by JS. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:51: hidden="[[!routeStatus_.canSetVolume]]" On 2017/05/03 21:26:59, mark a. foltz wrote: > Also disable if hidden Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:25: ignoreExternalTimeUpdates_: { On 2017/05/03 21:27:00, mark a. foltz wrote: > This might be clearer as isSeeking_, but I don't feel strongly. Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:35: ignoreExternalVolumeUpdates_: { On 2017/05/03 21:27:00, mark a. foltz wrote: > isVolumeChanging_ Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:43: * route name. On 2017/05/03 21:26:59, mark a. foltz wrote: > I don't follow how a media status update is related to the route name. It seems > like those are separate things. Removed this variable. I wanted to use route description as the description (instead of title to look the same as the extensionview) in case the media status didn't include a description. Doing it by creating a displayedDescription_ property that gets updated by route and media status updates. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:53: * @type {?media_router.Route} On 2017/05/03 21:26:59, mark a. foltz wrote: > Must this be null on construction? Removed. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:63: * updateRouteStatus() to discern from internal updates. On 2017/05/03 21:27:00, mark a. foltz wrote: > s/status/media status/ > > Can you explain "internal" versus "external" updates? I assume an "internal" > update happens when the user manipulates the controls, and an "external" update > happens when the browser sends a MediaStatus object to the WebUI. Removed. This was to discern updates by status object instantiated by this file vs external. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:73: * The value of the time slider, between 0 and 1. On 2017/05/03 21:26:59, mark a. foltz wrote: > s/time slider/seek bar/ > Shouldn't this be in seconds? Or does the polymer element only accept > fractional values? Switched to seconds. Also merged with displayedCurrentTime_. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:76: timeSliderValue_: { On 2017/05/03 21:26:59, mark a. foltz wrote: > currentTimeFraction_ ? Merged with displayedCurrentTime_ https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:82: * The value of the volume slider, between 0 and 1. On 2017/05/03 21:27:00, mark a. foltz wrote: > s/slider/control/ Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:119: var hours = Math.floor(timeInSec / 3600); On 2017/05/03 21:27:00, mark a. foltz wrote: > s/var/const/ The presubmit check tells us to use var instead of const :( https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:126: return timeParts.join(':'); On 2017/05/03 21:27:00, mark a. foltz wrote: > This can be combined with the previous line. Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:137: return (routeStatus && routeStatus.isMuted) ? 'av:volume-off' : On 2017/05/03 21:27:00, mark a. foltz wrote: > Can we initialize the element with a routeStatus to avoid all of these null > checks? Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:181: getSeekPosition_: function(seekPositionRatio, duration) { On 2017/05/03 21:27:00, mark a. foltz wrote: > getSeekTime_(seekPosition, duration) ? Using seconds in the slider, and removing this function and the one below. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:182: return duration ? Math.floor(seekPositionRatio * duration) : 0; On 2017/05/03 21:27:00, mark a. foltz wrote: > I don't think this extra check is necessary; Math.floor(0) = 0 Removing. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:193: getSeekPositionRatio_: function(seekPosition, duration) { On 2017/05/03 21:27:00, mark a. foltz wrote: > getSeekPosition_(seekTime, duration) ? Removing. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:203: onImmediateTimeSliderChange_: function(e) { On 2017/05/03 21:26:59, mark a. foltz wrote: > onSeekStart_ ? Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:218: onImmediateVolumeSliderChange_: function(e) { On 2017/05/03 21:27:00, mark a. foltz wrote: > onVolumeChangeStart_? Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:226: * Sends a mute or unmute command to the browser. On 2017/05/03 21:27:00, mark a. foltz wrote: > Called when the user toggles the mute status of the media Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:235: * Sends a play or pause command to the browser. On 2017/05/03 21:27:00, mark a. foltz wrote: > Called when the user toggles between playing and pausing the media Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:257: this.routeStatus_ = new media_router.RouteStatus( On 2017/05/03 21:27:00, mark a. foltz wrote: > Would this be simpler as just a <div> with text content? Can it share style > with the route description in the sink list? Creating a displayedDescription_ property to set instead. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:299: onTimeSliderChange_: function(e) { On 2017/05/03 21:27:00, mark a. foltz wrote: > onSeekComplete_ Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:318: onVolumeSliderChange_: function(e) { On 2017/05/03 21:27:00, mark a. foltz wrote: > onVolumeChangeComplete_ Done. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:342: this.receivedStatusUpdates_ = true; On 2017/05/03 21:27:00, mark a. foltz wrote: > Is this the same as testing this.routeStatus_ ? Removed. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:14: hidden$="[[!shouldShowExtensionView_(useNewRouteControls, isExtensionViewHidden_)]]"> On 2017/05/03 21:27:01, mark a. foltz wrote: > Does <hidden> prevent instantiation of the <extensionview> element or only > adjust its CSS display property? > > It would be preferable to not instantiate the <extensionview> at all so we can > accurately measure the performance impact of switching to WebUI controls. <template is="dom-if"> seems to do that. But it makes the extensionview load later than we currently use it, so we'll need to set a callback for loading. I couldn't find a nice way to do that -- Jennifer, do you know how? https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:240: return !useNewRouteControls && !isExtensionViewHidden; On 2017/05/03 21:27:01, mark a. foltz wrote: > If !isExtensionViewHidden is it already being shown? Renamed the variable to isExtensionViewReady https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:250: shouldShowRouteInfo_: function(useNewRouteControls, isExtensionViewHidden) { On 2017/05/03 21:27:01, mark a. foltz wrote: > Is this for routes that have no media controls? > It might be clearer as shouldShowRouteInfoOnly_() This is for when extension view is enabled but the route doesn't have one. Will be removed along with the extension view in the future. Renamed. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:264: if (this.route.canJoin) { On 2017/05/03 21:27:01, mark a. foltz wrote: > "Joining" or "changing source" should just be another cast mode at the end of > the day, but that requires migrating a bunch of logic to the browser. Some day > with the native rewrite... Acknowledged. https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_common.css (right): https://codereview.chromium.org/2725503002/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_common.css:8: --dialog-width: 340px; On 2017/05/03 21:27:01, mark a. foltz wrote: > Why this change? Without this, the dialog shows a disabled scroll bar at the bottom.
Thanks for reviewing, Mark. Jennifer, I wanted to make the loading of the extension view optional using <template is="dom-if">, but I wasn't sure how to set a callback for when it gets loaded. Do you know how I can do that? On 2017/05/03 21:27:01, mark a. foltz wrote: > Overall looks good and most of my comments were bikeshedding names. A couple of > high level things: > > - If elements can be constructed with required objects (even if they have dummy > values) that would remove a number of null checks. Of course that means you > have to find appropriate dummy values to initialize the element. Done. > > - The 'isExtensionViewHidden' flag was confusing to read - does it mean it is > supposed to exist but is currently hidden, or should not exist at all? Can > names/flags be clarified to indicate the intention vs the state? Renamed to isExtensionViewReady. > > Maybe the route details should have an enum CONTROLLER_NONE, > CONTROLLER_EXTENSION, CONTROLLER_WEBUI to indicate the desired state of the > controller. Done. > > - Can we avoid instantiating the <extensionview> altogether unless it's going to > be populated/shown? Wanted to use dom-if, but need to figure out how to set a callback for load event. > > - What title do we show for the controller: the route title/description or the > media status title/description? Does one override the other? Does the MRP just > end up copying one to the other? I was a little unclear from reading the patch. (It should've been the description instead of the title to keep the behavior consistent with the extension view.) The description will use the route description if none is provided by the route status.
Looks good overall. It would be a good idea to stamp the <extensionview> conditionally. We discussed offline but Jennifer probably has some ideas as well. https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2289: media_router.browserApi.onMediaControllerAvailable(route.id); Shouldn't this only be called if we are using the webui based controller? I was going to suggest that we move this call inside route-details onOpened(), but I wasn't sure if this.$$('route-details') will be non-null if we are stamping route-details. Maybe route-details could have a ready method that also calls onOpened()? https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:165: onRouteDetailsClosed: function() { nit: Maybe call this method reset or detach? https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:88: * extension view. @type {boolean} https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:150: * @param {bool} useWebUiRouteControls s/bool/boolean here and below https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:242: media_router.ui.setRouteControls(this.$$('route-controls')); Similar comment to the one in media_router_container.js -- this should only be called if we are using the webui controller. So maybe this coudld be: if (this.$$('route-controls')) { this.$$('route-controls').attach(); } (where attach() would call media_router.ui.setRouteControls(this);, and is also called on ready()) https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:37: container.onRouteControllerInvalidated(); Do you need to set routeControls to null here? https://codereview.chromium.org/2725503002/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_search_tests.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_search_tests.js:124: cr.define('media_router.browserApi', function() { Can this go in media_router_elements_browsertest.js?
Thanks for reviewing! https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2289: media_router.browserApi.onMediaControllerAvailable(route.id); On 2017/05/05 21:36:40, imcheng wrote: > Shouldn't this only be called if we are using the webui based controller? > > I was going to suggest that we move this call inside route-details onOpened(), > but I wasn't sure if this.$$('route-details') will be non-null if we are > stamping route-details. Maybe route-details could have a ready method that also > calls onOpened()? Yes, ready() should call onOpened() as well, since on the first pass this.$$('route-details') wouldn't exist yet. The problem with calling onMediaControllerAvailable() from onOpened() is that we can't guarantee that |route| is set when onOpened() gets called. Checking for |this.useWebUiRouteControls| in showRouteDetails_() should work instead. https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:165: onRouteDetailsClosed: function() { On 2017/05/05 21:36:40, imcheng wrote: > nit: Maybe call this method reset or detach? Renamed to reset(). https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:88: * extension view. On 2017/05/05 21:36:40, imcheng wrote: > @type {boolean} Done. https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:150: * @param {bool} useWebUiRouteControls On 2017/05/05 21:36:41, imcheng wrote: > s/bool/boolean here and below Done. https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:242: media_router.ui.setRouteControls(this.$$('route-controls')); On 2017/05/05 21:36:40, imcheng wrote: > Similar comment to the one in media_router_container.js -- this should only be > called if we are using the webui controller. So maybe this coudld be: > > if (this.$$('route-controls')) { > this.$$('route-controls').attach(); > } > > (where attach() would call media_router.ui.setRouteControls(this);, and is also > called on ready()) Thinking about this, since route-controls isn't a dom-if, it should always exist. So there's no need to check for existence (and we can use $[] instead of $$() ). We can use this.useWebUiRouteControls to check if we should call this. https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:37: container.onRouteControllerInvalidated(); On 2017/05/05 21:36:41, imcheng wrote: > Do you need to set routeControls to null here? We already do that in RouteControls.reset(), although we could do it here instead. https://codereview.chromium.org/2725503002/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_search_tests.js (right): https://codereview.chromium.org/2725503002/diff/240001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_search_tests.js:124: cr.define('media_router.browserApi', function() { On 2017/05/05 21:36:41, imcheng wrote: > Can this go in media_router_elements_browsertest.js? This methods needs the Polymer method to fire events on (e.g. |container| here), so if we defined this method in media_router_elements_browsertest.js, we'd have to pass it into individual files as an argument to registerTests() and call it in each file. Would that be better?
Overall looks good with mostly minor comments. This is a lot of code, but it was quite readable. I didn't intend for you to have to create a new template for the extensionview. Maybe it is cleaner since you can track loading progress now? At any rate it's okay to add a bit of complexity in the near term as we can now cleanly remove the extensionview once WebUI launches. I'm still a little fuzzy how route titles/descriptions are inserted into the WebUI. There seem to be a couple of places where that is done? Probably playing around with a demo would help. In general it would be good to add screenshots in the CL description with different configurations of the WebUI: - RTL and LTR language - Route controls with title and no title - Different controls enabled/disabled - Original route details (with extensionview) for comparison Are you working from the original redlines for the Media Router UI? At some point it would be good to do a check against them to see if there are any differences. I haven't looked in detail at the new test cases and will do that on the next pass. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:390: * Whether the new web UI route controls should be shown instead of the Nit: WebUI here and elsewhere. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:391: * extension view in the route details view. extensionview https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1083: * @param {?media_router.MediaRouterView} oldCurrentView The old current view previousView ? https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.html (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:12: [[displayedDescription_]] This is the route status description or route description, correct? https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:42: title="[[getPlayPauseTitle_(routeStatus.isPaused)]]" ISTM that these helper methods should take |routeStatus| consistently. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:12: * The current time displayed in seconds. Nit: in seconds, before formatting. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:155: if (this.routeStatus.isPaused) { This may be out of sync with the true state of the media (since there is a delay propagating changes to the WebUI). However Cast applications have had to deal with this already since it's a multi-controller model so I don't anticipate any new complications. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:185: if (newRouteStatus.description !== '') { If the new route status description is empty, should you fall back to route description here? https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:198: loadTimeData.getStringF('castingActivityStatus', route.description); Why is the route description inserted into a formatted string (while the route status description is taken as-is)? https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:211: var target = e.target; This can be inlined. Similar comment applies below. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:36: computed: 'computeControllerType_(useWebUiRouteControls,' + Can the parameters be pulled from the element instead of being passed in here? Mostly for readability. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:38: }, Do you need an observer to update this value if isExtensionViewReady changes? Or is this the controller that *eventually* will be shown regardless of what is currently displayed (i.e. is EXTENSION_VIEW while the extensionview is loading). https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:197: updateActivityStatus_: function() { This seems to duplicate displayedDescription_. What is the difference? https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:208: if (this.useWebUiRouteControls) { This could use controllerType_ == WEBUI as logically that represents the current state of the controls, and assuming it's maintained. It also makes it clearer in some places what code is mutually exclusive. Of course it depends on the meaning of controllerType_. (comment applies throughout) https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_search_tests.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_search_tests.js:123: var overrideBrowserApi = function() { Can this be moved into the common setup code?
Tests look great - really well written. https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:73: var overrideBrowserApi = function() { installMockBrowserApi()? https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/route_controls_tests.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:27: var checkText = function(expected, elementId) { assertElementText ? https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:106: 'chrome-extension://123/custom_view.html'); Nit: It doesn't look like there's any test coverage of the extension view. Maybe drop the URL? https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:146: assertFalse(isElementShown('route-play-pause-button')); Nit: You could have assertElementShown() and assertElementHidden() which would be slightly more readable.
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots (needs corp login): https://drive.google.com/corp/drive/u/0/folders/0BxrKSYHNwjQGMTFfSkE2UkxldnM The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots (needs corp login): https://drive.google.com/corp/drive/u/0/folders/0BxrKSYHNwjQGMTFfSkE2UkxldnM The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots (needs corp login): https://docs.google.com/document/d/1eiWQJ31h9wO9vhAtHQuQadKcuYUm9VTRJPneL-H8qNA The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #9 (id:300001) has been deleted
https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:390: * Whether the new web UI route controls should be shown instead of the On 2017/05/12 00:02:46, mark a. foltz wrote: > Nit: WebUI here and elsewhere. Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:391: * extension view in the route details view. On 2017/05/12 00:02:46, mark a. foltz wrote: > extensionview Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1083: * @param {?media_router.MediaRouterView} oldCurrentView The old current view On 2017/05/12 00:02:46, mark a. foltz wrote: > previousView ? Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.html (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:12: [[displayedDescription_]] On 2017/05/12 00:02:46, mark a. foltz wrote: > This is the route status description or route description, correct? Yes. And route status description takes precedence over route description. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.html:42: title="[[getPlayPauseTitle_(routeStatus.isPaused)]]" On 2017/05/12 00:02:46, mark a. foltz wrote: > ISTM that these helper methods should take |routeStatus| consistently. Right, there was a mismatch in the parameters and the arguments. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:12: * The current time displayed in seconds. On 2017/05/12 00:02:46, mark a. foltz wrote: > Nit: in seconds, before formatting. Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:155: if (this.routeStatus.isPaused) { On 2017/05/12 00:02:46, mark a. foltz wrote: > This may be out of sync with the true state of the media (since there is a delay > propagating changes to the WebUI). However Cast applications have had to deal > with this already since it's a multi-controller model so I don't anticipate any > new complications. Acknowledged. I think determining user intention based on what is shown by the WebUI is fine, since if the user is clicking the button they likely want a state change from what is displayed. Also the extension-side controller checks if the commands are consistent with the media status. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:185: if (newRouteStatus.description !== '') { On 2017/05/12 00:02:46, mark a. foltz wrote: > If the new route status description is empty, should you fall back to route > description here? displayedDescription_ would already be set to either the route description or the description of the old route status, so no need to update here (unless route description should take precedence over stale route status description, but I'm not sure about that). https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:198: loadTimeData.getStringF('castingActivityStatus', route.description); On 2017/05/12 00:02:46, mark a. foltz wrote: > Why is the route description inserted into a formatted string (while the route > status description is taken as-is)? This is to be consistent with the existing behavior. Currently the extensionview doesn't use the formatting, but the fallback description (that is shown when the extensionview doesn't load) does. The formatting simply prefixes "Casting: ", so perhaps it's not necessary. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:211: var target = e.target; On 2017/05/12 00:02:46, mark a. foltz wrote: > This can be inlined. Similar comment applies below. Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:36: computed: 'computeControllerType_(useWebUiRouteControls,' + On 2017/05/12 00:02:46, mark a. foltz wrote: > Can the parameters be pulled from the element instead of being passed in here? > Mostly for readability. They are passed in here so that when they change, the value of |controllerType_| is updated. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:38: }, On 2017/05/12 00:02:46, mark a. foltz wrote: > Do you need an observer to update this value if isExtensionViewReady changes? > Or is this the controller that *eventually* will be shown regardless of what is > currently displayed (i.e. is EXTENSION_VIEW while the extensionview is loading). The method in the "computed" attribute gets called every time its arguments (useWebUiRouteControls, isExtensionViewReady) change, so no observer is necessary. This is also why they need to be passed in here. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:197: updateActivityStatus_: function() { On 2017/05/12 00:02:47, mark a. foltz wrote: > This seems to duplicate displayedDescription_. What is the difference? |activityStatus_| is the fallback route description that is shown when we attempt but fail to load the extensionview. |displayedDescription_| in the new controller shows the same thing if a description isn't provided by route status updates. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:208: if (this.useWebUiRouteControls) { On 2017/05/12 00:02:46, mark a. foltz wrote: > This could use controllerType_ == WEBUI as logically that represents the current > state of the controls, and assuming it's maintained. It also makes it clearer in > some places what code is mutually exclusive. Of course it depends on the > meaning of controllerType_. > (comment applies throughout) Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:73: var overrideBrowserApi = function() { On 2017/05/12 21:07:34, mark a. foltz wrote: > installMockBrowserApi()? Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_search_tests.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_search_tests.js:123: var overrideBrowserApi = function() { On 2017/05/12 00:02:47, mark a. foltz wrote: > Can this be moved into the common setup code? Done. Figured out that HTMLElement#dispatchEvent does something similar to Polymer.Base#fire. https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/route_controls_tests.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:27: var checkText = function(expected, elementId) { On 2017/05/12 21:07:34, mark a. foltz wrote: > assertElementText ? Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:106: 'chrome-extension://123/custom_view.html'); On 2017/05/12 21:07:35, mark a. foltz wrote: > Nit: It doesn't look like there's any test coverage of the extension view. Maybe > drop the URL? Done. https://codereview.chromium.org/2725503002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:146: assertFalse(isElementShown('route-play-pause-button')); On 2017/05/12 21:07:34, mark a. foltz wrote: > Nit: You could have assertElementShown() and assertElementHidden() which would > be slightly more readable. Done.
On 2017/05/12 00:02:47, mark a. foltz wrote: > Overall looks good with mostly minor comments. This is a lot of code, but it > was quite readable. > > I didn't intend for you to have to create a new template for the extensionview. > Maybe it is cleaner since you can track loading progress now? At any rate it's > okay to add a bit of complexity in the near term as we can now cleanly remove > the extensionview once WebUI launches. Creating a template for the extensionview seemed to be the cleanest way to make it not load if unnecessary. > > I'm still a little fuzzy how route titles/descriptions are inserted into the > WebUI. There seem to be a couple of places where that is done? Probably > playing around with a demo would help. If route status is provided, we use its description, and otherwise we use "Casting: [[route.status]]". This is to be consistent with the current behavior. > > In general it would be good to add screenshots in the CL description with > different configurations of the WebUI: > > - RTL and LTR language > - Route controls with title and no title > - Different controls enabled/disabled > - Original route details (with extensionview) for comparison Done, added the link to the description. > > Are you working from the original redlines for the Media Router UI? At some > point it would be good to do a check against them to see if there are any > differences. Right now the biggest difference is that in the new controller for RTL the slider direction gets reversed as well. This behavior is inconsistent with that of YouTube and default HTML video controller so I'll fix it. > > I haven't looked in detail at the new test cases and will do that on the next > pass. >
LGTM Can you put the screenshots in a document on chromium.org so that others can view them? https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:185: if (newRouteStatus.description !== '') { On 2017/05/15 17:13:12, takumif wrote: > On 2017/05/12 00:02:46, mark a. foltz wrote: > > If the new route status description is empty, should you fall back to route > > description here? > > displayedDescription_ would already be set to either the route description or > the description of the old route status, so no need to update here (unless route > description should take precedence over stale route status description, but I'm > not sure about that). Acknowledged. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:198: loadTimeData.getStringF('castingActivityStatus', route.description); On 2017/05/15 17:13:12, takumif wrote: > On 2017/05/12 00:02:46, mark a. foltz wrote: > > Why is the route description inserted into a formatted string (while the route > > status description is taken as-is)? > > This is to be consistent with the existing behavior. Currently the extensionview > doesn't use the formatting, but the fallback description (that is shown when the > extensionview doesn't load) does. The formatting simply prefixes "Casting: ", so > perhaps it's not necessary. Okay, this inconsistency is a bit head scratching, but it can be cleaned up once we land everything. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:36: computed: 'computeControllerType_(useWebUiRouteControls,' + On 2017/05/15 17:13:12, takumif wrote: > On 2017/05/12 00:02:46, mark a. foltz wrote: > > Can the parameters be pulled from the element instead of being passed in here? > > Mostly for readability. > > They are passed in here so that when they change, the value of |controllerType_| > is updated. Acknowledged. https://codereview.chromium.org/2725503002/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:38: }, On 2017/05/15 17:13:12, takumif wrote: > On 2017/05/12 00:02:46, mark a. foltz wrote: > > Do you need an observer to update this value if isExtensionViewReady changes? > > Or is this the controller that *eventually* will be shown regardless of what > is > > currently displayed (i.e. is EXTENSION_VIEW while the extensionview is > loading). > > The method in the "computed" attribute gets called every time its arguments > (useWebUiRouteControls, isExtensionViewReady) change, so no observer is > necessary. This is also why they need to be passed in here. Acknowledged. https://codereview.chromium.org/2725503002/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/media_router/route_controls_tests.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:193: function waitForUnuteEvent(data) { Typo
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots (needs corp login): https://docs.google.com/document/d/1eiWQJ31h9wO9vhAtHQuQadKcuYUm9VTRJPneL-H8qNA The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots: https://docs.google.com/document/d/1v8jGV33FiFtH_qs2_Ui0UNubt1nHRN_CBrHLt1wRXF0 The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Nice work! https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:390: * Whether the new WebUI route controls should be shown instead of the Please avoid using "new" or "old" since this will likely be changing in the future as well. Is there a name this can be referred to as? https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1083: * @param {?media_router.MediaRouterView} previousView The old current view nit: "The previous |currentView|" may be more explicit. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:17: value: 0 nit: trailing commas please :) https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/extension_view_wrapper/extension_view_wrapper.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/extension_view_wrapper/extension_view_wrapper.js:5: // This Polymer element shows the custom controller for a route using extension nit: extensionview (one word) https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:20: <route-controls id="route-controls" What are the pros/cons of keeping <route-controls> out of a dom-if template? Are we more likely to use the route-controls? What is performance difference like? https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:201: ''; I feel this is an awkward placement for this line, but defer to the formatter. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:205: * Called when the route details view is closed. For onClosed/onOpened -- prefer comments be something like, "Resets route-controls when details view is closed" https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:240: ready: function() { nit: Move this to right after the list of behaviors as this is a Polymer called function. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:146: * getting closed. Does this refer to the process of getting closed (e.g. while on tear down, route controls was set again), or when the route details view is already closed? https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:494: * @param {number} volume The volume between 0 and 1. Does the volume persist across route instances? https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:494: * @param {number} volume The volume between 0 and 1. Are there minimal increments between volume values?
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The new route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the new controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots: https://docs.google.com/document/d/1v8jGV33FiFtH_qs2_Ui0UNubt1nHRN_CBrHLt1wRXF0 The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The WebUI route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the WebUI controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots: https://docs.google.com/document/d/1v8jGV33FiFtH_qs2_Ui0UNubt1nHRN_CBrHLt1wRXF0 The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for reviewing! https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:390: * Whether the new WebUI route controls should be shown instead of the On 2017/05/17 21:52:17, apacible wrote: > Please avoid using "new" or "old" since this will likely be changing in the > future as well. Is there a name this can be referred to as? Removed 'new'. I think calling it 'WebUI route controls' as opposed to 'extensionview (route controls)' is sufficient. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1083: * @param {?media_router.MediaRouterView} previousView The old current view On 2017/05/17 21:52:17, apacible wrote: > nit: "The previous |currentView|" may be more explicit. Done. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:17: value: 0 On 2017/05/17 21:52:17, apacible wrote: > nit: trailing commas please :) Done. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/extension_view_wrapper/extension_view_wrapper.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/extension_view_wrapper/extension_view_wrapper.js:5: // This Polymer element shows the custom controller for a route using extension On 2017/05/17 21:52:17, apacible wrote: > nit: extensionview (one word) Done. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:20: <route-controls id="route-controls" On 2017/05/17 21:52:17, apacible wrote: > What are the pros/cons of keeping <route-controls> out of a dom-if template? Are > we more likely to use the route-controls? What is performance difference like? Actually, since we'll be comparing the load times against extensionview it'd make sense to put it in dom-if as well. Doing that, and when we remove extensionview entirely, we'll take it out of dom-if again. Putting route-controls in dom-if seems to increase its load time by ~50ms. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:201: ''; On 2017/05/17 21:52:17, apacible wrote: > I feel this is an awkward placement for this line, but defer to the formatter. Since "git cl format" doesn't format JS, I copied over this function to a google3 repo to see how it gets formatted there :) Interestingly the lines above this got changed but not this one. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:205: * Called when the route details view is closed. On 2017/05/17 21:52:17, apacible wrote: > For onClosed/onOpened -- prefer comments be something like, "Resets > route-controls when details view is closed" Done. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.js:240: ready: function() { On 2017/05/17 21:52:17, apacible wrote: > nit: Move this to right after the list of behaviors as this is a Polymer called > function. Moving this functionality to ready() in route-controls since it's going to be loaded dynamically. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:146: * getting closed. On 2017/05/17 21:52:17, apacible wrote: > Does this refer to the process of getting closed (e.g. while on tear down, route > controls was set again), or when the route details view is already closed? More like the latter. When we navigate away from route details view (e.g. back to sink list) we call this function with null so that we don't update the route controls while it's hidden. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:494: * @param {number} volume The volume between 0 and 1. On 2017/05/17 21:52:17, apacible wrote: > Are there minimal increments between volume values? Not that I'm aware of. The slider is set to use 0.01 increments. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:494: * @param {number} volume The volume between 0 and 1. On 2017/05/17 21:52:17, apacible wrote: > Does the volume persist across route instances? Yes, volume/mute persists across routes on the same sink. That behavior hasn't changed from extensionview. https://codereview.chromium.org/2725503002/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/media_router/route_controls_tests.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/test/data/webui... chrome/test/data/webui/media_router/route_controls_tests.js:193: function waitForUnuteEvent(data) { On 2017/05/15 20:43:22, mark a. foltz wrote: > Typo Oh no. Thanks for noticing!
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #16 (id:460001) has been deleted
lgtm https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:390: * Whether the new WebUI route controls should be shown instead of the On 2017/05/18 18:01:55, takumif wrote: > On 2017/05/17 21:52:17, apacible wrote: > > Please avoid using "new" or "old" since this will likely be changing in the > > future as well. Is there a name this can be referred to as? > > Removed 'new'. I think calling it 'WebUI route controls' as opposed to > 'extensionview (route controls)' is sufficient. SGTM. https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_details/route_details.html (right): https://codereview.chromium.org/2725503002/diff/280001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_details/route_details.html:20: <route-controls id="route-controls" On 2017/05/18 18:01:55, takumif wrote: > On 2017/05/17 21:52:17, apacible wrote: > > What are the pros/cons of keeping <route-controls> out of a dom-if template? > Are > > we more likely to use the route-controls? What is performance difference like? > > Actually, since we'll be comparing the load times against extensionview it'd > make sense to put it in dom-if as well. Doing that, and when we remove > extensionview entirely, we'll take it out of dom-if again. Putting > route-controls in dom-if seems to increase its load time by ~50ms. SGTM.
Patchset #17 (id:500001) has been deleted
Patchset #18 (id:540001) has been deleted
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2725503002/diff/560001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/560001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:206: */ Does adding @override here and in reset() work? Similar comment for other impl classes.
Patchset #11 (id:360001) has been deleted
Patchset #12 (id:400001) has been deleted
Patchset #12 (id:420001) has been deleted
Patchset #11 (id:380001) has been deleted
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2725503002/diff/560001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/route_controls/route_controls.js (right): https://codereview.chromium.org/2725503002/diff/560001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/route_controls/route_controls.js:206: */ On 2017/05/26 18:17:17, imcheng wrote: > Does adding @override here and in reset() work? Similar comment for other impl > classes. No, I get "property not defined on any superclass" error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, apacible@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2725503002/#ps580001 (title: "Add braces to @implements {Interface}")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 580001, "attempt_start_ts": 1496177615484280,
"parent_rev": "5620d9efdeb210b6d27b041783f173dae3e5e37b", "commit_rev":
"3f7923888b023461435906b4b1488a5a857cdc32"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The WebUI route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the WebUI controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots: https://docs.google.com/document/d/1v8jGV33FiFtH_qs2_Ui0UNubt1nHRN_CBrHLt1wRXF0 The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Custom Controls 4 - Implement details view WebUI This CL adds a built-in alternative to the extensionview controller in the route details view of the media router dialog WebUI. The WebUI route controller is functionally equivalent to the extensionview controller. Whenever the WebUI is notified by the browser process of updates to the media status of the shown route, the details view is updated. Whenever the user interacts with the media controls on the details view, the media commands are sent to the browser process. The switch to the WebUI controller is managed by a Finch experiment that is currently disabled, so this CL has no behavioral change. Its functionality can be checked out with the flag --enable-features=MediaRouterUIRouteController. Before-and-after screenshots: https://docs.google.com/document/d/1v8jGV33FiFtH_qs2_Ui0UNubt1nHRN_CBrHLt1wRXF0 The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: this patch Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2724513002/ BUG=684633 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2725503002 Cr-Commit-Position: refs/heads/master@{#475667} Committed: https://chromium.googlesource.com/chromium/src/+/3f7923888b023461435906b4b148... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:580001) as https://chromium.googlesource.com/chromium/src/+/3f7923888b023461435906b4b148... |
