Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1099723003:
Add Media Router media-router-sink-picker. (Closed)
DescriptionAdd Media Router media-router-sink-picker.
This custom polymer element shows a list of media-router-sink elements. It is styled after the current cast extension's sinks.
BUG=464222
Committed: https://crrev.com/3f6c899c85e7849970246fc8bf103b5f8f2a421a
Cr-Commit-Position: refs/heads/master@{#326917}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 15
Patch Set 5 : #
Messages
Total messages: 22 (7 generated)
apacible@chromium.org changed reviewers: + jhawkins@chromium.org, jlklein@chromium.org, mfoltz@chromium.org, michaelpg@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
PTAL, thanks!
https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:49: * Rebuilds sinkMap. Can we just name this rebuildSinkMap? When or how it's called is mostly irrelevant. https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:51: * @param {!Array<media_router.Sink>} oldValue The old value of sinkList. These params are unused, so remove them. https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:66: * Rebuilds routeMap and sinkToRouteMap. Same as above.
https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:49: * Rebuilds sinkMap. On 2015/04/22 18:02:31, James Hawkins wrote: > Can we just name this rebuildSinkMap? When or how it's called is mostly > irrelevant. I'm using a changed watcher here (and for routeListChanged). Is there a preference to do this differently? https://www.polymer-project.org/0.5/docs/polymer/polymer.html#observeprops https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:51: * @param {!Array<media_router.Sink>} oldValue The old value of sinkList. On 2015/04/22 18:02:31, James Hawkins wrote: > These params are unused, so remove them. Done. https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:66: * Rebuilds routeMap and sinkToRouteMap. On 2015/04/22 18:02:31, James Hawkins wrote: > Same as above. Done.
https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:49: * Rebuilds sinkMap. On 2015/04/22 18:13:55, apacible wrote: > On 2015/04/22 18:02:31, James Hawkins wrote: > > Can we just name this rebuildSinkMap? When or how it's called is mostly > > irrelevant. > > I'm using a changed watcher here (and for routeListChanged). Is there a > preference to do this differently? > > https://www.polymer-project.org/0.5/docs/polymer/polymer.html#observeprops How you name the method is not related to the mechanism in which the method is called. Well-named methods should describe that they do, not how they're called.
Patchset #3 (id:70001) has been deleted
https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/10004/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:49: * Rebuilds sinkMap. On 2015/04/22 18:16:35, James Hawkins wrote: > On 2015/04/22 18:13:55, apacible wrote: > > On 2015/04/22 18:02:31, James Hawkins wrote: > > > Can we just name this rebuildSinkMap? When or how it's called is mostly > > > irrelevant. > > > > I'm using a changed watcher here (and for routeListChanged). Is there a > > preference to do this differently? > > > > https://www.polymer-project.org/0.5/docs/polymer/polymer.html#observeprops > > How you name the method is not related to the mechanism in which the method is > called. Well-named methods should describe that they do, not how they're > called. Got it. I moved all the map rebuilding functionality into separate functions and called them rebuildSinkMap and rebuildRouteMaps.
https://codereview.chromium.org/1099723003/diff/90001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/90001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:78: routeListChanged: function(oldValue, newValue) { Oh, so is this an @override or something? I was under the impression this was set up as an event handler somewhere.
https://codereview.chromium.org/1099723003/diff/90001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/90001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:78: routeListChanged: function(oldValue, newValue) { On 2015/04/22 20:50:58, James Hawkins wrote: > Oh, so is this an @override or something? I was under the impression this was > set up as an event handler somewhere. This implements the propertynameChanged handler. I can set it up as a custom property observer, actually. That does away with the propertnameChanged functions and leaves us with the rebuild map functions.
Patchset #4 (id:110001) has been deleted
https://codereview.chromium.org/1099723003/diff/90001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/90001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:78: routeListChanged: function(oldValue, newValue) { On 2015/04/22 21:04:23, apacible wrote: > On 2015/04/22 20:50:58, James Hawkins wrote: > > Oh, so is this an @override or something? I was under the impression this was > > set up as an event handler somewhere. > > This implements the propertynameChanged handler. I can set it up as a custom > property observer, actually. That does away with the propertnameChanged > functions and leaves us with the rebuild map functions. Removed propertynameChanged handlers, added custom property observers.
lgtm
https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:12: * @type {!Array<media_router.Route>} !media_router.Route here and below https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:21: * @type {!Array<media_router.Sink>} !media_router.Sink here and below https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:75: this.sinkList.forEach(function(e) { function(sink) for consistency with rebuildRouteMaps(). https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:76: this.sinkMap[e.id] = e; What if the new list of sinks is different from the keys in this.sinkToRouteMap? Do you need to rebuild sTRM as well? https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:83: * @param {media_router.Route} route The route to add. !media_router.Route https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:92: // sinkToRouteMap entry will be overwritten with that of the new route, How is this done? https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:100: * @param {string} sinkId The id of the media_router.Sink. Document @return
https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:12: * @type {!Array<media_router.Route>} On 2015/04/23 18:02:41, mark a. foltz wrote: > !media_router.Route here and below Done. https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:21: * @type {!Array<media_router.Sink>} On 2015/04/23 18:02:40, mark a. foltz wrote: > !media_router.Sink here and below Done. https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:75: this.sinkList.forEach(function(e) { On 2015/04/23 18:02:40, mark a. foltz wrote: > function(sink) for consistency with rebuildRouteMaps(). Done. https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:76: this.sinkMap[e.id] = e; On 2015/04/23 18:02:41, mark a. foltz wrote: > What if the new list of sinks is different from the keys in this.sinkToRouteMap? > Do you need to rebuild sTRM as well? Sinks keep the same ID. Any routes that are mapped with sinks that are not discovered anymore won't appear in the UI. If new sinks have an existing route, routeList will be updated and sTRM and rM will be rebuilt. https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:83: * @param {media_router.Route} route The route to add. On 2015/04/23 18:02:40, mark a. foltz wrote: > !media_router.Route Done. https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:92: // sinkToRouteMap entry will be overwritten with that of the new route, On 2015/04/23 18:02:40, mark a. foltz wrote: > How is this done? "push" adds the route to the end of the routeList. When we rebuild sinkToRouteMap, that means |route| is at the end of the routeList (looped from first to last). Say a previous route has the same sinkId as |route|. That previous route is added to sinkToRouteMap first (L63), which makes a mapping between sinkId->previousRouteID. When the loop reaches |route|, it overwrites the previous mapping to become sinkId->|route|.ID. https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:100: * @param {string} sinkId The id of the media_router.Sink. On 2015/04/23 18:02:40, mark a. foltz wrote: > Document @return Done.
lgtm https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js (right): https://codereview.chromium.org/1099723003/diff/130001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_sink_picker/media_router_sink_picker.js:92: // sinkToRouteMap entry will be overwritten with that of the new route, On 2015/04/23 18:30:37, apacible wrote: > On 2015/04/23 18:02:40, mark a. foltz wrote: > > How is this done? > > "push" adds the route to the end of the routeList. When we rebuild > sinkToRouteMap, that means |route| is at the end of the routeList (looped from > first to last). > > Say a previous route has the same sinkId as |route|. That previous route is > added to sinkToRouteMap first (L63), which makes a mapping between > sinkId->previousRouteID. When the loop reaches |route|, it overwrites the > previous mapping to become sinkId->|route|.ID. Acknowledged.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jhawkins@chromium.org Link to the patchset: https://codereview.chromium.org/1099723003/#ps150001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099723003/150001
Message was sent while issue was closed.
Committed patchset #5 (id:150001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3f6c899c85e7849970246fc8bf103b5f8f2a421a Cr-Commit-Position: refs/heads/master@{#326917} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
