Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11362267:
Add status service for remoting host. (Closed)
|
Created:
8 years, 1 month ago by Sergey Ulanov Modified:
8 years ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd status service for remoting host.
Host status service will accepts WebSocket connections from the webapp
and provides updates with the current status of the host.
BUG=132904
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169512
Patch Set 1 : #
Total comments: 76
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 11 (0 generated)
The new websocket server is turned off by default, and can be enabled via --enabled-status-service flag. It's obviously not the final state of how it should work, but I added this flag for testing for now.
https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... File remoting/host/host_status_service.cc (right): https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:24: const int kPortRangeMin = 12810; Does this range need a firewall rule on Windows? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:44: : service_(service), nit: Add a comment explaining that |service| must outlive |this|. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:46: websocket_->Accept(this); I believe this constructor is too complex to define it in the class declaration. Please move it down next of other methods of this class. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:48: virtual ~Connection() { nit: it is not required by the style guide but it will look nice of the destructor's body will be defined next to all other methods of the class. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:56: void SendHostStatusMessage(); nit: Add comments explaining what these methods are for. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:71: scoped_ptr<base::Value> json( nit: Can anything bad happen if a malicious sender sends a bad message? What if the message is very large? Malformed? Can it be detected early, so we will not need to spend time parsing megabytes of text? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:89: LOG(ERROR) << "Received message with unknown method: " << message; Call CloseOnError() here? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:121: char ip[] = {127, 0, 0, 1}; Will it still work in presence of IPv6? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:127: base::Unretained(this)))) { How is it guaranteed that |this| will not be deleted before the callback is invoked? Please add a comment. Or try to get rid of Unretained if possible. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:136: STLDeleteElements(&connections_); nit: Should |websocket_| be reset first? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:145: #ifndef NDEBUG Why is the difference? If it is just a sanity check that the main check should be outside of #ifdef https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:161: bool accept = true; nit: It is better to set |accept| to false by default and only set it to try when all checks pass. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:172: } else if (!IsAllowedOrigin(connection->origin())) { Can connection->origin() be trusted? Can the connecting party spoof it? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... File remoting/host/host_status_service.h (right): https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:19: class HostStatusService { nit: Add a comment explaining what is this class for. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:24: void SetState(bool started, const std::string& host_id); nit: Add a comment explaining what this method does. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:30: bool IsAllowedOrigin(const std::string& origin); nit: Add a comment explaining what this method does. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:37: scoped_ptr<base::DictionaryValue> GetStatusMessage(); nit: Add a comment explaining what this method does. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:39: WebsocketListener websocket_listener_; nit: Add comments explaining what these member variables are for. https://codereview.chromium.org/11362267/diff/4001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11362267/diff/4001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:913: status_service_->SetState(false, ""); nit: It may be worth splitting SetState into two separate methods: SetHostIsUp(host_id) and SetHostIsDown().
https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... File remoting/host/host_status_service.cc (right): https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:27: const char kChromeExtensionProtocolPrefix[] = "chrome-extension://"; You should really change this to kChromeExtensionUrlScheme (without the ://) and use google_url to split down the origin to check the scheme. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:29: #ifdef NDEBUG nit: #if !defined(NDEBUG) https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:48: virtual ~Connection() { On 2012/11/15 19:45:56, alexeypa wrote: > nit: it is not required by the style guide but it will look nice of the > destructor's body will be defined next to all other methods of the class. I'd prefer it be left in-line, and on a single line. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:60: // Closes this connection. Destroys the object, so should be used with care. nit: Reword e.g "Closes the connection and destroys this object." Use with care should be implicit. ;) https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:61: void CloseOnError(); nit: CloseOnError sounds like it tells the thing to close iff there's an error; I think you mean to close the connection and report an error? https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:74: base::DictionaryValue* message_dict = NULL; nit: Add a comment before this block explaining what we're doing. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:76: base::DictionaryValue* data = NULL; |data| doesn't seem to be used? https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:95: service_->OnConnectionClosed(this); You reset |websocket_| in CloseOnError() but not here; why not? If CloseOnError() deletes this object, does OnWebsocketClosed() not do the same? https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:126: endpoint, base::Bind(&HostStatusService::OnNewConnection, nit: Moving endpoint to the previous line would make this more readable, IMHO. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:136: STLDeleteElements(&connections_); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Should |websocket_| be reset first? It's held in a scoped_ptr<> in Connection, so that should be implicit, surely? https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:144: bool HostStatusService::IsAllowedOrigin(const std::string& origin) { Use GURL to break down the URL first, and then debug/non-debug enforcement based on the results. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:161: bool accept = true; I think this would be more readable as a series of: // Check that the request host is valid. if (...) { ... connection->Reject(); return; } clauses, rather than a sequence of if/else clauses and a flag. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:172: } else if (!IsAllowedOrigin(connection->origin())) { On 2012/11/15 19:45:56, alexeypa wrote: > Can connection->origin() be trusted? Can the connecting party spoof it? The origin protects from XSS; it's assumed the request was initiated by web-content, in which case the browser will have added the origin, so this is fine so long as you trust the browser. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.cc:186: connections_.erase(connection); This doesn't seem to delete Connection? https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... File remoting/host/host_status_service.h (right): https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:24: void SetState(bool started, const std::string& host_id); nit: I think it's cleaner to have two separate setters for these. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:27: class Connection; We seem to be re-inventing the socket-server wheel here, again; is there no existing class we can re-use for this? https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:37: scoped_ptr<base::DictionaryValue> GetStatusMessage(); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add a comment explaining what this method does. And similarly the comments for OnFoo should describe what OnFoo does. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:40: std::string host_name_; nit: Add a comment explaining what this is used for, and re-name to something more descriptive; specifically, this is not the name of the host whose status we are reporting, but actually some socket name. https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:44: std::string host_id_; nit: Add a comment e.g. "State values to provide to clients.
https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... File remoting/host/host_status_service.cc (right): https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:24: const int kPortRangeMin = 12810; On 2012/11/15 19:45:56, alexeypa wrote: > Does this range need a firewall rule on Windows? We are connecting from localhost to localhost. Are localhost->localhost connections blocked by the firewall? https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:27: const char kChromeExtensionProtocolPrefix[] = "chrome-extension://"; On 2012/11/15 23:34:59, Wez wrote: > You should really change this to kChromeExtensionUrlScheme (without the ://) and > use google_url to split down the origin to check the scheme. I considered this, but I don't think using GURL for parsing origin is the right thing to do: the origin is not supposed to be a full url - it's just scheme + domain. Not using it also makes code simpler. Renamed this const to kChromeExtensionUrlSchemePrefix. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:29: #ifdef NDEBUG On 2012/11/15 23:34:59, Wez wrote: > nit: #if !defined(NDEBUG) Changed to "#if defined(NDEBUG)", https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:44: : service_(service), On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add a comment explaining that |service| must outlive |this|. Done, but I don't think its really necessary because Connection class is internal to HostStatusService. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:46: websocket_->Accept(this); On 2012/11/15 19:45:56, alexeypa wrote: > I believe this constructor is too complex to define it in the class declaration. > Please move it down next of other methods of this class. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:48: virtual ~Connection() { On 2012/11/15 19:45:56, alexeypa wrote: > nit: it is not required by the style guide but it will look nice of the > destructor's body will be defined next to all other methods of the class. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:48: virtual ~Connection() { On 2012/11/15 23:34:59, Wez wrote: > On 2012/11/15 19:45:56, alexeypa wrote: > > nit: it is not required by the style guide but it will look nice of the > > destructor's body will be defined next to all other methods of the class. > > I'd prefer it be left in-line, and on a single line. I've already moved it, so I'll leave it there. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:56: void SendHostStatusMessage(); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add comments explaining what these methods are for. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:60: // Closes this connection. Destroys the object, so should be used with care. On 2012/11/15 23:34:59, Wez wrote: > nit: Reword e.g "Closes the connection and destroys this object." > > Use with care should be implicit. ;) Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:61: void CloseOnError(); On 2012/11/15 23:34:59, Wez wrote: > nit: CloseOnError sounds like it tells the thing to close iff there's an error; > I think you mean to close the connection and report an error? It means "close connection because there was a protocol error". Renamed to just Close() https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:71: scoped_ptr<base::Value> json( On 2012/11/15 19:45:56, alexeypa wrote: > nit: Can anything bad happen if a malicious sender sends a bad message? What if > the message is very large? Malformed? Can it be detected early, so we will not > need to spend time parsing megabytes of text? That's a good point. I think it's better to limit it on the lower level - in WebsocketConnection. Added call set_maximum_message_size() there and which is now called from Connection() constructor. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:74: base::DictionaryValue* message_dict = NULL; On 2012/11/15 23:34:59, Wez wrote: > nit: Add a comment before this block explaining what we're doing. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:76: base::DictionaryValue* data = NULL; On 2012/11/15 23:34:59, Wez wrote: > |data| doesn't seem to be used? We may need it in the future when we add some new methods, e.g. client may need to pass its version to the host. On this level this messaging protocol is identical to how client plugin and webapp communicate. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:89: LOG(ERROR) << "Received message with unknown method: " << message; On 2012/11/15 19:45:56, alexeypa wrote: > Call CloseOnError() here? Actually I think it's better to keep it connected, for backward compatibility, when we add new messages to the client. Also changed it to respond with "unsupportedMethod" message. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:95: service_->OnConnectionClosed(this); On 2012/11/15 23:34:59, Wez wrote: > You reset |websocket_| in CloseOnError() but not here; why not? If > CloseOnError() deletes this object, does OnWebsocketClosed() not do the same? Changed this to Close(). https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:121: char ip[] = {127, 0, 0, 1}; On 2012/11/15 19:45:56, alexeypa wrote: > Will it still work in presence of IPv6? Yes. On Linux localhost normally resolves to IPv4 address only (and localhost6 is used for IPv6 addresses). On Mac and Windows localhost resolves to both ::1 and 127.0.0.1. If a domain is resolved to both IPv4 and IPv6 addresses then the browser should try to connect to both, so it should still be able to connect if we listen just on the IPv4 port. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:126: endpoint, base::Bind(&HostStatusService::OnNewConnection, On 2012/11/15 23:34:59, Wez wrote: > nit: Moving endpoint to the previous line would make this more readable, IMHO. No, it doesn't, because that would mean that the second parameter has to be aligned with (, which makes this line longer than 80 chars. Anyway moved |endpoint| to a separate line, but that doesn't improve readability, IMHO. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:127: base::Unretained(this)))) { On 2012/11/15 19:45:56, alexeypa wrote: > How is it guaranteed that |this| will not be deleted before the callback is > invoked? Please add a comment. Or try to get rid of Unretained if possible. We own websocket_listener_ which is guaranteed to call the callback only while it still exists. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:136: STLDeleteElements(&connections_); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Should |websocket_| be reset first? It doesn't matter. websocket_listener_ is not connected to connections_ in any way. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:144: bool HostStatusService::IsAllowedOrigin(const std::string& origin) { On 2012/11/15 23:34:59, Wez wrote: > Use GURL to break down the URL first, and then debug/non-debug enforcement based > on the results. As explained above I think it would be overkill to use GURL here because we are not parsing URLs. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:145: #ifndef NDEBUG On 2012/11/15 19:45:56, alexeypa wrote: > Why is the difference? If it is just a sanity check that the main check should > be outside of #ifdef We want to limit which webapps can connect to the webservice. Problem is that it doesn't work well with a locally-built client which may have some random ID, so just to simplify debugging debug builds accept connections from any chrome webapp. Alternative solution would be to add parameter (e.g. in the config) that would allow whitelist specific app IDs, but it would make it harder to debug this code, but may still be useful. Added TODO about it. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:161: bool accept = true; On 2012/11/15 19:45:56, alexeypa wrote: > nit: It is better to set |accept| to false by default and only set it to try > when all checks pass. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:161: bool accept = true; On 2012/11/15 23:34:59, Wez wrote: > I think this would be more readable as a series of: > > // Check that the request host is valid. > if (...) { > ... > connection->Reject(); > return; > } > > clauses, rather than a sequence of if/else clauses and a flag. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:172: } else if (!IsAllowedOrigin(connection->origin())) { On 2012/11/15 19:45:56, alexeypa wrote: > Can connection->origin() be trusted? Can the connecting party spoof it? A web site cannot spoof it, but any locally installed code (or even a Chrome app that has access to TCP APIs) can. The main point is to block connections from drive-by web apps. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.cc:186: connections_.erase(connection); On 2012/11/15 23:34:59, Wez wrote: > This doesn't seem to delete Connection? Good catch. Fixed. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... File remoting/host/host_status_service.h (right): https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:19: class HostStatusService { On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add a comment explaining what is this class for. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:24: void SetState(bool started, const std::string& host_id); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add a comment explaining what this method does. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:24: void SetState(bool started, const std::string& host_id); On 2012/11/15 23:34:59, Wez wrote: > nit: I think it's cleaner to have two separate setters for these. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:27: class Connection; On 2012/11/15 23:34:59, Wez wrote: > We seem to be re-inventing the socket-server wheel here, again; is there no > existing class we can re-use for this? I'm not sure what you mean. We need to allow multiple parallel connections to the service (e.g. from different instances of the webapp), this requires having a class that knows how to handle messages from each connection separately. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:30: bool IsAllowedOrigin(const std::string& origin); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add a comment explaining what this method does. Added, but I think the name of the method is descriptive enough. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:37: scoped_ptr<base::DictionaryValue> GetStatusMessage(); On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add a comment explaining what this method does. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:37: scoped_ptr<base::DictionaryValue> GetStatusMessage(); On 2012/11/15 23:34:59, Wez wrote: > On 2012/11/15 19:45:56, alexeypa wrote: > > nit: Add a comment explaining what this method does. > > And similarly the comments for OnFoo should describe what OnFoo does. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:39: WebsocketListener websocket_listener_; On 2012/11/15 19:45:56, alexeypa wrote: > nit: Add comments explaining what these member variables are for. The purpose of |websocket_listener_| and |connections_| should be obvious from the names. Added comments for other memebers. https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:40: std::string host_name_; On 2012/11/15 23:34:59, Wez wrote: > nit: Add a comment explaining what this is used for, and re-name to something > more descriptive; specifically, this is not the name of the host whose status we > are reporting, but actually some socket name. Done. Renamed to service_host_name_ https://codereview.chromium.org/11362267/diff/4001/remoting/host/host_status_... remoting/host/host_status_service.h:44: std::string host_id_; On 2012/11/15 23:34:59, Wez wrote: > nit: Add a comment e.g. "State values to provide to clients. Done. https://codereview.chromium.org/11362267/diff/4001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11362267/diff/4001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:913: status_service_->SetState(false, ""); On 2012/11/15 19:45:56, alexeypa wrote: > nit: It may be worth splitting SetState into two separate methods: > SetHostIsUp(host_id) and SetHostIsDown(). Done.
ping
lgtm https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... File remoting/host/host_status_service.h (right): https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:30: bool IsAllowedOrigin(const std::string& origin); On 2012/11/16 00:52:10, sergeyu wrote: > On 2012/11/15 19:45:56, alexeypa wrote: > > nit: Add a comment explaining what this method does. > > Added, but I think the name of the method is descriptive enough. What I meant was a comment like "Returns true if |origin| is one of the remoting extension URLs." for example. https://chromiumcodereview.appspot.com/11362267/diff/8009/remoting/host/host_... File remoting/host/host_status_service.h (right): https://chromiumcodereview.appspot.com/11362267/diff/8009/remoting/host/host_... remoting/host/host_status_service.h:20: // remoting web app and lets it get current status of the host. nit: "HostStatusService implements a WebSocket server for the remoting web app to use to query the current status of the host"
https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... File remoting/host/host_status_service.h (right): https://chromiumcodereview.appspot.com/11362267/diff/4001/remoting/host/host_... remoting/host/host_status_service.h:30: bool IsAllowedOrigin(const std::string& origin); On 2012/11/20 05:55:53, Wez wrote: > On 2012/11/16 00:52:10, sergeyu wrote: > > On 2012/11/15 19:45:56, alexeypa wrote: > > > nit: Add a comment explaining what this method does. > > > > Added, but I think the name of the method is descriptive enough. > > What I meant was a comment like "Returns true if |origin| is one of the remoting > extension URLs." for example. Done. https://chromiumcodereview.appspot.com/11362267/diff/8009/remoting/host/host_... File remoting/host/host_status_service.h (right): https://chromiumcodereview.appspot.com/11362267/diff/8009/remoting/host/host_... remoting/host/host_status_service.h:20: // remoting web app and lets it get current status of the host. On 2012/11/20 05:55:53, Wez wrote: > nit: "HostStatusService implements a WebSocket server for the remoting web app > to use to query the current status of the host" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11362267/8010
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11362267/19002
Message was sent while issue was closed.
Change committed as 169512 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
