Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1026083003:
Add Media Router cast-mode-picker. (Closed)
DescriptionAdd Media Router cast-mode-picker.
This is a drop down that shows cast modes. It is styled after the current cast extension's cast mode picker. Some styling (e.g. positioning) will be updated when the rest of the WebUI has landed.
The default selectedCastMode is denoted with a -1. This refers to the default cast mode for the current tab. The possible cast mode values are defined elsewhere (to be landed later).
BUG=464222
Committed: https://crrev.com/34b2daa21b14019e961230b8beb59566512c81e2
Cr-Commit-Position: refs/heads/master@{#322650}
Patch Set 1 : #
Total comments: 26
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 26 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
PTAL, thanks! +mfoltz: media router OWNER +haibinlu: media router WebUI +michaelpg, jlklein: polymer +jhawkins: browser_resources.grd OWNER
https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:7: padding-left: 20px; RTL https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:17: background-color: #FFF; white https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:21: font-size: 12px; You need to use em for the font size so it scales according to user's font size preference. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:23: left: 82px; RTL https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:30: z-index: 3; Why is this z-index necessary?
https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html:1: <polymer-element name="cast-mode-picker" attributes="castModeList We've elected to use "publish" blocks in the js rather than attributes here because it will make for an easier transition to 0.8 and provide more type info to the compiler once we have a compiler pass for polymer. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html:8: <option class="cast-mode" on-click="{{onCastModeSelected}}" nit: Any reason not to use the change event from the select? https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:7: /** @type {!Array.<media_router.CastMode>} */ Should this be "!media_router.CastMode" or can entries in this array be null? https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:10: /** @type {!Object.<number, media_router.CastMode>} */ Same question. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:10: /** @type {!Object.<number, media_router.CastMode>} */ Can you include a brief description for what this mapping includes?
Looks good modulo jhawkins' comments. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:7: /** @type {!Array.<media_router.CastMode>} */ nit: omit . in Array.<> and Object.<> in new code
One comment. Will look again when other comments are addressed. Do we have a unit testing framework for individual polymer elements (or would it make sense to test larger functional units)? https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:14: this.selectedCastMode = -1; Please add a comment explaining the special value -1
On 2015/03/26 22:16:49, mark a. foltz wrote: > One comment. Will look again when other comments are addressed. > > Do we have a unit testing framework for individual polymer elements (or would it > make sense to test larger functional units)? Yes, I'm working on the unit testing framework right now -- it's basically a subset of web-component-tester (https://github.com/Polymer/web-component-tester). I can't speak for Media Router, but Chrome Settings will focus on comprehensive unit tests, using functional/end-to-end tests only where we really need to. > > https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... > File > chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js > (right): > > https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... > chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:14: > this.selectedCastMode = -1; > Please add a comment explaining the special value -1
https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:7: padding-left: 20px; On 2015/03/26 18:17:24, James Hawkins wrote: > RTL Removed for now; discussing i18n with UX. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:17: background-color: #FFF; On 2015/03/26 18:17:23, James Hawkins wrote: > white Done. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:21: font-size: 12px; On 2015/03/26 18:17:23, James Hawkins wrote: > You need to use em for the font size so it scales according to user's font size > preference. Done. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:23: left: 82px; On 2015/03/26 18:17:24, James Hawkins wrote: > RTL Removed for now; discussing i18n with UX. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:30: z-index: 3; On 2015/03/26 18:17:24, James Hawkins wrote: > Why is this z-index necessary? This will appear over other elements when the list is expanded. This ensures the cast mode list isn't hidden under something. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html:1: <polymer-element name="cast-mode-picker" attributes="castModeList On 2015/03/26 18:50:51, Jeremy Klein wrote: > We've elected to use "publish" blocks in the js rather than attributes here > because it will make for an easier transition to 0.8 and provide more type info > to the compiler once we have a compiler pass for polymer. I see! Changed to publish block. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html:8: <option class="cast-mode" on-click="{{onCastModeSelected}}" On 2015/03/26 18:50:51, Jeremy Klein wrote: > nit: Any reason not to use the change event from the select? Talked offline. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:7: /** @type {!Array.<media_router.CastMode>} */ On 2015/03/26 18:50:51, Jeremy Klein wrote: > Should this be "!media_router.CastMode" or can entries in this array be null? Fixed. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:7: /** @type {!Array.<media_router.CastMode>} */ On 2015/03/26 20:08:23, michaelpg wrote: > nit: omit . in Array.<> and Object.<> in new code Done. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:10: /** @type {!Object.<number, media_router.CastMode>} */ On 2015/03/26 18:50:51, Jeremy Klein wrote: > Same question. Fixed. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:10: /** @type {!Object.<number, media_router.CastMode>} */ On 2015/03/26 18:50:51, Jeremy Klein wrote: > Can you include a brief description for what this mapping includes? Done. https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:14: this.selectedCastMode = -1; On 2015/03/26 22:16:49, mark a. foltz wrote: > Please add a comment explaining the special value -1 Done.
https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:30: z-index: 3; On 2015/03/27 03:50:55, apacible wrote: > On 2015/03/26 18:17:24, James Hawkins wrote: > > Why is this z-index necessary? > > This will appear over other elements when the list is expanded. This ensures the > cast mode list isn't hidden under something. How was this particular value (3) picked? https://codereview.chromium.org/1026083003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/100001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:10: background-color: rgb(187,206,236); nit: Spaces after commas. Here and line 18.
https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:30: z-index: 3; On 2015/03/27 16:27:53, James Hawkins wrote: > On 2015/03/27 03:50:55, apacible wrote: > > On 2015/03/26 18:17:24, James Hawkins wrote: > > > Why is this z-index necessary? > > > > This will appear over other elements when the list is expanded. This ensures > the > > cast mode list isn't hidden under something. > > How was this particular value (3) picked? Talked offline; will re-add z-index in a future patch if needed when all the components are put together to make it easier to understand where/when the z-index properties are needed. https://codereview.chromium.org/1026083003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/100001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:10: background-color: rgb(187,206,236); On 2015/03/27 16:27:53, James Hawkins wrote: > nit: Spaces after commas. Here and line 18. Done.
lgtm https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html (right): https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html:1: <polymer-element name="cast-mode-picker"> This file should probably import polymer.html https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:14: castModeList: [], If you want to be really safe, you should set defaults for these in the created callback to ensure that a separate Array is created for each instance of this element. Check out: https://www.polymer-project.org/0.5/docs/polymer/polymer.html#default-propert...
LGTM with nit. https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:17: border: 1px solid #A5A5A5; nit: Be consistent with casing of #hex values, i.e., either lower-case this line or upper-case line 19.
https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css (right): https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.css:17: border: 1px solid #A5A5A5; On 2015/03/27 17:52:41, James Hawkins wrote: > nit: Be consistent with casing of #hex values, i.e., either lower-case this line > or upper-case line 19. Done. https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html (right): https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.html:1: <polymer-element name="cast-mode-picker"> On 2015/03/27 17:38:57, Jeremy Klein wrote: > This file should probably import polymer.html Right, we're switching from importing in js to html. Done. https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:14: castModeList: [], On 2015/03/27 17:38:57, Jeremy Klein wrote: > If you want to be really safe, you should set defaults for these in the created > callback to ensure that a separate Array is created for each instance of this > element. Check out: > > https://www.polymer-project.org/0.5/docs/polymer/polymer.html#default-propert... Done.
https://codereview.chromium.org/1026083003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:34: this.selectedCastMode = -1; Double initialization (with a different type than declared above)? https://codereview.chromium.org/1026083003/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:47: headerText: this.castModeMap[sender.value].title What if this is called before castModeListChanged? this.castModeMap will be undefined. Initialize this.castModeMap in created()?
https://codereview.chromium.org/1026083003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js (right): https://codereview.chromium.org/1026083003/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:34: this.selectedCastMode = -1; On 2015/03/27 18:40:46, mark a. foltz wrote: > Double initialization (with a different type than declared above)? Renaming error, fixed. https://codereview.chromium.org/1026083003/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/cast_mode_picker/cast_mode_picker.js:47: headerText: this.castModeMap[sender.value].title On 2015/03/27 18:40:46, mark a. foltz wrote: > What if this is called before castModeListChanged? this.castModeMap will be > undefined. > > Initialize this.castModeMap in created()? Fixed.
lgtm
lgtm
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org, jhawkins@chromium.org Link to the patchset: https://codereview.chromium.org/1026083003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1026083003/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/34b2daa21b14019e961230b8beb59566512c81e2 Cr-Commit-Position: refs/heads/master@{#322650} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
