Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
464133003:
CastExtensionHandler for Chromoting Webapp (Closed)
|
Created:
6 years, 4 months ago by aiguha Modified:
6 years, 4 months ago Reviewers:
Jamie CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionThe CastExtensionHandler handles interaction with the Cast Host Extension of
the chromoting host.
It only modifies webapp behavior if the "casting" capability is part of the
negotiated set between host and client.
It performs the following tasks:
1. Sends and receives extension messages to/from the host.
2. Initializes and uses the Google Cast Chrome Sender API library to interact
with nearby Cast Receivers, acting as a Sender App.
3. Acts as a message proxy between the Cast Host Extension and the Cast
Receiver, brokering their peer connection.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290140
Patch Set 1 #
Total comments: 1
Patch Set 2 : Minor Fixes #
Total comments: 84
Patch Set 3 : Changes based on review #Patch Set 4 : Passed JSCompile #Patch Set 5 : Flag to disable CastExtensionHandler #Patch Set 6 : minor fix #
Messages
Total messages: 7 (0 generated)
Based on the changes we discussed today, here's the CL containing the changes to the webapp for Cast Host Extension support. Please take a look. Thanks! https://codereview.chromium.org/464133003/diff/1/remoting/webapp/client_plugi... File remoting/webapp/client_plugin.js (right): https://codereview.chromium.org/464133003/diff/1/remoting/webapp/client_plugi... remoting/webapp/client_plugin.js:328: case 'cast_message': Should we also check here if the capability is enabled? Or is it enough that the empty function above will silently do nothing with the message? (theoretically cast-extension-messages would never be exchanged if it wasn't part of the final capabilities). https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... File remoting/webapp/cast_extension_handler.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:28: remoting.CastExtensionHandler = function(clientSession) { We discussed calling this CastExtensionMessageProxy earlier today, but it later occurred to me that that doesn't reflect the fact that it is responsible for initializing the Cast API and acting as a "Cast Sender App", which wasn't too well reflected in the name. What do you think? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:60: '### Multiple calls to CastExtensionHandler.start not expected.'); The Sender API turns on logging by default, and though useful, it is quite verbose. I was using the "###" to easily distinguish my own logs. If you prefer, I'll get rid of the ### and/or try to find a way to disable the logging. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:147: var applicationID = "8A1211E3"; This Application ID is registered against the custom cast receiver application I've built on the Google Cast SDK Dev Console. Until published, the receiver app cannot be started on any device except those registered against the ID in the console. Given that, I figured it would be safe to put this ID in the open. What do you think? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/manifest... File remoting/webapp/manifest.json.jinja2 (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/manifest... remoting/webapp/manifest.json.jinja2:40: "content_security_policy": "default-src 'self'; script-src 'self' {{ TALK_GADGET_HOST }} https://www.gstatic.com; style-src 'self' https://fonts.googleapis.com; img-src 'self' {{ TALK_GADGET_HOST }} data:; font-src *; connect-src 'self' {{ OAUTH2_ACCOUNTS_HOST }} {{ GOOGLE_API_HOSTS }} {{ TALK_GADGET_HOST }} https://relay.google.com", This is to allow us to use the Google Cast Sender API library.
Mostly looks good, module some style nits. Some of the requested changes are not trivial though, so I'll take another look. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... File remoting/webapp/cast_extension_handler.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:13: * creates a session loads our registered receiver application and then becomes Comma after "session" https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:28: remoting.CastExtensionHandler = function(clientSession) { On 2014/08/13 01:29:34, aiguha wrote: > We discussed calling this CastExtensionMessageProxy earlier today, but it later > occurred to me that that doesn't reflect the fact that it is responsible for > initializing the Cast API and acting as a "Cast Sender App", which wasn't too > well reflected in the name. What do you think? Acknowledged. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:29: this.clientSession_ = clientSession; Please add @private. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:32: * @private */ Nit: For multi-line doc comments, the format you've used for the method signature is correct (with the opening and closing comment characters on the own line). https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:33: this.session = null; Private members should have a trailing underscore. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:37: this.chromotingNamespace = 'urn:x-cast:com.chromoting.cast.all'; If this is a constant, we conventionally prefix the name with a "k". Also, we shouldn't really use the name Chromoting externally; since it's implicit by virtual of being a class member, I think you can call this kCastNamespace. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:39: /** @type {Array} Can you be more specific about the type? Is it Array.<string>, for example? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:43: this.start(); Generally, doing anything non-trivial in the ctor is a bad idea. If you must call start() from the ctor, then it should probably be a private method. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:60: '### Multiple calls to CastExtensionHandler.start not expected.'); On 2014/08/13 01:29:34, aiguha wrote: > The Sender API turns on logging by default, and though useful, it is quite > verbose. I was using the "###" to easily distinguish my own logs. > If you prefer, I'll get rid of the ### and/or try to find a way to disable the > logging. I think we should disable logs by default, especially if they can easily by turned on at runtime. I don't think the ### is necessary, since you're using console.error, which is red and easily filterable in the dev tools. For your other console logs, I would get rid of anything that's not an error, unless it will fire infrequently and you feel strongly that it aids debuggability. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:69: document.body.insertBefore(node, document.body.firstChild); I think this should happen after adding the event listeners. JS is single-threaded, so I don't think there's a race if you do it in this order, but it feels more natural. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:86: * Process cast-host-extension messages. I'm not sure how to parse "cast-host-extension messages", especially since it doesn't have hyphens on the next line. Do you mean "Cast->host extension messages" (ie, messages from Cast to the host)? Being consistent about capitalizing Cast can help with this sort of ambiguity. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:89: remoting.CastExtensionHandler.prototype.onMessage = function(msgString) { I think this and most of the subsequent methods are private. They should be annotated as such and have a trailing underscore in the name. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:90: var message = getJsonObjectFromString(msgString); This can throw an exception, which you're not catching. Normally, functions from typecheck.js are intended to be used inside a try...catch to at least log an error if a type violation occurs. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:122: this.sendMessageFailure.bind(this)); Nit: subsequent parameters should be either indented 4 characters relative to the previous line, or aligned with the first parameter. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:128: * Event handler for '__onGCastApiAvailable' window event. Can you provide a bit more context about this event? When is it fired, what does it signify, etc. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:147: var applicationID = "8A1211E3"; On 2014/08/13 01:29:34, aiguha wrote: > This Application ID is registered against the custom cast receiver application > I've built on the Google Cast SDK Dev Console. Until published, the receiver app > cannot be started on any device except those registered against the ID in the > console. Given that, I figured it would be safe to put this ID in the open. What > do you think? I think that's fine. If that was the reason for restricting this code review, I think you can unrestrict it. It feels like another constant with a similar purpose to the namespace, which you've declared in the ctor. I think it would be cleaner to declare this there as well. I realize it's only used in this one function, but logically I think it belongs with the namespace; WDYT? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:170: console.log("### Initialization Failed."); console.error https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:177: * @param {chrome.cast.Session} e The resulting session, non-nullable. I think we can probably come up with a more descriptive name for this parameter ;) Also, rather than commenting that it's non-nullable, you can express it as part of the type annotation. I haven't tended to bother with that elsewhere, but if the non-nullability concept is important, that's the place to declare it. You could also assert it at the start of the function. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:184: this.onMediaDiscovered('sessionListener', this.session.media[0]); Why are we only interested in the first session? I think an explanatory comment would be useful. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:190: this.chromotingMessageListener.bind(this)); Nit: Indentation https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:194: this.sendMessageFailure.bind(this)); Nit: Indentation https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:207: }; Do this function need to exist at all? I was assuming it was a callback from the Cast API that we're not interested in, but it seems you're calling it explicitly. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:230: * successfully. This is the same comment as for sendMessageSuccess; is that intentional? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:231: * @param {string} ns The namespace of the message received. You're not using the namespace. Shouldn't you be checking it against the expected value? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:257: * @param {chrome.cast.ReceiverAvailability} e Describes receiver availability. Parameter name. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:260: if( e === chrome.cast.ReceiverAvailability.AVAILABLE) { Space after 'if', not before 'e'. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:263: else { No newline before 'else'. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:271: * initialization is used. I don't really understand what this function does. You say that the SessionRequest passed during initialization is used by default, which suggests to me that this method should allow an alternative to be specified, but it doesn't. I think this comment needs to be more explicit. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:283: * Listener invoked when the chrome.cast.requestSession is successful. s/the// s/is successful/completes successfully/ https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_p... File remoting/webapp/client_plugin.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_p... remoting/webapp/client_plugin.js:328: case 'cast_message': For consistency with the other extension messages, please rename this to cast-message. Since that will presumably entail changing host code as well, it can be a follow-up CL. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... File remoting/webapp/client_session.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:380: CAST: 'casting' I think I would call this capability chromeCast (and CHROME_CAST), since "cast" is a pretty generic term. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1595: return; Braces for single-line conditionals, for consistency. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1600: * Process a remote cast extension message from the host. s/cast/Cast/, here and below https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1616: if(this.mode_ == remoting.ClientSession.Mode.ME2ME) { Nit: Space after 'if' https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1616: if(this.mode_ == remoting.ClientSession.Mode.ME2ME) { Why is this restricted to Me2Me? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/html/tem... File remoting/webapp/html/template_main.html (left): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/html/tem... remoting/webapp/html/template_main.html:18: Please reinstate this blank line :) https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/js_proto... File remoting/webapp/js_proto/chrome_proto.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/js_proto... remoting/webapp/js_proto/chrome_proto.js:366: chrome.cast.DefaultActionPolicy.CREATE_SESSION; AutoJoinPolicy, DefaultActionPolicy and ReceiverAvailability feel like enums. Are they actually complex types under-the-hood? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/manifest... File remoting/webapp/manifest.json.jinja2 (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/manifest... remoting/webapp/manifest.json.jinja2:40: "content_security_policy": "default-src 'self'; script-src 'self' {{ TALK_GADGET_HOST }} https://www.gstatic.com; style-src 'self' https://fonts.googleapis.com; img-src 'self' {{ TALK_GADGET_HOST }} data:; font-src *; connect-src 'self' {{ OAUTH2_ACCOUNTS_HOST }} {{ GOOGLE_API_HOSTS }} {{ TALK_GADGET_HOST }} https://relay.google.com", On 2014/08/13 01:29:34, aiguha wrote: > This is to allow us to use the Google Cast Sender API library. Does it work with apps v2?
PTAL. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... File remoting/webapp/cast_extension_handler.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:13: * creates a session loads our registered receiver application and then becomes On 2014/08/13 23:21:02, Jamie wrote: > Comma after "session" Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:29: this.clientSession_ = clientSession; On 2014/08/13 23:21:03, Jamie wrote: > Please add @private. Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:32: * @private */ On 2014/08/13 23:21:02, Jamie wrote: > Nit: For multi-line doc comments, the format you've used for the method > signature is correct (with the opening and closing comment characters on the own > line). Acknowledged. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:33: this.session = null; On 2014/08/13 23:21:03, Jamie wrote: > Private members should have a trailing underscore. Acknowledged. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:37: this.chromotingNamespace = 'urn:x-cast:com.chromoting.cast.all'; On 2014/08/13 23:21:03, Jamie wrote: > If this is a constant, we conventionally prefix the name with a "k". Also, we > shouldn't really use the name Chromoting externally; since it's implicit by > virtual of being a class member, I think you can call this kCastNamespace. Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:39: /** @type {Array} On 2014/08/13 23:21:03, Jamie wrote: > Can you be more specific about the type? Is it Array.<string>, for example? Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:43: this.start(); On 2014/08/13 23:21:02, Jamie wrote: > Generally, doing anything non-trivial in the ctor is a bad idea. If you must > call start() from the ctor, then it should probably be a private method. I think the call is unavoidable, because we need to load the script before doing anything else, and that happens in start(). It is now a private method though. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:60: '### Multiple calls to CastExtensionHandler.start not expected.'); On 2014/08/13 23:21:02, Jamie wrote: > On 2014/08/13 01:29:34, aiguha wrote: > > The Sender API turns on logging by default, and though useful, it is quite > > verbose. I was using the "###" to easily distinguish my own logs. > > If you prefer, I'll get rid of the ### and/or try to find a way to disable the > > logging. > > I think we should disable logs by default, especially if they can easily by > turned on at runtime. I don't think the ### is necessary, since you're using > console.error, which is red and easily filterable in the dev tools. > > For your other console logs, I would get rid of anything that's not an error, > unless it will fire infrequently and you feel strongly that it aids > debuggability. Sounds good, I've left in the logs that represent unexpected events/error situations. Looks like the chrome sender api has no way to disable logging, which makes me think that the logging is really coming from my Cast Canary Extension installed on my browser (i.e., the stable version won't log anything by default). https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:69: document.body.insertBefore(node, document.body.firstChild); On 2014/08/13 23:21:02, Jamie wrote: > I think this should happen after adding the event listeners. JS is > single-threaded, so I don't think there's a race if you do it in this order, but > it feels more natural. I was following wcs_loader.js's start() exactly here. Should I still change it? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:86: * Process cast-host-extension messages. On 2014/08/13 23:21:03, Jamie wrote: > I'm not sure how to parse "cast-host-extension messages", especially since it > doesn't have hyphens on the next line. Do you mean "Cast->host extension > messages" (ie, messages from Cast to the host)? Being consistent about > capitalizing Cast can help with this sort of ambiguity. Acknowledged. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:89: remoting.CastExtensionHandler.prototype.onMessage = function(msgString) { On 2014/08/13 23:21:03, Jamie wrote: > I think this and most of the subsequent methods are private. They should be > annotated as such and have a trailing underscore in the name. This one is actually called from ClientSession, but I will change rest, thanks! https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:90: var message = getJsonObjectFromString(msgString); On 2014/08/13 23:21:03, Jamie wrote: > This can throw an exception, which you're not catching. Normally, functions from > typecheck.js are intended to be used inside a try...catch to at least log an > error if a type violation occurs. Acknowledged. Caught above in client_session as is done with gnubby auth handler. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:122: this.sendMessageFailure.bind(this)); On 2014/08/13 23:21:02, Jamie wrote: > Nit: subsequent parameters should be either indented 4 characters relative to > the previous line, or aligned with the first parameter. Acknowledged. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:128: * Event handler for '__onGCastApiAvailable' window event. On 2014/08/13 23:21:04, Jamie wrote: > Can you provide a bit more context about this event? When is it fired, what does > it signify, etc. Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:147: var applicationID = "8A1211E3"; On 2014/08/13 23:21:03, Jamie wrote: > On 2014/08/13 01:29:34, aiguha wrote: > > This Application ID is registered against the custom cast receiver application > > I've built on the Google Cast SDK Dev Console. Until published, the receiver > app > > cannot be started on any device except those registered against the ID in the > > console. Given that, I figured it would be safe to put this ID in the open. > What > > do you think? > > I think that's fine. If that was the reason for restricting this code review, I > think you can unrestrict it. > > It feels like another constant with a similar purpose to the namespace, which > you've declared in the ctor. I think it would be cleaner to declare this there > as well. I realize it's only used in this one function, but logically I think it > belongs with the namespace; WDYT? I agree completely! I actually wanted to do that to when I took another look at it. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:170: console.log("### Initialization Failed."); On 2014/08/13 23:21:03, Jamie wrote: > console.error Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:177: * @param {chrome.cast.Session} e The resulting session, non-nullable. On 2014/08/13 23:21:02, Jamie wrote: > I think we can probably come up with a more descriptive name for this parameter > ;) > > Also, rather than commenting that it's non-nullable, you can express it as part > of the type annotation. I haven't tended to bother with that elsewhere, but if > the non-nullability concept is important, that's the place to declare it. You > could also assert it at the start of the function. I think I'll follow suit and not bother with it, because its actually a guarantee from the API. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:184: this.onMediaDiscovered('sessionListener', this.session.media[0]); On 2014/08/13 23:21:02, Jamie wrote: > Why are we only interested in the first session? I think an explanatory comment > would be useful. Done. We only bother with the first because it's enough indication that something is wrong. We never directly load media on the receiver from the sender, so this should always be empty. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:190: this.chromotingMessageListener.bind(this)); On 2014/08/13 23:21:03, Jamie wrote: > Nit: Indentation Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:194: this.sendMessageFailure.bind(this)); On 2014/08/13 23:21:03, Jamie wrote: > Nit: Indentation Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:207: }; On 2014/08/13 23:21:04, Jamie wrote: > Do this function need to exist at all? I was assuming it was a callback from the > Cast API that we're not interested in, but it seems you're calling it > explicitly. It is a callback from the API, but it is also called explictly when we find an existing session with media since the API doesn't callback in that case. It should never be fired because we shouldn't find media on our sessions. If it is fired, we log an error. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:230: * successfully. On 2014/08/13 23:21:02, Jamie wrote: > This is the same comment as for sendMessageSuccess; is that intentional? Nope, I copied it over and forgot to s/sent to/received from. Thanks! https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:231: * @param {string} ns The namespace of the message received. On 2014/08/13 23:21:03, Jamie wrote: > You're not using the namespace. Shouldn't you be checking it against the > expected value? Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:257: * @param {chrome.cast.ReceiverAvailability} e Describes receiver availability. On 2014/08/13 23:21:02, Jamie wrote: > Parameter name. Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:260: if( e === chrome.cast.ReceiverAvailability.AVAILABLE) { On 2014/08/13 23:21:03, Jamie wrote: > Space after 'if', not before 'e'. Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:263: else { On 2014/08/13 23:21:02, Jamie wrote: > No newline before 'else'. Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:271: * initialization is used. On 2014/08/13 23:21:02, Jamie wrote: > I don't really understand what this function does. You say that the > SessionRequest passed during initialization is used by default, which suggests > to me that this method should allow an alternative to be specified, but it > doesn't. I think this comment needs to be more explicit. Clarified the comment. It is supposed to use the SessionRequest specified in the init phase. You COULD change the app being launched here, but we don't want to that. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:283: * Listener invoked when the chrome.cast.requestSession is successful. On 2014/08/13 23:21:03, Jamie wrote: > s/the// > s/is successful/completes successfully/ Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_p... File remoting/webapp/client_plugin.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_p... remoting/webapp/client_plugin.js:328: case 'cast_message': On 2014/08/13 23:21:04, Jamie wrote: > For consistency with the other extension messages, please rename this to > cast-message. Since that will presumably entail changing host code as well, it > can be a follow-up CL. Yup, it would entail changes in the host and the receiver app. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... File remoting/webapp/client_session.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:380: CAST: 'casting' On 2014/08/13 23:21:04, Jamie wrote: > I think I would call this capability chromeCast (and CHROME_CAST), since "cast" > is a pretty generic term. All the Cast docs avoid using Chromecast, because they are trying to imply that there can be multiple cast devices. Would "CAST_DESKTOP" make more sense? Changing the actual string will again require updating the Host and Android app, so it would make sense to do it in a follow if necessary. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1595: return; On 2014/08/13 23:21:04, Jamie wrote: > Braces for single-line conditionals, for consistency. sendClipboardItem and sendGnubbbyAuthMessage don't have braces, which is what I was following. WDYT? https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1600: * Process a remote cast extension message from the host. On 2014/08/13 23:21:04, Jamie wrote: > s/cast/Cast/, here and below Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1616: if(this.mode_ == remoting.ClientSession.Mode.ME2ME) { On 2014/08/13 23:21:04, Jamie wrote: > Nit: Space after 'if' Done. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1616: if(this.mode_ == remoting.ClientSession.Mode.ME2ME) { On 2014/08/13 23:21:04, Jamie wrote: > Why is this restricted to Me2Me? Only the remoting_me2me_host target supports the CastExtension, so I thought this was correct. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/html/tem... File remoting/webapp/html/template_main.html (left): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/html/tem... remoting/webapp/html/template_main.html:18: On 2014/08/13 23:21:04, Jamie wrote: > Please reinstate this blank line :) Haha, my apologies :) https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/js_proto... File remoting/webapp/js_proto/chrome_proto.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/js_proto... remoting/webapp/js_proto/chrome_proto.js:366: chrome.cast.DefaultActionPolicy.CREATE_SESSION; On 2014/08/13 23:21:04, Jamie wrote: > AutoJoinPolicy, DefaultActionPolicy and ReceiverAvailability feel like enums. > Are they actually complex types under-the-hood? They are, but I wasn't sure how to prototype those. Can this be fixed in a follow up? (jscompile is happy with this, though they are really enums). https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/manifest... File remoting/webapp/manifest.json.jinja2 (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/manifest... remoting/webapp/manifest.json.jinja2:40: "content_security_policy": "default-src 'self'; script-src 'self' {{ TALK_GADGET_HOST }} https://www.gstatic.com; style-src 'self' https://fonts.googleapis.com; img-src 'self' {{ TALK_GADGET_HOST }} data:; font-src *; connect-src 'self' {{ OAUTH2_ACCOUNTS_HOST }} {{ GOOGLE_API_HOSTS }} {{ TALK_GADGET_HOST }} https://relay.google.com", On 2014/08/13 23:21:04, Jamie wrote: > On 2014/08/13 01:29:34, aiguha wrote: > > This is to allow us to use the Google Cast Sender API library. > > Does it work with apps v2? Haven't had a chance to investigate that, can we discuss this tomorrow?
All remaining changes can be done in a follow-up if you want to CQ this right away. LGTM. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... File remoting/webapp/cast_extension_handler.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:43: this.start(); On 2014/08/15 07:09:54, aiguha wrote: > On 2014/08/13 23:21:02, Jamie wrote: > > Generally, doing anything non-trivial in the ctor is a bad idea. If you must > > call start() from the ctor, then it should probably be a private method. > > I think the call is unavoidable, because we need to load the script before doing > anything else, and that happens in start(). It is now a private method though. You can just call it explicitly immediately after calling new. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/cast_ext... remoting/webapp/cast_extension_handler.js:69: document.body.insertBefore(node, document.body.firstChild); On 2014/08/15 07:09:54, aiguha wrote: > On 2014/08/13 23:21:02, Jamie wrote: > > I think this should happen after adding the event listeners. JS is > > single-threaded, so I don't think there's a race if you do it in this order, > but > > it feels more natural. > > I was following wcs_loader.js's start() exactly here. Should I still change it? No, I'm pretty sure there's no race, so it can stay as-is. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... File remoting/webapp/client_session.js (right): https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:380: CAST: 'casting' On 2014/08/15 07:09:56, aiguha wrote: > On 2014/08/13 23:21:04, Jamie wrote: > > I think I would call this capability chromeCast (and CHROME_CAST), since > "cast" > > is a pretty generic term. > > All the Cast docs avoid using Chromecast, because they are trying to imply that > there can be multiple cast devices. Would "CAST_DESKTOP" make more sense? > Changing the actual string will again require updating the Host and Android app, > so it would make sense to do it in a follow if necessary. How about GOOGLE_CAST, since that's the name of the API? Leaving this for a follow-up is fine. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1595: return; On 2014/08/15 07:09:56, aiguha wrote: > On 2014/08/13 23:21:04, Jamie wrote: > > Braces for single-line conditionals, for consistency. > > sendClipboardItem and sendGnubbbyAuthMessage don't have braces, which is what I > was following. WDYT? I didn't realize they weren't consistent. Since there are no other critical changes for this CL, I don't think it needs to change before landing. https://codereview.chromium.org/464133003/diff/20001/remoting/webapp/client_s... remoting/webapp/client_session.js:1616: if(this.mode_ == remoting.ClientSession.Mode.ME2ME) { On 2014/08/15 07:09:56, aiguha wrote: > On 2014/08/13 23:21:04, Jamie wrote: > > Why is this restricted to Me2Me? > > Only the remoting_me2me_host target supports the CastExtension, so I thought > this was correct. I didn't realize that. I think it's correct in that case, but in principle there's no reason why the IT2Me version couldn't also support it.
The CQ bit was checked by aiguha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiguha@chromium.org/464133003/100001
Message was sent while issue was closed.
Committed patchset #6 (100001) as 290140 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
