Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1029213002:
Add Media Router JS structs. (Closed)
DescriptionAdd Media Router JS structs.
These will be used in the Media Router WebUI custom Polymer elements, which will be submitted in future patches.
BUG=464222
Committed: https://crrev.com/3f271cee90a286c264d3aaa044f10f5d70d6884a
Cr-Commit-Position: refs/heads/master@{#322173}
Patch Set 1 : #
Total comments: 40
Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Messages
Total messages: 27 (9 generated)
apacible@chromium.org changed reviewers: + jhawkins@chromium.org, mfoltz@chromium.org
PTAL, thanks!
Patchset #1 (id:1) has been deleted
apacible@chromium.org changed reviewers: + haibinlu@chromium.org
On 2015/03/23 21:50:41, apacible wrote: > PTAL, thanks! +haibinlu
LGTM with nits. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:26: /** nit: Double blank lines between top-level JSDoc blocks. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:42: defaultActionType, optActionText, optActionType, nit: opt_actionText, opt_actionType https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:60: this.optActionText = optActionText; nit: Only the parameter should have 'opt' in the name. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:131: var SinkStatus = { Consider moving SinkStatus higher up (at least higher than where it's first used. Grouping definitions by type would have this go at the very top, but it's up to you.
Mostly requests for clarification in comments. Also I want to make sure the localization is thought through for data passed through these structs. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:9: * @param {number} castMode The type of cast mode. Where are legal values defined? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:10: * @param {string} title The title of the cast mode. Is the title and description for internal use, or are they shown to the user? If the latter, how would the WebUI page map them to a localized resource? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:27: * @param {string} id The ID of this issue. What are legal values? Are these defined somewhere? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:28: * @param {string} title The issue title. Similar questions about localization for all the strings passed in here. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:30: * @param {string} defaultActionText The button text of default action. What if the text overflows the dimensions of the button? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:31: * @param {number} defaultActionType The type of default action. Where are the types defined? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:33: * @param {?number} optActionType The type of optional action. Where are the types defined? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:34: * @param {?string} routeId The route ID to which this issue mediaRouteId https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:35: * pertains. If not set, this is a global issue. What if the issue occurred before or after the route was created? Should this be a screenId instead? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:76: * @param {string} id The route ID. The media route ID https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:77: * @param {string} sinkId The ID of the sink running this route. "the media sink" https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:78: * @param {?string} iconUrl The icon URL of this route. This should just point to a bundled resource, correct? Can this just be an enum that references one of those instead of an arbitrary URL? https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:82: * @param {boolean} isLocal True if this is a locally launched route. s/launched/created/ https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:107: * @param {string} id The ID of the sink. "the media sink" https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:140: * @param {boolean} hasCastApp True if tab has a current Cast App. When is this set to true? Does it mean to say "an active media route"?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:9: * @param {number} castMode The type of cast mode. On 2015/03/24 18:09:16, mark a. foltz wrote: > Where are legal values defined? RE: all legal values questions -- These are all defined in Media Router, but not all landed in Chromium yet. Some are also defined in the component extension (depending on whether we pass them from the CE to MR). https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:10: * @param {string} title The title of the cast mode. On 2015/03/24 18:09:17, mark a. foltz wrote: > Is the title and description for internal use, or are they shown to the user? > If the latter, how would the WebUI page map them to a localized resource? > The description is shown. RE: all localization questions -- All of the strings are localized in Media Router before being passed to the WebUI. No localization will be done from the WebUI side. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:26: /** On 2015/03/24 17:48:06, James Hawkins wrote: > nit: Double blank lines between top-level JSDoc blocks. Done. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:27: * @param {string} id The ID of this issue. On 2015/03/24 18:09:15, mark a. foltz wrote: > What are legal values? Are these defined somewhere? Issue IDs are created in Media Router using GenerateGUID. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:28: * @param {string} title The issue title. On 2015/03/24 18:09:16, mark a. foltz wrote: > Similar questions about localization for all the strings passed in here. Acknowledged. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:30: * @param {string} defaultActionText The button text of default action. On 2015/03/24 18:09:16, mark a. foltz wrote: > What if the text overflows the dimensions of the button? We'll need to talk to UX about this (and all other potentially overflowing text). Since we're looking into some proposed redesigns right now, this will be something I'll bring up with Jonny. Our final designs should take this into account. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:31: * @param {number} defaultActionType The type of default action. On 2015/03/24 18:09:16, mark a. foltz wrote: > Where are the types defined? Acknowledged. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:33: * @param {?number} optActionType The type of optional action. On 2015/03/24 18:09:16, mark a. foltz wrote: > Where are the types defined? Acknowledged. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:34: * @param {?string} routeId The route ID to which this issue On 2015/03/24 18:09:17, mark a. foltz wrote: > mediaRouteId Done. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:35: * pertains. If not set, this is a global issue. On 2015/03/24 18:09:16, mark a. foltz wrote: > What if the issue occurred before or after the route was created? > Should this be a screenId instead? We currently don't have screen-specific issues. If the issue occurs before any route was created, it would be a global issue. If an issue occurs after (or as) a route is created, it could be either route-specific or global. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:42: defaultActionType, optActionText, optActionType, On 2015/03/24 17:48:06, James Hawkins wrote: > nit: opt_actionText, opt_actionType Ack, question in next comment before I change this. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:60: this.optActionText = optActionText; On 2015/03/24 17:48:06, James Hawkins wrote: > nit: Only the parameter should have 'opt' in the name. Why is 'opt' restricted to the parameter? Should I rename this to 'secondaryActionText'? There's a 'default' action text and an 'optional/secondary' action text. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:76: * @param {string} id The route ID. On 2015/03/24 18:09:15, mark a. foltz wrote: > The media route ID Done. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:77: * @param {string} sinkId The ID of the sink running this route. On 2015/03/24 18:09:15, mark a. foltz wrote: > "the media sink" Done. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:78: * @param {?string} iconUrl The icon URL of this route. On 2015/03/24 18:09:16, mark a. foltz wrote: > This should just point to a bundled resource, correct? Can this just be an enum > that references one of those instead of an arbitrary URL? This is the favicon of whatever we're casting. Talked to haibinlu@ -- I'm removing this for now since we're not 100% sure what our UX for mirroring will be. Bringing this up with Jonny as well. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:82: * @param {boolean} isLocal True if this is a locally launched route. On 2015/03/24 18:09:16, mark a. foltz wrote: > s/launched/created/ Done. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:107: * @param {string} id The ID of the sink. On 2015/03/24 18:09:17, mark a. foltz wrote: > "the media sink" Done. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:131: var SinkStatus = { On 2015/03/24 17:48:06, James Hawkins wrote: > Consider moving SinkStatus higher up (at least higher than where it's first > used. Grouping definitions by type would have this go at the very top, but it's > up to you. Moved to top. https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:140: * @param {boolean} hasCastApp True if tab has a current Cast App. On 2015/03/24 18:09:16, mark a. foltz wrote: > When is this set to true? Does it mean to say "an active media route"? It's when the tab has a current cast app (e.g. YouTube) so we know which cast modes to show. Talked to imcheng@/haibinlu@ -- removing this because we don't need this anymore.
https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:60: this.optActionText = optActionText; On 2015/03/24 20:46:26, apacible wrote: > On 2015/03/24 17:48:06, James Hawkins wrote: > > nit: Only the parameter should have 'opt' in the name. > > Why is 'opt' restricted to the parameter? > > Should I rename this to 'secondaryActionText'? There's a 'default' action text > and an 'optional/secondary' action text. Because the member variable itself is not optional; it exists for each object instance. It may be null, but that is documented by its type. secondaryActionText seems fine to me, but that's your call.
https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_data.js:60: this.optActionText = optActionText; On 2015/03/24 21:27:56, James Hawkins wrote: > On 2015/03/24 20:46:26, apacible wrote: > > On 2015/03/24 17:48:06, James Hawkins wrote: > > > nit: Only the parameter should have 'opt' in the name. > > > > Why is 'opt' restricted to the parameter? > > > > Should I rename this to 'secondaryActionText'? There's a 'default' action text > > and an 'optional/secondary' action text. > > Because the member variable itself is not optional; it exists for each object > instance. It may be null, but that is documented by its type. > > secondaryActionText seems fine to me, but that's your call. Thanks! Also, should I prepend all optional parameters with opt_*, or is that only the convention if 'opt' is used somewhere in the parameter? (e.g. using @param secondaryActionText and then this.secondaryActionText is fine)
On 2015/03/24 21:58:11, apacible wrote: > https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/media_router/media_router_data.js (right): > > https://codereview.chromium.org/1029213002/diff/20001/chrome/browser/resource... > chrome/browser/resources/media_router/media_router_data.js:60: > this.optActionText = optActionText; > On 2015/03/24 21:27:56, James Hawkins wrote: > > On 2015/03/24 20:46:26, apacible wrote: > > > On 2015/03/24 17:48:06, James Hawkins wrote: > > > > nit: Only the parameter should have 'opt' in the name. > > > > > > Why is 'opt' restricted to the parameter? > > > > > > Should I rename this to 'secondaryActionText'? There's a 'default' action > text > > > and an 'optional/secondary' action text. > > > > Because the member variable itself is not optional; it exists for each object > > instance. It may be null, but that is documented by its type. > > > > secondaryActionText seems fine to me, but that's your call. > > Thanks! Also, should I prepend all optional parameters with opt_*, or is that > only the convention if 'opt' is used somewhere in the parameter? (e.g. using > @param secondaryActionText and then this.secondaryActionText is fine) Optional parameters must be prepended with opt_.
lgtm Please add a file level comment explaining: (1) Strings shown to the user are already localized (2) Values for castMode, issueId will be defined in other places to be determined later.
Patchset #3 (id:80001) has been deleted
Added file level comment. Prepended all optional parameters as opt_*. Renamed optAction* fields as secondaryAction*.
https://codereview.chromium.org/1029213002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_data.js:45: * @param {?string} opt_secondaryActionText The button text of optional rename to secondaryActionText? per closure annotation, if this is an optional param, it should {string=}, which is string or undefined. https://codereview.chromium.org/1029213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_data.js:52: * clicked. ditto
lgtm
https://codereview.chromium.org/1029213002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_data.js (right): https://codereview.chromium.org/1029213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_data.js:45: * @param {?string} opt_secondaryActionText The button text of optional On 2015/03/25 00:22:58, haibinlu wrote: > rename to secondaryActionText? > > per closure annotation, if this is an optional param, it should {string=}, which > is string or undefined. Talked offline with haibinlu@ -- for this case, we don't want to use the strict definition of "optional" parameters, so we want to keep the names of these parameters the same as before. "optAction*" will be "secondaryAction*". https://codereview.chromium.org/1029213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_data.js:52: * clicked. On 2015/03/25 00:22:58, haibinlu wrote: > ditto Done.
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, mfoltz@chromium.org, haibinlu@chromium.org Link to the patchset: https://codereview.chromium.org/1029213002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1029213002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by apacible@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1029213002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3f271cee90a286c264d3aaa044f10f5d70d6884a Cr-Commit-Position: refs/heads/master@{#322173} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
