Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1238343002:
Added ConnectionTimeObserver to calculate the times to authenticate and connect. (Closed)
DescriptionAdded ConnectionTimeObserver to provide a way to calculate times between the different stages.
The ConnectionTimeObserver saves the state of the client and the time when the client state changes. The delta times can be accessed through GetStateTransitionDelay and a start and end state.
BUG=
Committed: https://crrev.com/684d1414ae40738e455092831088fbc0391ab11d
Cr-Commit-Position: refs/heads/master@{#341232}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Changed to ConnectionTimeObserver. #
Total comments: 7
Patch Set 3 : Synced with recent changes to connect to local host and cleaned up Connection Time Observer. #
Total comments: 32
Patch Set 4 : Added unittest, display connection stats ability, and error check for transition delay. #
Total comments: 26
Patch Set 5 : Modified display stats and added quit after connected. #
Total comments: 18
Patch Set 6 : Removed timer from observer, used print friendly string in TestChromotingClient, and made modificat… #
Total comments: 24
Patch Set 7 : Moved ConnectionStateToFriendlyString to ConnectionToHostImpl and Added environment class to main. #
Total comments: 11
Patch Set 8 : Moved FriendlyString to errors.h and connection_to_host.h. Cleaned up connection_time_observer_unit… #
Total comments: 10
Patch Set 9 : Made ToString methods into functions. Made code more readable and removed multiple defines in chromoting_instance.cc #
Total comments: 3
Patch Set 10 : Moved implementation to connection_to_host_impl.cc and removed inline. #
Total comments: 8
Patch Set 11 : Added errors.cc and cleaned up switch statements. #
Total comments: 6
Patch Set 12 : Removed unused headers. #
Messages
Total messages: 41 (11 generated)
Patchset #1 (id:1) has been deleted
tonychun@google.com changed reviewers: + anandc@chromium.org, joedow@chromium.org, sergeyu@chromium.org
Created a ChromotingConnectionObserver to display times to authenticate and connect.
https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_connection_observer.cc (right): https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:20: {protocol::ConnectionToHost::State::CLOSED, "CLOSED"}}; I think this would be cleaner as a file-scoped function in an anonymous namespace. You can basically just make a switch and return a value. It is easier to log errors there (using a default case) vs. having to check if a value exists before accessing the map. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:21: delta_ = base::TimeTicks::Now(); Why are you initializing this in the C'Tor? This seems odd as the class could be created a long time (in computer time) from when the connection is started. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:29: protocol::ErrorCode error_code) { Do you care about errors? https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:41: } You don't need to implement any of these functions if you aren't calc'ing any data in them. IIRC the test_chromoting_client already VLOGs the messages they come in so this is just adding spam to the log output. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_connection_observer.h (right): https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Is it intentional that you have not added this to any build files or unit tests? This seems like a partial CL. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.h:17: class ChromotingConnectionObserver I would probably just call this class a connection time observer. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.h:43: base::TimeTicks delta_; I think this var should be renamed as delta_ isn't very descriptive. last_state_change_time_ticks would be more clear.
Changed the name of the class to ConnectionTimeObserver and replaced the map with a switch case. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_connection_observer.cc (right): https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:20: {protocol::ConnectionToHost::State::CLOSED, "CLOSED"}}; On 2015/07/20 16:30:57, joedow wrote: > I think this would be cleaner as a file-scoped function in an anonymous > namespace. You can basically just make a switch and return a value. It is > easier to log errors there (using a default case) vs. having to check if a value > exists before accessing the map. I moved it all into a switch. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:21: delta_ = base::TimeTicks::Now(); On 2015/07/20 16:30:58, joedow wrote: > Why are you initializing this in the C'Tor? This seems odd as the class could > be created a long time (in computer time) from when the connection is started. If I don't do this, I get this error: INITIALIZING to CONNECTING Delta Time: 592674719 ms If I initialize it, it becomes: INITIALIZING to CONNECTING Delta Time: 1 ms I've seen the range from INITIALIZING to CONNECTING change from 1 ms to 3 ms on my local machine. ConnectionStateChanged is called very soon after the initialization of the class. In addition, we aren't looking for that metric, so the time between these two states are not important for Sergey or Anand. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:29: protocol::ErrorCode error_code) { On 2015/07/20 16:30:57, joedow wrote: > Do you care about errors? A log error is output already through the TestChromotingClient. Example: " TestChromotingClient::OnConnectionState(state: CONNECTING, error_code: NONE) Called INITIALIZING to CONNECTING Delta Time: 1 ms TestChromotingClient::OnConnectionState(state: FAILED, error_code: AUTHENTICATION_FAILED) Called CONNECTING to FAILED Delta Time: 505 ms " On any rate, I don't need to know about the error code. I only need transition times. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.cc:41: } On 2015/07/20 16:30:57, joedow wrote: > You don't need to implement any of these functions if you aren't calc'ing any > data in them. IIRC the test_chromoting_client already VLOGs the messages they > come in so this is just adding spam to the log output. Done. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_connection_observer.h (right): https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/07/20 16:30:58, joedow wrote: > Is it intentional that you have not added this to any build files or unit tests? > This seems like a partial CL. This is a partial CL. Once my changes for environment and connecting to localhost land, I will continue to add this change. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.h:17: class ChromotingConnectionObserver On 2015/07/20 16:30:58, joedow wrote: > I would probably just call this class a connection time observer. Done. https://codereview.chromium.org/1238343002/diff/20001/remoting/test/chromotin... remoting/test/chromoting_connection_observer.h:43: base::TimeTicks delta_; On 2015/07/20 16:30:58, joedow wrote: > I think this var should be renamed as delta_ isn't very descriptive. > last_state_change_time_ticks would be more clear. Done.
https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.cc:38: : current_state_(protocol::ConnectionToHost::State::INITIALIZING), nit: move this initializer to .h https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.h:14: // connection process. And it also logs the calculated values. The purpose is not clear without mentioning it in the comment, because the class doesn't provide any way to get these latency values. Alternatively the class can just store the number and provide GetStateTransitionDelay() method to get them. https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.h:31: base::TimeTicks last_state_change_time_ticks; add _ at the end of the name https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.h:31: base::TimeTicks last_state_change_time_ticks; nit: call it last_state_change_time_ or last_state_change_ticks_ - both are just as clear and shorter.
Synced with recent changes to connect to local host and added a GetStateTransitionDelay method to calculate the times between each state. https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.cc:38: : current_state_(protocol::ConnectionToHost::State::INITIALIZING), On 2015/07/20 23:04:04, Sergey Ulanov wrote: > nit: move this initializer to .h Done. https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.h:14: // connection process. On 2015/07/20 23:04:04, Sergey Ulanov wrote: > And it also logs the calculated values. The purpose is not clear without > mentioning it in the comment, because the class doesn't provide any way to get > these latency values. Alternatively the class can just store the number and > provide GetStateTransitionDelay() method to get them. Done. https://codereview.chromium.org/1238343002/diff/40001/remoting/test/connectio... remoting/test/connection_time_observer.h:31: base::TimeTicks last_state_change_time_ticks; On 2015/07/20 23:04:04, Sergey Ulanov wrote: > nit: call it last_state_change_time_ or last_state_change_ticks_ - both are just > as clear and shorter. Done.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/1238343002/diff/80001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/remoting_test.... remoting/remoting_test.gypi:62: 'test/connection_time_observer.h', Unittests? https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:29: LOG(ERROR) << "Unknown current state"; When printing errors in default cases it is useful to include the value that was unknown. The error above should be "Unknown state: " << state; https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:43: std::make_pair(current_state_, base::TimeTicks::Now())); I don't think this should occur in the C'Tor, you are essentially dictating usage of the class (i.e. it must be instantiated just before you start a connection). Someone may want to create it as part of a class and attach/detach in different functions, in that case this value is going to be WAY off. A better solution would be to have the client call "ConnectionStateChanged(Initializing)" manually just before creating the connection. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:52: // Save the current TimeTick of the new state into |transition_times_map_|. Please remove comment, it is just stating what the next line of codes does. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:59: // Update the current state of the client. please remove comment, documenting code, same as other comments about comments in this file. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:63: int ConnectionTimeObserver::GetStateTransitionDelay( Can you update the function name? It isn't obvious what units the int value returned represents. It should be something like GetStateTransitionDelayMs() https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:71: // Otherwise, return the delta time between the two states. This comment isn't really needed, I can see what is going on from reading the code. Comments should document intent and assumptions but not act as pseudocode (otherwise you need to update the pseudocode when the actual code changes) https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:72: if (iter_start_state == iter_end) { A better pattern for the return values would be to create a 'int connection_delay_ms = 0;' above the if/else if/else block and only set the value in the success case. Then you can have a single return statement which would make this logic cleaner. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:77: return (iter_end_state->second - iter_start_state->second).InMilliseconds(); You may want to check this value to ensure it isn't negative, otherwise log an error. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:79: return -1; I wonder if 0 is a better option. No action should ever take 0ms. Using negative values might cause odd formatting and logic issues whereas 0 is pretty obvious that there is a problem. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:81: I think it would be good to add a function that rolls up the stats (DisplayConnectionStats()) that will show total time as well as time between transitions. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.h:15: // Observes and saves the times when TestChromotingClient changes its state. It I don't think the comment should mention the TestChromotingClient, this connection observer could be used for any class which creates a chromoting connection and allows an observer to be attached. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.h:35: // Saves the latest state the ChromotingClient is in. Technically the state is provided by the ConnectionToHost class, not the ChromotingClient (the client just pulls all of the connection pieces together, it doesn't create the connection). Simpler just to say that it represents the current connection state of client to host.
https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:11: namespace { Move this inside remoting::test namespace then you wouldn't need to specify namespace inside the function. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:79: return -1; On 2015/07/23 16:57:54, joedow wrote: > I wonder if 0 is a better option. No action should ever take 0ms. Using > negative values might cause odd formatting and logic issues whereas 0 is pretty > obvious that there is a problem. With base::TimeDelta result this can TimeDelta::Max() https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:79: return -1; Normally we handle error cases first, and the end should be non-error case, i.e.: auto iter_start_state = transition_times_map_.find(start_state); if (iter_start_state == iter_end) { LOG(ERROR) << "No time found for state " << GetStateAsString(start_state); return -1; } auto iter_end_state = transition_times_map_.find(end_state); if (iter_end_state == iter_end) { LOG(ERROR) << "No time found for state " << GetStateAsString(end_state); return -1; } return iter_end_state->second - iter_start_state->second; https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.h:30: int GetStateTransitionDelay( Return base::TimeDelta here?
Updated the ConnectionTimeObserver to be able to display the connection stats (shows total time and delta time). I also made the GetStateTransitionDelay return base::DeltaTime with error checks for finding non-existent states and created the unittest for ConnectionTimeObserver. https://codereview.chromium.org/1238343002/diff/80001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/remoting_test.... remoting/remoting_test.gypi:62: 'test/connection_time_observer.h', On 2015/07/23 16:57:54, joedow wrote: > Unittests? Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:11: namespace { On 2015/07/23 19:20:57, Sergey Ulanov wrote: > Move this inside remoting::test namespace then you wouldn't need to specify > namespace inside the function. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:29: LOG(ERROR) << "Unknown current state"; On 2015/07/23 16:57:54, joedow wrote: > When printing errors in default cases it is useful to include the value that was > unknown. The error above should be "Unknown state: " << state; Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:43: std::make_pair(current_state_, base::TimeTicks::Now())); On 2015/07/23 16:57:54, joedow wrote: > I don't think this should occur in the C'Tor, you are essentially dictating > usage of the class (i.e. it must be instantiated just before you start a > connection). Someone may want to create it as part of a class and attach/detach > in different functions, in that case this value is going to be WAY off. > > A better solution would be to have the client call > "ConnectionStateChanged(Initializing)" manually just before creating the > connection. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:52: // Save the current TimeTick of the new state into |transition_times_map_|. On 2015/07/23 16:57:54, joedow wrote: > Please remove comment, it is just stating what the next line of codes does. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:59: // Update the current state of the client. On 2015/07/23 16:57:54, joedow wrote: > please remove comment, documenting code, same as other comments about comments > in this file. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:63: int ConnectionTimeObserver::GetStateTransitionDelay( On 2015/07/23 16:57:54, joedow wrote: > Can you update the function name? It isn't obvious what units the int value > returned represents. It should be something like GetStateTransitionDelayMs() I'm keeping it the way it is, but returning base::TimeDelta instead. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:71: // Otherwise, return the delta time between the two states. On 2015/07/23 16:57:54, joedow wrote: > This comment isn't really needed, I can see what is going on from reading the > code. Comments should document intent and assumptions but not act as pseudocode > (otherwise you need to update the pseudocode when the actual code changes) Good point. Thanks. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:72: if (iter_start_state == iter_end) { On 2015/07/23 16:57:54, joedow wrote: > A better pattern for the return values would be to create a 'int > connection_delay_ms = 0;' above the if/else if/else block and only set the value > in the success case. Then you can have a single return statement which would > make this logic cleaner. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:77: return (iter_end_state->second - iter_start_state->second).InMilliseconds(); On 2015/07/23 16:57:54, joedow wrote: > You may want to check this value to ensure it isn't negative, otherwise log an > error. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:79: return -1; On 2015/07/23 19:20:57, Sergey Ulanov wrote: > On 2015/07/23 16:57:54, joedow wrote: > > I wonder if 0 is a better option. No action should ever take 0ms. Using > > negative values might cause odd formatting and logic issues whereas 0 is > pretty > > obvious that there is a problem. > > With base::TimeDelta result this can TimeDelta::Max() Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.cc:81: On 2015/07/23 16:57:54, joedow wrote: > I think it would be good to add a function that rolls up the stats > (DisplayConnectionStats()) that will show total time as well as time between > transitions. Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.h:15: // Observes and saves the times when TestChromotingClient changes its state. It On 2015/07/23 16:57:55, joedow wrote: > I don't think the comment should mention the TestChromotingClient, this > connection observer could be used for any class which creates a chromoting > connection and allows an observer to be attached. Good point. Thank you. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.h:30: int GetStateTransitionDelay( On 2015/07/23 19:20:57, Sergey Ulanov wrote: > Return base::TimeDelta here? Done. https://codereview.chromium.org/1238343002/diff/80001/remoting/test/connectio... remoting/test/connection_time_observer.h:35: // Saves the latest state the ChromotingClient is in. On 2015/07/23 16:57:54, joedow wrote: > Technically the state is provided by the ConnectionToHost class, not the > ChromotingClient (the client just pulls all of the connection pieces together, > it doesn't create the connection). Simpler just to say that it represents the > current connection state of client to host. I've changed the name to current_connection_state_ to better reflect what it means. I've also changed the comments.
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:15: std::string GetStateAsString(const protocol::ConnectionToHost::State& state) { no need for const ref on the state param. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:50: transition_times_map_.insert(std::make_pair(state, base::TimeTicks::Now())); Seems like you would want to protect against adding the same state twice (in case of a future product or test bug). Perhaps a LOG(ERROR) and early return would make sense. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:81: GetStateTransitionDelay(current_state, state).InMilliseconds(), "ms"); This loop prints the difference between initializing and each state, I was thinking of something a little different: 1.) Print total connection duration (either initializing to connected or initializing to error (if one occurred) 2.) Print total connection time (initializing to closed/failed) 3.) Print duration for each transition segment (initializing to connecting, connecting to authenticated, etc) WIth these stats it will be clear where the most time is being spent and how it rolls up to the total, with the method above you are making the reader of the output do more work. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:106: LOG(ERROR) << "Transition delay is negative. Did you switch the states?"; nit: 'Did you switch the states?' is a little strongly worded, I'd suggest something like 'check the state ordering: start iter_start->second, end iter_end->second' so that the debug output shows the states passed in so from the debug log the reader can determine if it is a calling error or a product error. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:21: // client was in. nit: s/was in/transitioned through https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:30: protocol::ErrorCode error_code) override; You may consider adding a helper method for setting the intial state (either something like 'Start' if the usage is like a stopwatch or a simplified version of this method so the caller doesn't need to provide an ErrorCode (since you don't use it at the moment). https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:33: void DisplayConnectionStats(); this method should be const. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:35: // Returns the delta time in milliseconds between |start_state| and nit: s/delta time/time delta https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:37: base::TimeDelta GetStateTransitionDelay( With the updated signature, this method should probably be called GetStateTransitionDelayTimeDelta(). https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:38: const protocol::ConnectionToHost::State& start_state, The parameters here do not need to be passed as const ref because they are passed by value. Please remove the const ref'ness for the enum values being passed. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:96: TEST_F(ConnectionTimeObserverTest, NegativeTranitionDelay) { s/Tranition/Transition https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:125: protocol::ConnectionToHost::State::FAILED).InMilliseconds(), 0); You probably want to use the is_zero() method on the return value and use EXPECT_FALSE here. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:126: } You should add a test that sets two states via ConnectionStateChanged and verify that they are properly added. Using the test map is good for the others but you want to cover the map insert code as well.
Updated display stats and added ability to run done closure as soon as chromoting connection is established. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:15: std::string GetStateAsString(const protocol::ConnectionToHost::State& state) { On 2015/07/24 13:38:28, joedow wrote: > no need for const ref on the state param. Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:50: transition_times_map_.insert(std::make_pair(state, base::TimeTicks::Now())); On 2015/07/24 13:38:28, joedow wrote: > Seems like you would want to protect against adding the same state twice (in > case of a future product or test bug). Perhaps a LOG(ERROR) and early return > would make sense. Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:81: GetStateTransitionDelay(current_state, state).InMilliseconds(), "ms"); On 2015/07/24 13:38:28, joedow wrote: > This loop prints the difference between initializing and each state, I was > thinking of something a little different: > 1.) Print total connection duration (either initializing to connected or > initializing to error (if one occurred) > 2.) Print total connection time (initializing to closed/failed) > 3.) Print duration for each transition segment (initializing to connecting, > connecting to authenticated, etc) > > WIth these stats it will be clear where the most time is being spent and how it > rolls up to the total, with the method above you are making the reader of the > output do more work. I have added these suggestions. However, I don't think that #2 will serve much use because we are only really concerned with authentication time and authenticated to connected time. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.cc:106: LOG(ERROR) << "Transition delay is negative. Did you switch the states?"; On 2015/07/24 13:38:28, joedow wrote: > nit: 'Did you switch the states?' is a little strongly worded, I'd suggest > something like 'check the state ordering: start iter_start->second, end > iter_end->second' so that the debug output shows the states passed in so from > the debug log the reader can determine if it is a calling error or a product > error. Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:21: // client was in. On 2015/07/24 13:38:28, joedow wrote: > nit: s/was in/transitioned through Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:30: protocol::ErrorCode error_code) override; On 2015/07/24 13:38:28, joedow wrote: > You may consider adding a helper method for setting the intial state (either > something like 'Start' if the usage is like a stopwatch or a simplified version > of this method so the caller doesn't need to provide an ErrorCode (since you > don't use it at the moment). Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:33: void DisplayConnectionStats(); On 2015/07/24 13:38:28, joedow wrote: > this method should be const. Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:35: // Returns the delta time in milliseconds between |start_state| and On 2015/07/24 13:38:29, joedow wrote: > nit: s/delta time/time delta Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:37: base::TimeDelta GetStateTransitionDelay( On 2015/07/24 13:38:28, joedow wrote: > With the updated signature, this method should probably be called > GetStateTransitionDelayTimeDelta(). Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer.h:38: const protocol::ConnectionToHost::State& start_state, On 2015/07/24 13:38:29, joedow wrote: > The parameters here do not need to be passed as const ref because they are > passed by value. Please remove the const ref'ness for the enum values being > passed. Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:96: TEST_F(ConnectionTimeObserverTest, NegativeTranitionDelay) { On 2015/07/24 13:38:29, joedow wrote: > s/Tranition/Transition Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:125: protocol::ConnectionToHost::State::FAILED).InMilliseconds(), 0); On 2015/07/24 13:38:29, joedow wrote: > You probably want to use the is_zero() method on the return value and use > EXPECT_FALSE here. Done. https://codereview.chromium.org/1238343002/diff/100001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:126: } On 2015/07/24 13:38:29, joedow wrote: > You should add a test that sets two states via ConnectionStateChanged and verify > that they are properly added. Using the test map is good for the others but you > want to cover the map insert code as well. Done.
https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.cc:77: timer_->user_task().Run(); If you are trying to get to the QuitClosure() to signal the connection is ready, then I think you should just use that type. Passing the timer around is a lot of overhead for little benefit. Also, this class is should handle gathering and reporting performance metrics, it shouldn't be signaling connection state changes. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.h:34: // Initializes INITIALIZING state's TimeTicks. This comment is awkward, I think the method name is a little hard to understand. The idea for the name depends on how your want people to use this class (previously I mentioned a stopwatch as an example though I don't think that is the usage here). I mentioned in a previous iteration that you could use an override for 'ConnectionStateChanged' which just took the state, that may still be cleaner as I wouldn't know what this method did without reading the code. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:24: These members should have comments and live in the protected section.
https://codereview.chromium.org/1238343002/diff/140001/remoting/test/chromoti... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/chromoti... remoting/test/chromoting_test_driver.cc:201: // Update the logging verbosity level is user specified one. "if user specified one" https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.cc:16: std::string GetStateAsString(protocol::ConnectionToHost::State state) { ConnectionStateToFriendlyString, as defined here: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/test/test... ? https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.cc:73: // will be saved when a test completes a chromoting connection to a host. Do you mean "when a test terminates a Chromoting connection"? https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.h:42: base::TimeDelta GetStateTransitionDelayTimeDelta( GetStateTransitionTime. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.h:55: // Stores TimeTick of states the client changes to. "The TimeTicks to get to a state from the previous state." https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:171: protocol::ConnectionToHost::State::CONNECTED).is_zero()); Strictly speaking, this should expect_false is_less_than_zero.
Removed timer from observer, used print friendly string in TestChromotingClient, and made modifications to code and comments for clarity. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/chromoti... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/chromoti... remoting/test/chromoting_test_driver.cc:201: // Update the logging verbosity level is user specified one. On 2015/07/27 18:22:28, anandc wrote: > "if user specified one" Done. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.cc:16: std::string GetStateAsString(protocol::ConnectionToHost::State state) { On 2015/07/27 18:22:29, anandc wrote: > ConnectionStateToFriendlyString, as defined here: > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/test/test... > ? Done. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.cc:73: // will be saved when a test completes a chromoting connection to a host. On 2015/07/27 18:22:29, anandc wrote: > Do you mean "when a test terminates a Chromoting connection"? Done. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.cc:77: timer_->user_task().Run(); On 2015/07/27 16:25:39, joedow wrote: > If you are trying to get to the QuitClosure() to signal the connection is ready, > then I think you should just use that type. Passing the timer around is a lot > of overhead for little benefit. Also, this class is should handle gathering and > reporting performance metrics, it shouldn't be signaling connection state > changes. I will remove this and create a helper class to handle signaling connection state changes. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer.h (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.h:34: // Initializes INITIALIZING state's TimeTicks. On 2015/07/27 16:25:39, joedow wrote: > This comment is awkward, I think the method name is a little hard to understand. > The idea for the name depends on how your want people to use this class > (previously I mentioned a stopwatch as an example though I don't think that is > the usage here). I mentioned in a previous iteration that you could use an > override for 'ConnectionStateChanged' which just took the state, that may still > be cleaner as I wouldn't know what this method did without reading the code. I've added clearer comments and changed the name to Initialize. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.h:42: base::TimeDelta GetStateTransitionDelayTimeDelta( On 2015/07/27 18:22:29, anandc wrote: > GetStateTransitionTime. Done. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer.h:55: // Stores TimeTick of states the client changes to. On 2015/07/27 18:22:29, anandc wrote: > "The TimeTicks to get to a state from the previous state." Done. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:24: On 2015/07/27 16:25:39, joedow wrote: > These members should have comments and live in the protected section. Done. https://codereview.chromium.org/1238343002/diff/140001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:171: protocol::ConnectionToHost::State::CONNECTED).is_zero()); On 2015/07/27 18:22:29, anandc wrote: > Strictly speaking, this should expect_false is_less_than_zero. I've made it check specifically for the 10 millisecond difference. This shows that the functionality really waits 10 milliseconds and is a positive difference.
https://codereview.chromium.org/1238343002/diff/160001/remoting/test/chromoti... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/chromoti... remoting/test/chromoting_test_driver.cc:310: test_chromoting_client.AddRemoteConnectionObserver(&observer); Please send an update once your change to add the environment class to main() lands so we can review the merge of the two CLs. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:12: #include "remoting/test/test_chromoting_client.h" I don't think this class should know about the TestChromotingClient since the only thing it uses is a helper method. May we can put the helper method in the same file that defines the connection states, that way any code which uses those states can print nice error messages. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:30: LOG(ERROR) << "INITIALIZING state is already initialized"; nit: s/is already initialized/has already been set. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:34: protocol::ConnectionToHost::State::INITIALIZING, why not call just call ConectionStateChanged() with initializing and ok from here. That way you get the error checking (to prevent double initializing) and the insert willl be done for you. This is a good wrapper method but you may as well call the existing method to do the work for you. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:53: base::TimeTicks::Now())); What if 'ready' is true? Then you will still log connection state as 'closed'. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:67: std::vector<protocol::ConnectionToHost::State> list_of_states; if you don't include 'closed' here, this should probably be called 'connected_states'. You don't need to say 'list_of' since the type is apparent. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:71: list_of_states.push_back(protocol::ConnectionToHost::State::FAILED); nit: a comment that the order of the list is important (i.e. that it mimics the expected order when a connection is made) would be good here. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:98: << " ms"; Do you want to make sure there is a closed state before logging this? If a connection was never created then the state may never be added. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:108: LOG(ERROR) << "No time found for state " nit: I like to add a colon (or similar) to the end of the static text so I know when the dynamic text (error string/state) starts. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:160: // Verify that the time waited is positive and exactly 10 milliseconds. I don't think this will work consistently. Posting a delayed task (say for 10ms in the future) is not guaranteed to run in exactly 10ms, it could take longer. You could make this a greater than check to be safe. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... File remoting/test/test_chromoting_client.cc (left): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... remoting/test/test_chromoting_client.cc:90: const char* ProtocolErrorToFriendlyString( Per my previous comment about adding a pretty printing helper, it's probably a good idea to move these into the same file as the enum values are defined so anyone who includes the file (i.e. interested in the values) can print nice error messages. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... remoting/test/test_chromoting_client.cc:124: // static please remove comment, it isn't needed in the impl file
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:180001) has been deleted
Merged with environment to main changes and made changes to use FriendlyString. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/chromoti... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/chromoti... remoting/test/chromoting_test_driver.cc:310: test_chromoting_client.AddRemoteConnectionObserver(&observer); On 2015/07/27 21:45:44, joedow wrote: > Please send an update once your change to add the environment class to main() > lands so we can review the merge of the two CLs. The changes have landed and are now together with this CL. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:12: #include "remoting/test/test_chromoting_client.h" On 2015/07/27 21:45:44, joedow wrote: > I don't think this class should know about the TestChromotingClient since the > only thing it uses is a helper method. May we can put the helper method in the > same file that defines the connection states, that way any code which uses those > states can print nice error messages. Moved helper method to ConnectionToHostImpl. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:30: LOG(ERROR) << "INITIALIZING state is already initialized"; On 2015/07/27 21:45:44, joedow wrote: > nit: s/is already initialized/has already been set. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:34: protocol::ConnectionToHost::State::INITIALIZING, On 2015/07/27 21:45:44, joedow wrote: > why not call just call ConectionStateChanged() with initializing and ok from > here. That way you get the error checking (to prevent double initializing) and > the insert willl be done for you. This is a good wrapper method but you may as > well call the existing method to do the work for you. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:53: base::TimeTicks::Now())); On 2015/07/27 21:45:44, joedow wrote: > What if 'ready' is true? Then you will still log connection state as 'closed'. For right now, since I don't have the helper class implemented, I will remove this and have the helper class cleanly close the connection. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:67: std::vector<protocol::ConnectionToHost::State> list_of_states; On 2015/07/27 21:45:44, joedow wrote: > if you don't include 'closed' here, this should probably be called > 'connected_states'. You don't need to say 'list_of' since the type is apparent. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:71: list_of_states.push_back(protocol::ConnectionToHost::State::FAILED); On 2015/07/27 21:45:44, joedow wrote: > nit: a comment that the order of the list is important (i.e. that it mimics the > expected order when a connection is made) would be good here. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:98: << " ms"; On 2015/07/27 21:45:44, joedow wrote: > Do you want to make sure there is a closed state before logging this? If a > connection was never created then the state may never be added. Removed. I will implement closing with the helper class, then re-add in the LOG that I removed. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer.cc:108: LOG(ERROR) << "No time found for state " On 2015/07/27 21:45:44, joedow wrote: > nit: I like to add a colon (or similar) to the end of the static text so I know > when the dynamic text (error string/state) starts. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:160: // Verify that the time waited is positive and exactly 10 milliseconds. On 2015/07/27 21:45:44, joedow wrote: > I don't think this will work consistently. Posting a delayed task (say for 10ms > in the future) is not guaranteed to run in exactly 10ms, it could take longer. > You could make this a greater than check to be safe. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... File remoting/test/test_chromoting_client.cc (left): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... remoting/test/test_chromoting_client.cc:90: const char* ProtocolErrorToFriendlyString( On 2015/07/27 21:45:45, joedow wrote: > Per my previous comment about adding a pretty printing helper, it's probably a > good idea to move these into the same file as the enum values are defined so > anyone who includes the file (i.e. interested in the values) can print nice > error messages. Done. https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1238343002/diff/160001/remoting/test/test_chr... remoting/test/test_chromoting_client.cc:124: // static On 2015/07/27 21:45:44, joedow wrote: > please remove comment, it isn't needed in the impl file Done.
https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.h (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.h:44: remoting::protocol::ErrorCode error_code); I'm wondering if this should be in connection_to_host.h (I don't think the impl headers should be included by anything other than the impl.cc file. Can you check with sergey and confirm? https://codereview.chromium.org/1238343002/diff/200001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:28: std::map<protocol::ConnectionToHost::State, base::TimeTicks> test_map_; please add newline here https://codereview.chromium.org/1238343002/diff/200001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:108: ConnectionTimeObserver connection_time_observer; Can you move the connection_time_observer into the test fixture? You create one in each test and you can save some space moving it up.
https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.cc (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:31: case remoting::protocol::ConnectionToHost::INITIALIZING: You can use macro like this to make this code shorter: https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_util... https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.h (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.h:40: const char* ConnectionStateToFriendlyString( Make this static method in ConnectionToHost? https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.h:44: remoting::protocol::ErrorCode error_code); On 2015/07/28 23:09:05, joedow wrote: > I'm wondering if this should be in connection_to_host.h (I don't think the impl > headers should be included by anything other than the impl.cc file. Can you > check with sergey and confirm? Yeah, I don't think it belongs in this file. Move to remoting/protocol/error.[h|cc]?
Moved around ToFriendString methods to errors.h and ConnectionToHost. https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.cc (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:31: case remoting::protocol::ConnectionToHost::INITIALIZING: On 2015/07/29 00:56:23, Sergey Ulanov wrote: > You can use macro like this to make this code shorter: > https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_util... Done. https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.h (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.h:40: const char* ConnectionStateToFriendlyString( On 2015/07/29 00:56:23, Sergey Ulanov wrote: > Make this static method in ConnectionToHost? Done. https://codereview.chromium.org/1238343002/diff/200001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.h:44: remoting::protocol::ErrorCode error_code); On 2015/07/29 00:56:23, Sergey Ulanov wrote: > On 2015/07/28 23:09:05, joedow wrote: > > I'm wondering if this should be in connection_to_host.h (I don't think the > impl > > headers should be included by anything other than the impl.cc file. Can you > > check with sergey and confirm? > > Yeah, I don't think it belongs in this file. Move to > remoting/protocol/error.[h|cc]? I've added an Error class which also has a static method. https://codereview.chromium.org/1238343002/diff/200001/remoting/test/connecti... File remoting/test/connection_time_observer_unittest.cc (right): https://codereview.chromium.org/1238343002/diff/200001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:28: std::map<protocol::ConnectionToHost::State, base::TimeTicks> test_map_; On 2015/07/28 23:09:05, joedow wrote: > please add newline here Done. https://codereview.chromium.org/1238343002/diff/200001/remoting/test/connecti... remoting/test/connection_time_observer_unittest.cc:108: ConnectionTimeObserver connection_time_observer; On 2015/07/28 23:09:05, joedow wrote: > Can you move the connection_time_observer into the test fixture? You create one > in each test and you can save some space moving it up. Done.
https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:18: return #x; I'm personally not a fan of macros like this for a handful of returns, but I'll leave this one up to you since Sergey suggested it. https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/erro... File remoting/protocol/errors.h (right): https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/erro... remoting/protocol/errors.h:34: class Error { Adding a helper method to ConnectionToHost felt fine but this seems awkward (i.e. defining a class just for a static method). I think this should be a namespace instead (or just plop the method in the remoting::protocol namespace. https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... remoting/test/connection_time_observer.cc:32: state) << " state has already been set"; Can you fix the indentation here. Alternatively you could save this state into a string and just clean up this statement. https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... remoting/test/connection_time_observer.cc:78: current_state).InMilliseconds() << " ms"; The indentation is kind of distracting here, can you clean it up? https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... remoting/test/connection_time_observer.cc:97: << protocol::ConnectionToHost::ConnectionStateToFriendlyString( Using a static method on ConnectionToHost to generate the string is really causing readability issues for me. We should consider either getting the string before the log message, moving the helper method out of 'ConnectionToHost' and into the namespace, or perhaps shortening the method name.
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
I've moved the methods out into inlined functions. I've also cleaned up the code in ConnectionTimeObserver to be more readable and made changes to ChromotingInstance to deal with redefinition error. https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:18: return #x; On 2015/07/29 17:07:02, joedow wrote: > I'm personally not a fan of macros like this for a handful of returns, but I'll > leave this one up to you since Sergey suggested it. Done. https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/erro... File remoting/protocol/errors.h (right): https://codereview.chromium.org/1238343002/diff/220001/remoting/protocol/erro... remoting/protocol/errors.h:34: class Error { On 2015/07/29 17:07:02, joedow wrote: > Adding a helper method to ConnectionToHost felt fine but this seems awkward > (i.e. defining a class just for a static method). I think this should be a > namespace instead (or just plop the method in the remoting::protocol namespace. Done. https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... File remoting/test/connection_time_observer.cc (right): https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... remoting/test/connection_time_observer.cc:32: state) << " state has already been set"; On 2015/07/29 17:07:02, joedow wrote: > Can you fix the indentation here. Alternatively you could save this state into > a string and just clean up this statement. Done. https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... remoting/test/connection_time_observer.cc:78: current_state).InMilliseconds() << " ms"; On 2015/07/29 17:07:02, joedow wrote: > The indentation is kind of distracting here, can you clean it up? Done. https://codereview.chromium.org/1238343002/diff/220001/remoting/test/connecti... remoting/test/connection_time_observer.cc:97: << protocol::ConnectionToHost::ConnectionStateToFriendlyString( On 2015/07/29 17:07:02, joedow wrote: > Using a static method on ConnectionToHost to generate the string is really > causing readability issues for me. We should consider either getting the string > before the log message, moving the helper method out of 'ConnectionToHost' and > into the namespace, or perhaps shortening the method name. Done. https://codereview.chromium.org/1238343002/diff/280001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (left): https://codereview.chromium.org/1238343002/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:101: std::string ConnectionStateToString(protocol::ConnectionToHost::State state) { Since this functionality exists in remoting/protocol/connection_to_host.h, I removed this.
Patchset #10 (id:300001) has been deleted
lgtm. Once inline is removed. https://codereview.chromium.org/1238343002/diff/280001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/280001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:114: inline const char* ConnectionStateToString(ConnectionToHost::State state) { I'd remove the inline here. This function is called for debugging or displaying nice error messages and isn't performance critical so it is more likely to bloat the code than speed it up. https://codereview.chromium.org/1238343002/diff/280001/remoting/protocol/erro... File remoting/protocol/errors.h (right): https://codereview.chromium.org/1238343002/diff/280001/remoting/protocol/erro... remoting/protocol/errors.h:34: inline const char* ErrorCodeToString(ErrorCode error) { remove inline
Removed inline and moved implementation to connection_to_host_impl.cc.
https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:111: const char* ConnectionStateToString(ConnectionToHost::State state); I think I suggested this before: make this a static function in ConnectionToHost (i.e. ConnectionToHost::StateToString()) as this is where the State enum is defined. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.cc (right): https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:314: default: nit: default case should be marked as NOTREACHED() as we never expect this (and you don't need the LOG(ERROR)) move default case out of the switch() statement, i.e. switch (state) { .. } NOTREACHED(); return nullptr; This way the compiler will warn about missing cases when a new value is added in the enum. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:320: const char* ErrorCodeToString(ErrorCode error) { This function is declared in remoting/protocol/errors.h , so it doesn't belong to this file. Please create errors.cc and move this function there. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:332: default: same as the previous comment about default case.
Created errors.cc and moved around switch statements. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:111: const char* ConnectionStateToString(ConnectionToHost::State state); On 2015/07/30 21:00:42, Sergey Ulanov wrote: > I think I suggested this before: make this a static function in ConnectionToHost > (i.e. ConnectionToHost::StateToString()) as this is where the State enum is > defined. Done. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.cc (right): https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:314: default: On 2015/07/30 21:00:42, Sergey Ulanov wrote: > nit: default case should be marked as NOTREACHED() as we never expect this (and > you don't need the LOG(ERROR)) > move default case out of the switch() statement, i.e. > switch (state) { > .. > } > NOTREACHED(); > return nullptr; > > This way the compiler will warn about missing cases when a new value is added in > the enum. Done. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:320: const char* ErrorCodeToString(ErrorCode error) { On 2015/07/30 21:00:42, Sergey Ulanov wrote: > This function is declared in remoting/protocol/errors.h , so it doesn't belong > to this file. Please create errors.cc and move this function there. Done. https://codereview.chromium.org/1238343002/diff/320001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:332: default: On 2015/07/30 21:00:42, Sergey Ulanov wrote: > same as the previous comment about default case. Done.
lgtm when my nits are addressed. https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:11: #include "base/logging.h" nit: don't need this include. https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.cc (right): https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:25: #define RETURN_STRING_LITERAL(x) \ nit: move this next to StateToString() https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/erro... File remoting/protocol/errors.h (right): https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/erro... remoting/protocol/errors.h:8: #include "base/logging.h" nit: don't need this include.
Removed unused headers. https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... File remoting/protocol/connection_to_host.h (right): https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... remoting/protocol/connection_to_host.h:11: #include "base/logging.h" On 2015/07/30 22:00:44, Sergey Ulanov wrote: > nit: don't need this include. Removed. https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... File remoting/protocol/connection_to_host_impl.cc (right): https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/conn... remoting/protocol/connection_to_host_impl.cc:25: #define RETURN_STRING_LITERAL(x) \ On 2015/07/30 22:00:44, Sergey Ulanov wrote: > nit: move this next to StateToString() Done. https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/erro... File remoting/protocol/errors.h (right): https://codereview.chromium.org/1238343002/diff/340001/remoting/protocol/erro... remoting/protocol/errors.h:8: #include "base/logging.h" On 2015/07/30 22:00:44, Sergey Ulanov wrote: > nit: don't need this include. Removed.
The CQ bit was checked by tonychun@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1238343002/#ps360001 (title: "Removed unused headers.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1238343002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1238343002/360001
Message was sent while issue was closed.
Committed patchset #12 (id:360001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/684d1414ae40738e455092831088fbc0391ab11d Cr-Commit-Position: refs/heads/master@{#341232} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
