Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1237093004:
Support for connecting to localhost on the chromoting test driver. (Closed)
DescriptionSupport for connecting to localhost on the chromoting test driver.
Completed connection to localhost. I've added a new StartConnection method in TestChromotingClient which reuses almost all of the code provided to create a chromoting connection. In addition, I've added a new parameter called "client-pin", which will allow for the user to pass in a PIN to authenticate the connection.
BUG=
Committed: https://crrev.com/f21b41ef4aa2b84f0be11fdc95c47cf54e543e1f
Cr-Commit-Position: refs/heads/master@{#339504}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Refactored App Remoting and Chromoting code to share StartConnection method. #
Total comments: 30
Patch Set 3 : Cleaned up ConnectionSetupInfo, comments, and merge changes. #
Total comments: 30
Patch Set 4 : Edited merge errors, comments, and optimized code. #
Total comments: 20
Patch Set 5 : Made changes to comments and variable name. #
Total comments: 10
Patch Set 6 : Made changes to comments and removed unused headers. #
Total comments: 3
Patch Set 7 : Made changes to unittest to use ConnectionSetupInfo. #
Total comments: 1
Patch Set 8 : Indent fix. #
Messages
Total messages: 26 (5 generated)
tonychun@google.com changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
Added support to connect to a remote host (that does not have third party authentication) through TestChromotingClient.
https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:124: 'test/host_info.h', Why were the chromoting and app remoting specific files moved to the common target? I don't think this is necessary... https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:141: '../remoting/proto/chromotocol.gyp:chromotocol_proto_lib', Shouldn't this go in the common section since it is shared by both apps? https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:28: const char kClientPINSwitchName[] = "client-pin"; kClientPinSwitchName, also, is this a client pin? Isn't it passed to the host? https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:238: // For some connection, a client_pin is not required. Thus, I did not make it s/connection/connections https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:239: // a requirement. Actually, I'm wondering if this comment is needed. I think that it is evident that the param is not required since it is in the optional section of the help output and you aren't checking its value here. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:298: base::Timer* timer = new base::Timer(true, false); If this needs to be a pointer then use a scoped_ptr, but I think this can live on the stack. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:303: for (auto host_info : hostlist) { Instead of using a loop here, just use find() on hostlist, retrieve the index, then create a connection if it was found. No need for looping AFAICT https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.cc:37: const char kChromotingCapabilities[] = ""; This isn't really providing any value. This probably should be a param passed in to StartConnection(). https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.cc:70: // TODO(TonyChun): Pairing currently not supported for testing. Why include this code? The condition below actually succeeds if the binder and caller of this function both pass false. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.cc:169: const HostInfo& host_info) { There is a lot of duplication here, can you refactor the StartConnection method so that it shares code between Chromoting and App Remoting? https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.h:51: // or a PIN to complete the process. Can you remove the comments you added, I don't think implementation details need to be specified here. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.h:55: const HostInfo& host_info); Please add unit tests for any new functionality added to an existing class.
Refactored code so that Chromoting and App Remoting both use the same StartConnection() method. https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:124: 'test/host_info.h', On 2015/07/14 19:58:44, joedow wrote: > Why were the chromoting and app remoting specific files moved to the common > target? I don't think this is necessary... I have made a new struct called ConnectionInfo which takes away the need handle different types of hosts (from app remoting and chromoting). They weren't very different to begin with, so this was relatively simple. https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:141: '../remoting/proto/chromotocol.gyp:chromotocol_proto_lib', On 2015/07/14 19:58:44, joedow wrote: > Shouldn't this go in the common section since it is shared by both apps? I tried moving this dependency up to 'remoting_test_driver_common', but for some odd reason, neither driver builds if I do this. Perhaps we can go over this to find some work around. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:28: const char kClientPINSwitchName[] = "client-pin"; On 2015/07/14 19:58:44, joedow wrote: > kClientPinSwitchName, also, is this a client pin? Isn't it passed to the host? Changed it to kPinSwitchName. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:238: // For some connection, a client_pin is not required. Thus, I did not make it On 2015/07/14 19:58:44, joedow wrote: > s/connection/connections Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:239: // a requirement. On 2015/07/14 19:58:44, joedow wrote: > Actually, I'm wondering if this comment is needed. I think that it is evident > that the param is not required since it is in the optional section of the help > output and you aren't checking its value here. Removed. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:298: base::Timer* timer = new base::Timer(true, false); On 2015/07/14 19:58:44, joedow wrote: > If this needs to be a pointer then use a scoped_ptr, but I think this can live > on the stack. Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:303: for (auto host_info : hostlist) { On 2015/07/14 19:58:44, joedow wrote: > Instead of using a loop here, just use find() on hostlist, retrieve the index, > then create a connection if it was found. No need for looping AFAICT Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.cc:37: const char kChromotingCapabilities[] = ""; On 2015/07/14 19:58:44, joedow wrote: > This isn't really providing any value. This probably should be a param passed > in to StartConnection(). Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.cc:70: // TODO(TonyChun): Pairing currently not supported for testing. On 2015/07/14 19:58:44, joedow wrote: > Why include this code? The condition below actually succeeds if the binder and > caller of this function both pass false. Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.cc:169: const HostInfo& host_info) { On 2015/07/14 19:58:44, joedow wrote: > There is a lot of duplication here, can you refactor the StartConnection method > so that it shares code between Chromoting and App Remoting? Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.h:51: // or a PIN to complete the process. On 2015/07/14 19:58:44, joedow wrote: > Can you remove the comments you added, I don't think implementation details need > to be specified here. Done. https://codereview.chromium.org/1237093004/diff/1/remoting/test/test_chromoti... remoting/test/test_chromoting_client.h:55: const HostInfo& host_info); On 2015/07/14 19:58:44, joedow wrote: > Please add unit tests for any new functionality added to an existing class. Done.
anandc@chromium.org changed reviewers: + anandc@chromium.org
https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:141: '../remoting/proto/chromotocol.gyp:chromotocol_proto_lib', On 2015/07/15 17:32:50, tonychun wrote: > On 2015/07/14 19:58:44, joedow wrote: > > Shouldn't this go in the common section since it is shared by both apps? > > I tried moving this dependency up to 'remoting_test_driver_common', but for some > odd reason, neither driver builds if I do this. Perhaps we can go over this to > find some work around. How about moving it to remoting_test_common? https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:301: // Find the requested online host info and initiate a session. "Check if requested host is online and ready to receive connections." https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:308: // Total time to connect to a remote host before aborting the connection. "Host is online and ready, initiate a remote session." https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:309: timer->Start(FROM_HERE, base::TimeDelta::FromSeconds(10), Make it a constant? Or explain the reason for using 10. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... File remoting/test/connection_info.h (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.h:14: struct ConnectionInfo { "ConnectionInfo" reads like it holds the information for an already established connection. Maybe "ConnectionSetupInfo"? https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.h:28: // Aoo Remoting information. Typo. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.h:35: std::string host_id; Why is this "common" and not "Chromoting host" information? Same for host jid. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/host_info... remoting/test/host_info.cc:79: return status == kHostStatusOnline; If all this is doing is this 1 check, why have a separate method for it? Also, a host may be online but not ready for connections if it hasn't sent a heartbeat to the directory server. See https://code.google.com/p/chromium/codesearch#chromium/src/remoting/test/host... for a more comprehensive check. Not sure if that applies here. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.h:49: // Starts a chromoting connection with the specified remote host. Starts a Chromoting connection "to" the specified remote host. Or Starts a Chromoting connection "using the specified connection-setup info."
https://codereview.chromium.org/1237093004/diff/20001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/remoting_test.... remoting/remoting_test.gypi:126: 'test/connection_info.h', sync with Sergey on this as he has a change that will conflict here. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... File remoting/test/connection_info.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.cc:7: #include "base/logging.h" is this header really needed in this file? https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:30: #include "remoting/test/remote_host_info_fetcher.h" This header seems like it is not needed https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:60: bool pairing_supported, is pairing_supported needed? You bind true to it but don't use it. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:208: connection_info.authorization_code, indentation seems off here, either 4 spaces or line up with the ampersand on FetchThirdPartyToken https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:212: // Parse |auth_methods_str| for authentication methods. Why build this here? This feels like something that could be built when generating the ConnectionInfo struct, then you wouldn't need the string parsing here, you could just create the vector there and assign it here. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:231: std::string(), // shared_secret you can pass the connection_info members for pairing_id and shared_secret here can't you? They are just going to be empty strings plus this function will support other connection methods as long as the info struct is set up correctly.
Merged with Sergey's code and cleaned up comments and ConnectionSetupInfo. https://codereview.chromium.org/1237093004/diff/20001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/remoting_test.... remoting/remoting_test.gypi:126: 'test/connection_info.h', On 2015/07/16 03:23:19, joedow wrote: > sync with Sergey on this as he has a change that will conflict here. I talked to Sergey and I'll take care of the merge when he lands his changes, that is if his changes land before mine. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:301: // Find the requested online host info and initiate a session. On 2015/07/15 21:12:29, anandc wrote: > "Check if requested host is online and ready to receive connections." Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:308: // Total time to connect to a remote host before aborting the connection. On 2015/07/15 21:12:29, anandc wrote: > "Host is online and ready, initiate a remote session." Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:309: timer->Start(FROM_HERE, base::TimeDelta::FromSeconds(10), On 2015/07/15 21:12:29, anandc wrote: > Make it a constant? Or explain the reason for using 10. Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... File remoting/test/connection_info.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.cc:7: #include "base/logging.h" On 2015/07/16 03:23:19, joedow wrote: > is this header really needed in this file? Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... File remoting/test/connection_info.h (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.h:14: struct ConnectionInfo { On 2015/07/15 21:12:29, anandc wrote: > "ConnectionInfo" reads like it holds the information for an already established > connection. > Maybe "ConnectionSetupInfo"? Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.h:28: // Aoo Remoting information. On 2015/07/15 21:12:29, anandc wrote: > Typo. Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/connectio... remoting/test/connection_info.h:35: std::string host_id; On 2015/07/15 21:12:29, anandc wrote: > Why is this "common" and not "Chromoting host" information? Same for host jid. Because the host_id and host_jid are also part of the RemoteHostInfo struct for app remoting. https://code.google.com/p/chromium/codesearch#chromium/src/remoting/test/remo... Is there a reason I shouldn't list it as common? https://codereview.chromium.org/1237093004/diff/20001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/host_info... remoting/test/host_info.cc:79: return status == kHostStatusOnline; On 2015/07/15 21:12:29, anandc wrote: > If all this is doing is this 1 check, why have a separate method for it? > Also, a host may be online but not ready for connections if it hasn't sent a > heartbeat to the directory server. > See > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/test/host... > for a more comprehensive check. > Not sure if that applies here. Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:30: #include "remoting/test/remote_host_info_fetcher.h" On 2015/07/16 03:23:19, joedow wrote: > This header seems like it is not needed Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:60: bool pairing_supported, On 2015/07/16 03:23:19, joedow wrote: > is pairing_supported needed? You bind true to it but don't use it. Removed the unused parameter. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:208: connection_info.authorization_code, On 2015/07/16 03:23:19, joedow wrote: > indentation seems off here, either 4 spaces or line up with the ampersand on > FetchThirdPartyToken Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:212: // Parse |auth_methods_str| for authentication methods. On 2015/07/16 03:23:19, joedow wrote: > Why build this here? This feels like something that could be built when > generating the ConnectionInfo struct, then you wouldn't need the string parsing > here, you could just create the vector there and assign it here. Done. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:231: std::string(), // shared_secret On 2015/07/16 03:23:19, joedow wrote: > you can pass the connection_info members for pairing_id and shared_secret here > can't you? They are just going to be empty strings plus this function will > support other connection methods as long as the info struct is set up correctly. I've added a new member in ConnectionSetupInfo (newly named) called pairing_id. https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/20001/remoting/test/test_chro... remoting/test/test_chromoting_client.h:49: // Starts a chromoting connection with the specified remote host. On 2015/07/15 21:12:29, anandc wrote: > Starts a Chromoting connection "to" the specified remote host. > Or Starts a Chromoting connection "using the specified connection-setup info." Done.
https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:79: # Files from remoting_test_common not in separate test_support targets. Please fix this merge issue (sergey removed this section) https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:60: 'test/app_remoting_test_driver_environment_app_details.cc', Same comments as the other CL, please make sure all of the targets build and merge conflicts are handled appropriately https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:115: 'target_name': 'remoting_test_driver_common', Pretty sure this was removed, please make sure this CL is cleanly merged with Sergey's changes. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:42: const unsigned int kTimeToConnectToHost = 10; newline between this value and the one above since they are unrelated? https://codereview.chromium.org/1237093004/diff/60001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:102: for (std::string part : parts) { I don't think you need to do any string parsing here, you should be able to create a vector and push_back the application auth methods. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/remote_ho... File remoting/test/remote_host_info.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:66: connection_setup_info.auth_methods.push_back(auth_method); same as the chromoting version, please remove the string parsing bits and populate the vector linearly.
https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:78: sources = [ This looks like a failed merge. You want to add connection_setup_info.cc in remoting/test/BUILD.gn and remove sources=[] from this file. https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:61: 'test/app_remoting_test_driver_environment.h', Please remove these 3 files - it would fail to compile with the internal test driver. https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:115: 'target_name': 'remoting_test_driver_common', I removed this target in my CL. It's not necessary. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:42: const unsigned int kTimeToConnectToHost = 10; Is that the maximum allowed time? If so, maybe call it kConnectionTimeout or something like that? https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:282: // Reset runloop to use it to retrieve hostlist from directory service. I don't think you need this comment. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:298: make_scoped_ptr<base::Timer>(new base::Timer(true, false)); This can be allocated on stack: base::Timer timer(true, false); Also it can be moved inside the if block below. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:308: if (it != hostlist.end()) { Log an error if the host wasn't found https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:316: timer->Stop(); Don't need this if you scope |timer| inside the if block https://codereview.chromium.org/1237093004/diff/60001/remoting/test/connectio... File remoting/test/connection_setup_info.h (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/connectio... remoting/test/connection_setup_info.h:16: // Holds the information needed to establish a connection with a remote host. Also mention that it's used to pass parameters to TestChromotingClient. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/test_chro... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/test_chro... remoting/test/test_chromoting_client.h:17: #include "remoting/test/connection_setup_info.h" ConnectionSetupInfo is passed by reference so it can be forward-declared instead of including this file here.
Fixed merge errors, comments, and optimized code. https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:78: sources = [ On 2015/07/16 23:42:09, Sergey Ulanov wrote: > This looks like a failed merge. > > You want to add connection_setup_info.cc in remoting/test/BUILD.gn and remove > sources=[] from this file. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:79: # Files from remoting_test_common not in separate test_support targets. On 2015/07/16 23:26:33, joedow wrote: > Please fix this merge issue (sergey removed this section) Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:60: 'test/app_remoting_test_driver_environment_app_details.cc', On 2015/07/16 23:26:33, joedow wrote: > Same comments as the other CL, please make sure all of the targets build and > merge conflicts are handled appropriately Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:61: 'test/app_remoting_test_driver_environment.h', On 2015/07/16 23:42:09, Sergey Ulanov wrote: > Please remove these 3 files - it would fail to compile with the internal test > driver. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/remoting_test.... remoting/remoting_test.gypi:115: 'target_name': 'remoting_test_driver_common', On 2015/07/16 23:42:09, Sergey Ulanov wrote: > I removed this target in my CL. It's not necessary. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:42: const unsigned int kTimeToConnectToHost = 10; On 2015/07/16 23:42:09, Sergey Ulanov wrote: > Is that the maximum allowed time? If so, maybe call it kConnectionTimeout or > something like that? Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:282: // Reset runloop to use it to retrieve hostlist from directory service. On 2015/07/16 23:42:09, Sergey Ulanov wrote: > I don't think you need this comment. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:298: make_scoped_ptr<base::Timer>(new base::Timer(true, false)); On 2015/07/16 23:42:09, Sergey Ulanov wrote: > This can be allocated on stack: > base::Timer timer(true, false); > Also it can be moved inside the if block below. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:308: if (it != hostlist.end()) { On 2015/07/16 23:42:10, Sergey Ulanov wrote: > Log an error if the host wasn't found Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:316: timer->Stop(); On 2015/07/16 23:42:09, Sergey Ulanov wrote: > Don't need this if you scope |timer| inside the if block Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/connectio... File remoting/test/connection_setup_info.h (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/connectio... remoting/test/connection_setup_info.h:16: // Holds the information needed to establish a connection with a remote host. On 2015/07/16 23:42:10, Sergey Ulanov wrote: > Also mention that it's used to pass parameters to TestChromotingClient. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:102: for (std::string part : parts) { On 2015/07/16 23:26:34, joedow wrote: > I don't think you need to do any string parsing here, you should be able to > create a vector and push_back the application auth methods. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/remote_ho... File remoting/test/remote_host_info.cc (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:66: connection_setup_info.auth_methods.push_back(auth_method); On 2015/07/16 23:26:34, joedow wrote: > same as the chromoting version, please remove the string parsing bits and > populate the vector linearly. Done. https://codereview.chromium.org/1237093004/diff/60001/remoting/test/test_chro... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/60001/remoting/test/test_chro... remoting/test/test_chromoting_client.h:17: #include "remoting/test/connection_setup_info.h" On 2015/07/16 23:42:10, Sergey Ulanov wrote: > ConnectionSetupInfo is passed by reference so it can be forward-declared instead > of including this file here. Done.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1237093004/diff/80001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:43: const unsigned int kConnectionTimeout = 10; You should add a suffix to indicate the units being represented. e.g. kConnectionTimeoutSeconds https://codereview.chromium.org/1237093004/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:316: LOG(ERROR) << "Requested host not found or ready to connect"; nit s/or ready to connect/or not ready for connection. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.cc:5: remove extra newline https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.cc:6: #include "base/strings/string_split.h" Is the string split header still needed? It seems like it isn't being used. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.cc:91: connection_setup_info.user_name = user_name; Can you some newlines here, one after line 84 would be good (separate the var definition and when you start initializing it) and then another after line 91 to break up the auth method init lines from the string member initialization. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info.h File remoting/test/host_info.h (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.h:28: // Returns true if the remote host is ready to accept connections. remote host or chromoting host? Since this is a host_info struct you may want to say chromoting host to be specific https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... File remoting/test/remote_host_info.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:5: remove extra newline https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:6: #include "base/strings/string_split.h" remove string_split header https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:57: protocol::AuthenticationMethod::ThirdParty()); Please add some newlines as in host_info.cc https://codereview.chromium.org/1237093004/diff/80001/remoting/test/test_chro... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:211: } add newline here please
Cleaned up unused headers and added new lines for readability. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:43: const unsigned int kConnectionTimeout = 10; On 2015/07/17 23:26:29, joedow wrote: > You should add a suffix to indicate the units being represented. e.g. > kConnectionTimeoutSeconds Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:316: LOG(ERROR) << "Requested host not found or ready to connect"; On 2015/07/17 23:26:29, joedow wrote: > nit s/or ready to connect/or not ready for connection. Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.cc:5: On 2015/07/17 23:26:29, joedow wrote: > remove extra newline Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.cc:6: #include "base/strings/string_split.h" On 2015/07/17 23:26:30, joedow wrote: > Is the string split header still needed? It seems like it isn't being used. Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.cc:91: connection_setup_info.user_name = user_name; On 2015/07/17 23:26:29, joedow wrote: > Can you some newlines here, one after line 84 would be good (separate the var > definition and when you start initializing it) and then another after line 91 to > break up the auth method init lines from the string member initialization. Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info.h File remoting/test/host_info.h (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/host_info... remoting/test/host_info.h:28: // Returns true if the remote host is ready to accept connections. On 2015/07/17 23:26:30, joedow wrote: > remote host or chromoting host? Since this is a host_info struct you may want > to say chromoting host to be specific Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... File remoting/test/remote_host_info.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:5: On 2015/07/17 23:26:30, joedow wrote: > remove extra newline Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:6: #include "base/strings/string_split.h" On 2015/07/17 23:26:30, joedow wrote: > remove string_split header Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/remote_ho... remoting/test/remote_host_info.cc:57: protocol::AuthenticationMethod::ThirdParty()); On 2015/07/17 23:26:30, joedow wrote: > Please add some newlines as in host_info.cc Done. https://codereview.chromium.org/1237093004/diff/80001/remoting/test/test_chro... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/80001/remoting/test/test_chro... remoting/test/test_chromoting_client.cc:211: } On 2015/07/17 23:26:30, joedow wrote: > add newline here please Done.
https://codereview.chromium.org/1237093004/diff/100001/remoting/remoting_test... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/remoting_test... remoting/remoting_test.gypi:108: # A chromoting version of remoting_test_driver_common This comment looks wrong,I would just remove it, even if it were updated I don't think it provides much value. https://codereview.chromium.org/1237093004/diff/100001/remoting/test/connecti... File remoting/test/connection_setup_info.h (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/test/connecti... remoting/test/connection_setup_info.h:17: // Used to pass parameters to TestChromotingClient. I'd remove the second line. Your intention is to use this struct to pass data to a TestChromotingClient, but it could be used in a different way later and no one is going to know to come in and update this comment :) https://codereview.chromium.org/1237093004/diff/100001/remoting/test/remote_h... File remoting/test/remote_host_info.cc (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/test/remote_h... remoting/test/remote_host_info.cc:5: please remove newline. Didn't I make this comment last week for this review? https://codereview.chromium.org/1237093004/diff/100001/remoting/test/remote_h... remoting/test/remote_host_info.cc:6: #include "base/strings/string_split.h" string_split isn't needed, please remove. I seem to recall this as well from last week. https://codereview.chromium.org/1237093004/diff/100001/remoting/test/test_chr... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/test/test_chr... remoting/test/test_chromoting_client.h:18: #include "remoting/test/remote_host_info.h" You can remove remote_host_info from here since you are using a ConnectionSetupInfo struct now.
Removed comments and unused headers. https://codereview.chromium.org/1237093004/diff/100001/remoting/remoting_test... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/remoting_test... remoting/remoting_test.gypi:108: # A chromoting version of remoting_test_driver_common On 2015/07/20 16:39:53, joedow wrote: > This comment looks wrong,I would just remove it, even if it were updated I don't > think it provides much value. Done. https://codereview.chromium.org/1237093004/diff/100001/remoting/test/connecti... File remoting/test/connection_setup_info.h (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/test/connecti... remoting/test/connection_setup_info.h:17: // Used to pass parameters to TestChromotingClient. On 2015/07/20 16:39:53, joedow wrote: > I'd remove the second line. Your intention is to use this struct to pass data > to a TestChromotingClient, but it could be used in a different way later and no > one is going to know to come in and update this comment :) Done. https://codereview.chromium.org/1237093004/diff/100001/remoting/test/remote_h... File remoting/test/remote_host_info.cc (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/test/remote_h... remoting/test/remote_host_info.cc:5: On 2015/07/20 16:39:53, joedow wrote: > please remove newline. Didn't I make this comment last week for this review? This is magic. (I know I removed it, but CLs don't lie. A million trillion apologies for making you waste time on this.) https://codereview.chromium.org/1237093004/diff/100001/remoting/test/remote_h... remoting/test/remote_host_info.cc:6: #include "base/strings/string_split.h" On 2015/07/20 16:39:53, joedow wrote: > string_split isn't needed, please remove. I seem to recall this as well from > last week. This is magic. https://codereview.chromium.org/1237093004/diff/100001/remoting/test/test_chr... File remoting/test/test_chromoting_client.h (right): https://codereview.chromium.org/1237093004/diff/100001/remoting/test/test_chr... remoting/test/test_chromoting_client.h:18: #include "remoting/test/remote_host_info.h" On 2015/07/20 16:39:53, joedow wrote: > You can remove remote_host_info from here since you are using a > ConnectionSetupInfo struct now. Done. https://codereview.chromium.org/1237093004/diff/120001/remoting/test/test_chr... File remoting/test/test_chromoting_client.cc (right): https://codereview.chromium.org/1237093004/diff/120001/remoting/test/test_chr... remoting/test/test_chromoting_client.cc:28: #include "remoting/test/connection_setup_info.h" This was originally in remote_host_info.h, but it's moved to here now. https://codereview.chromium.org/1237093004/diff/120001/remoting/test/test_chr... File remoting/test/test_chromoting_client_unittest.cc (right): https://codereview.chromium.org/1237093004/diff/120001/remoting/test/test_chr... remoting/test/test_chromoting_client_unittest.cc:9: #include "remoting/test/remote_host_info.h" Tests use remote_host_info
https://codereview.chromium.org/1237093004/diff/120001/remoting/test/test_chr... File remoting/test/test_chromoting_client_unittest.cc (right): https://codereview.chromium.org/1237093004/diff/120001/remoting/test/test_chr... remoting/test/test_chromoting_client_unittest.cc:9: #include "remoting/test/remote_host_info.h" Sorry for the late comment on this file, but I'm wondering why the unittests don't use a ConnectionSetupInfo struct here instead of a remote host info? Let's remove the RHI dependency here and use the struct that you created.
Made changes to support ConnectionSetupInfo in unittest.
lgtm
lgtm https://codereview.chromium.org/1237093004/diff/140001/remoting/test/chromoti... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1237093004/diff/140001/remoting/test/chromoti... remoting/test/chromoting_test_driver.cc:309: base::TimeDelta::FromSeconds(kConnectionTimeoutSeconds), nit: indentation.
The CQ bit was checked by tonychun@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/1237093004/#ps160001 (title: "Indent fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1237093004/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f21b41ef4aa2b84f0be11fdc95c47cf54e543e1f Cr-Commit-Position: refs/heads/master@{#339504} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
