Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162243002:
Add Media Router JS Gin module. (Closed)
|
Created:
5 years, 6 months ago by Kevin M Modified:
3 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Media Router JS Gin module.
This module bridges calls made by the extension to the Media Router Mojo
service.
Calls made from the media router into the extension are performed via
observer callbacks.
Add Media Router module to renderer resources.
BUG=464205
R=haibinlu
Committed: https://crrev.com/9662e2fce73725ce001f0143a9124d5339a42916
Cr-Commit-Position: refs/heads/master@{#333534}
Patch Set 1 #Patch Set 2 : Add preprocessor guards #
Total comments: 46
Patch Set 3 : Code review feedback. #
Total comments: 10
Patch Set 4 : Code review feedback addressed. #
Total comments: 8
Patch Set 5 : Code review feedback. #
Total comments: 2
Patch Set 6 : Type fixes #Patch Set 7 : Remove unnecessary newline. #
Total comments: 25
Patch Set 8 : Addressed Devlin's feedback. #
Total comments: 8
Patch Set 9 : Feedback #Patch Set 10 : Incorporated mfoltz's improvements to comments #
Total comments: 3
Messages
Total messages: 38 (10 generated)
https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:23: add @param and @return https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:27: 'media_source': route.mediaSource, // nullable is 'nullable' necessary? since mojo def already documents it. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:30: 'name': opt_sinkName, this "new mediaRouterMojom.MediaSink" can use a util method below: MediaRouterObserver.sinkToMojo_ https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:33: 'icon_url': route.iconUrl, // nullable ditto https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:39: * @constructor @param https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:43: * The Mojo service proxy. Allows extension code to call methods that reside document type for each member. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:81: * @return {Promise<string>} instanceId !Promise or Promsie? https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:81: * @return {Promise<string>} instanceId explain instanceId https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:84: // Register the MRPM with the MR service by connecting it to one of the can this comment be part of the method doc? https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:95: * @param {Object} receiver 'receiver' to 'sink'. we should use the naming consistently. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:108: * @type {Object} what is @type? is the 'handlers' nullable? https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:128: * @param {Array<Object>} sinks Array or !Array? https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:158: * Sends a message to an active mediaRoute. media route https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:161: * @param {Object} message @param {!Object|string} message A message that can be converted to JSON string. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:171: * @param {Object} issue !Object https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:219: @param https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:223: console.log('sinks: ' + JSON.stringify(sinks)); do we always want to log sinks? https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:228: // Convert mr.Routes to Mojo routes, and adding their sink names the type, mr.Routes is not in Chromium. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:245: * @param {string} requestId document the requestId https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:272: * @type {function(!string, !string, !string, !number)} use '!string' or 'string' consistently https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:317: * @type {MediaRouterHandlers} !MediaRouterHandlers https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:323: */ @type https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:335: this.handlers_ = handlers; can you check all the required methods here instead of in each method below? https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:375: * @return {!Promise.<!Object>} document that this will always resolve and never reject. By the way, why not use reject for error cases?
marshallk@google.com changed reviewers: + marshallk@google.com
https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:23: On 2015/06/01 18:36:33, haibinlu wrote: > add @param and @return Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:27: 'media_source': route.mediaSource, // nullable On 2015/06/01 18:36:33, haibinlu wrote: > is 'nullable' necessary? since mojo def already documents it. Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:30: 'name': opt_sinkName, On 2015/06/01 18:36:33, haibinlu wrote: > this "new mediaRouterMojom.MediaSink" can use a util method below: > MediaRouterObserver.sinkToMojo_ Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:33: 'icon_url': route.iconUrl, // nullable On 2015/06/01 18:36:32, haibinlu wrote: > ditto Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:39: * @constructor On 2015/06/01 18:36:33, haibinlu wrote: > @param Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:43: * The Mojo service proxy. Allows extension code to call methods that reside On 2015/06/01 18:36:33, haibinlu wrote: > document type for each member. Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:81: * @return {Promise<string>} instanceId On 2015/06/01 18:36:32, haibinlu wrote: > !Promise or Promsie? Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:81: * @return {Promise<string>} instanceId On 2015/06/01 18:36:33, haibinlu wrote: > explain instanceId Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:84: // Register the MRPM with the MR service by connecting it to one of the On 2015/06/01 18:36:33, haibinlu wrote: > can this comment be part of the method doc? Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:95: * @param {Object} receiver On 2015/06/01 18:36:33, haibinlu wrote: > 'receiver' to 'sink'. we should use the naming consistently. Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:108: * @type {Object} On 2015/06/01 18:36:34, haibinlu wrote: > what is @type? > is the 'handlers' nullable? It is nullable. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:128: * @param {Array<Object>} sinks On 2015/06/01 18:36:32, haibinlu wrote: > Array or !Array? Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:158: * Sends a message to an active mediaRoute. On 2015/06/01 18:36:33, haibinlu wrote: > media route Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:161: * @param {Object} message On 2015/06/01 18:36:32, haibinlu wrote: > @param {!Object|string} message A message that can be converted to JSON string. Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:171: * @param {Object} issue On 2015/06/01 18:36:32, haibinlu wrote: > !Object Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:219: On 2015/06/01 18:36:33, haibinlu wrote: > @param Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:223: console.log('sinks: ' + JSON.stringify(sinks)); On 2015/06/01 18:36:33, haibinlu wrote: > do we always want to log sinks? We don't, thanks. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:228: // Convert mr.Routes to Mojo routes, and adding their sink names On 2015/06/01 18:36:33, haibinlu wrote: > the type, mr.Routes is not in Chromium. done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:245: * @param {string} requestId On 2015/06/01 18:36:33, haibinlu wrote: > document the requestId Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:272: * @type {function(!string, !string, !string, !number)} On 2015/06/01 18:36:33, haibinlu wrote: > use '!string' or 'string' consistently Done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:335: this.handlers_ = handlers; On 2015/06/01 18:36:33, haibinlu wrote: > can you check all the required methods here instead of in each method below? Good idea, done. https://codereview.chromium.org/1162243002/diff/20001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:375: * @return {!Promise.<!Object>} On 2015/06/01 18:36:32, haibinlu wrote: > document that this will always resolve and never reject. > > By the way, why not use reject for error cases? I don't think it's worth commenting, since this won't be called by anything but Mojo. Mojo doesn't propagate Promise rejections across the IPC boundary.
https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:27: * @param {Object} sink !Object https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:28: * @return {mediaRouterMojom.MediaSink} !mediaRouterMojom.MediaSink https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:41: * @param {MediaRoute} route !MediaRoute Please check nullable for the rest parameters https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:49: 'media_sink': new mediaRouterMojom.MediaSink({ sinkToMojo_ https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:374: return Promise.resolve({error_text: 'joinRoute handler not registered.'}); this method has no return value.
https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:27: * @param {Object} sink On 2015/06/02 17:54:15, haibinlu wrote: > !Object Done. https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:28: * @return {mediaRouterMojom.MediaSink} On 2015/06/02 17:54:15, haibinlu wrote: > !mediaRouterMojom.MediaSink Done. https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:41: * @param {MediaRoute} route On 2015/06/02 17:54:16, haibinlu wrote: > !MediaRoute > > Please check nullable for the rest parameters Done. https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:49: 'media_sink': new mediaRouterMojom.MediaSink({ On 2015/06/02 17:54:15, haibinlu wrote: > sinkToMojo_ sinkToMojo_ doesn't work here, since we're going from a sink ID (string), not a sink (Object). https://codereview.chromium.org/1162243002/diff/40001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:374: return Promise.resolve({error_text: 'joinRoute handler not registered.'}); On 2015/06/02 17:54:15, haibinlu wrote: > this method has no return value. Done.
https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:42: * @param {string} opt_sinkName !string= https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:60: * @param {MediaRouterService} service nullable? https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:74: * @type {MediaRouter} !MediaRouter https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:82: * prevent the connection from closing automatically. nullable? If so, ?mojo.MessagePipe
https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:42: * @param {string} opt_sinkName On 2015/06/02 19:44:05, haibinlu wrote: > !string= Done. https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:60: * @param {MediaRouterService} service On 2015/06/02 19:44:05, haibinlu wrote: > nullable? Not nullable. Done. https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:74: * @type {MediaRouter} On 2015/06/02 19:44:05, haibinlu wrote: > !MediaRouter Done. https://codereview.chromium.org/1162243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:82: * prevent the connection from closing automatically. On 2015/06/02 19:44:05, haibinlu wrote: > nullable? If so, ?mojo.MessagePipe Not nullable, this will always succeed.
lgtm https://codereview.chromium.org/1162243002/diff/80001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/80001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:67: * @type {MediaRouterService} !MediaRouterService
https://codereview.chromium.org/1162243002/diff/80001/extensions/renderer/res... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/80001/extensions/renderer/res... extensions/renderer/resources/media_router_bindings.js:67: * @type {MediaRouterService} On 2015/06/02 20:14:14, haibinlu wrote: > !MediaRouterService > Done.
kmarshall@chromium.org changed reviewers: + xhwang@chromium.org
+xiaohan Note that the type annotations are provided for documentation purposes; the code is not meant to be processed by the Closure compiler.
On 2015/06/02 20:29:18, Kevin M wrote: > +xiaohan > > Note that the type annotations are provided for documentation purposes; the code > is not meant to be processed by the Closure compiler. I have never done JS binding so I am afraid I can't provide any useful feedback on this CL. Actually I think I learned from this CL :)
kmarshall@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - xhwang@chromium.org
kmarshall@chromium.org changed reviewers: + finnur@chromium.org
+finnur, if Devlin isn't available to review. Thanks
I'm making my way through it. :) https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:94: // Define the stub used to bind this.mrpm_ to the Mojo interface. jsdoc-style comments https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:107: * Register the Media Router Provider Manager with the Media Router. nit: function comments should be descriptive, i.e. make this "Registers". https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:121: * @param {Object} handlers https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:168: I don't think we have the double-newline style.
gah, didn't actually mean to publish those drafts yet. oh well.
kmarshall@chromium.org changed reviewers: - finnur@chromium.org
-finnur Thanks Devlin. Sorry for inducing you to publish your drafts prematurely!
I'm not really familiar with most media router code - I can review this, but I probably can't catch much wrong that's media-router specific. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:69: this.observer_ = service; Wait, why does a MediaRouterObserver have an observer? https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:76: this.mrpm_ = new MediaRouter(this); Also quite odd for the MediaRouterObserver to own the MediaRouter, IMO... https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:172: * remove (and below) https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:238: * @param {!Array<MediaSink>} routes sinks? https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:243: for (var i = 0; i < sinks.length; i++) { nit: forEach() is surprisingly faster than for (var i...), and might make this a bit cleaner: sinks.forEach(function(sink) { sinkNameMap[sink.id] = sink.friendlyName; }); Same for the routes. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:345: this.mediaRouter_ = mediaRouterObserver; Why is |mediaRouter_| a mediaRouterObserver? And more importantly, where is it used? https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:357: if (!this.handlers_.stopObservingMediaRoutes) { var requiredHandlers = ['stopObservingMediaRouters', 'startObservingMediaRouters', 'postMessage', ...]; requiredHandlers.forEach(function(requiredHandler) { if (!handlers.hasOwnProperty(requiredHandler)) console.error('" + requiredHandler + '" handler not registered.'); }); https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:420: * @return {!Promise.<!Object>} Kind of a shame that this can't be a Promise<mojo.MediaRoute> - could we just reject the promise in the case of a route failing to create? (Ditto below).
https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:69: this.observer_ = service; On 2015/06/04 23:01:19, Devlin wrote: > Wait, why does a MediaRouterObserver have an observer? Done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:76: this.mrpm_ = new MediaRouter(this); On 2015/06/04 23:01:19, Devlin wrote: > Also quite odd for the MediaRouterObserver to own the MediaRouter, IMO... Agreed with the awkward names. We'll do a quality pass on the names, but as part of a followup patch - we can't exercise the code in integration and validate the changes until all the features have landed in Chromium. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:107: * Register the Media Router Provider Manager with the Media Router. On 2015/06/04 22:42:25, Devlin wrote: > nit: function comments should be descriptive, i.e. make this "Registers". Thanks, done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:121: * @param {Object} On 2015/06/04 22:42:26, Devlin wrote: > handlers Done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:168: On 2015/06/04 22:42:25, Devlin wrote: > I don't think we have the double-newline style. Done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:172: * On 2015/06/04 23:01:19, Devlin wrote: > remove (and below) Done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:238: * @param {!Array<MediaSink>} routes On 2015/06/04 23:01:19, Devlin wrote: > sinks? Done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:243: for (var i = 0; i < sinks.length; i++) { On 2015/06/04 23:01:19, Devlin wrote: > nit: forEach() is surprisingly faster than for (var i...), and might make this a > bit cleaner: > sinks.forEach(function(sink) { > sinkNameMap[sink.id] = sink.friendlyName; > }); > > Same for the routes. Is this still the case? I ran this benchmark suite https://jsperf.com/for-vs-foreach/37 and it looks like a simple index-based for loop is quite a bit faster (3.4k iterations for forEach vs. 13.5k with an index). If performance isn't an issue, then I think I'll stick with a conventional for-loop. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:345: this.mediaRouter_ = mediaRouterObserver; On 2015/06/04 23:01:19, Devlin wrote: > Why is |mediaRouter_| a mediaRouterObserver? And more importantly, where is it > used? A previous code reviewer thought it strange to have calls that mimic those in the MediaRouter interface (media_router.h) w/o it being called MediaRouter, so we changed the names. The compromise is not ideal. We'll fix that in a followup CL. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:357: if (!this.handlers_.stopObservingMediaRoutes) { On 2015/06/04 23:01:19, Devlin wrote: > var requiredHandlers = ['stopObservingMediaRouters', > 'startObservingMediaRouters', 'postMessage', ...]; > requiredHandlers.forEach(function(requiredHandler) { > if (!handlers.hasOwnProperty(requiredHandler)) > console.error('" + requiredHandler + '" handler not registered.'); > }); Done. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:420: * @return {!Promise.<!Object>} On 2015/06/04 23:01:19, Devlin wrote: > Kind of a shame that this can't be a Promise<mojo.MediaRoute> - could we just > reject the promise in the case of a route failing to create? (Ditto below). I agree, but Mojo's JS binding library doesn't have rejection handlers, so rejecting the promise yields an uncaught promise exception.
https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:243: for (var i = 0; i < sinks.length; i++) { On 2015/06/08 17:32:58, Kevin M wrote: > On 2015/06/04 23:01:19, Devlin wrote: > > nit: forEach() is surprisingly faster than for (var i...), and might make this > a > > bit cleaner: > > sinks.forEach(function(sink) { > > sinkNameMap[sink.id] = sink.friendlyName; > > }); > > > > Same for the routes. > > Is this still the case? I ran this benchmark suite > https://jsperf.com/for-vs-foreach/37 and it looks like a simple index-based for > loop is quite a bit faster (3.4k iterations for forEach vs. 13.5k with an > index). > > If performance isn't an issue, then I think I'll stick with a conventional > for-loop. Huh. Used to be the inverse. Also, it's important to note things like the fact that here, you're de-referencing twice, so it will do two separate lookups in the array. And, I tend to just think that (for a case like this), the forEach looks a little cleaner. But, none of these matter enough for me to block this CL on them. Up to you. https://codereview.chromium.org/1162243002/diff/120001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:420: * @return {!Promise.<!Object>} On 2015/06/08 17:32:59, Kevin M wrote: > On 2015/06/04 23:01:19, Devlin wrote: > > Kind of a shame that this can't be a Promise<mojo.MediaRoute> - could we just > > reject the promise in the case of a route failing to create? (Ditto below). > > I agree, but Mojo's JS binding library doesn't have rejection handlers, so > rejecting the promise yields an uncaught promise exception. Oh, bummer. https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:26: * @param {!Object} sink Can we {!MediaSink} this? https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:29: function sinkToMojo_(function(sink) { This line appears broken - "function(sink)" https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:136: * @param {!Array<!Object>} sinks here, too, MediaSink? https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:355: if (error) { why is this needed?
https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... File extensions/renderer/resources/media_router_bindings.js (right): https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:26: * @param {!Object} sink On 2015/06/08 19:51:42, Devlin wrote: > Can we {!MediaSink} this? Done. https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:29: function sinkToMojo_(function(sink) { On 2015/06/08 19:51:42, Devlin wrote: > This line appears broken - "function(sink)" Done. https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:136: * @param {!Array<!Object>} sinks On 2015/06/08 19:51:42, Devlin wrote: > here, too, MediaSink? Done. https://codereview.chromium.org/1162243002/diff/140001/extensions/renderer/re... extensions/renderer/resources/media_router_bindings.js:355: if (error) { On 2015/06/08 19:51:42, Devlin wrote: > why is this needed? To increase the LoC count. <_< Removed.
lgtm
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1162243002/#ps180001 (title: "Incorporated mfoltz's improvements to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162243002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9662e2fce73725ce001f0143a9124d5339a42916 Cr-Commit-Position: refs/heads/master@{#333534}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1162243002/diff/180001/extensions/renderer/re... File extensions/renderer/resources/extensions_renderer_resources.grd (right): https://codereview.chromium.org/1162243002/diff/180001/extensions/renderer/re... extensions/renderer/resources/extensions_renderer_resources.grd:99: <include name="IDR_MEDIA_ROUTER_MOJOM_JS" file="${mojom_root}\chrome\browser\media\router\media_router.mojom.js" use_base_dir="false" type="BINDATA" /> Is extensions/ the right place for this code? Everything else looking at ENABLE_MEDIA_ROUTER is in chrome/, and this here references chrome/browser from extensions which seems like a layering violation.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + rockot@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1162243002/diff/180001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1162243002/diff/180001/extensions/renderer/di... extensions/renderer/dispatcher.cc:596: std::make_pair("chrome/browser/media/router/media_router.mojom", This looks like a way worse dependency cycle, come to think of it, now that I see it.
Message was sent while issue was closed.
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1162243002/diff/180001/extensions/renderer/re... File extensions/renderer/resources/extensions_renderer_resources.grd (right): https://codereview.chromium.org/1162243002/diff/180001/extensions/renderer/re... extensions/renderer/resources/extensions_renderer_resources.grd:99: <include name="IDR_MEDIA_ROUTER_MOJOM_JS" file="${mojom_root}\chrome\browser\media\router\media_router.mojom.js" use_base_dir="false" type="BINDATA" /> On 2017/03/21 19:00:57, Nico wrote: > Is extensions/ the right place for this code? Everything else looking at > ENABLE_MEDIA_ROUTER is in chrome/, and this here references chrome/browser from > extensions which seems like a layering violation. Maybe it should have been. Though longer term we would like to rewrite MediaRouterAndroid to use the same mojom. Any ideas? Maybe components/media_router would be a good place? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
