Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1212333011:
Retrieved Hostlist information from Directory service. (Closed)
DescriptionAdding the ChromotingHostListFetcher and its respective unittests.
The ChromotingHostListFetcher requests a host list from the Directory service (currently a REST call). The rest call is parsed and creates a ChromotingHostInfo struct instance, which encapsulates important host information. Once the host list of ChromotingHostInfos is created, it is returned to the callback.
BUG=
Committed: https://crrev.com/e3fe98489be2fe1f9e504fa5c6ac1f2e205fd561
Cr-Commit-Position: refs/heads/master@{#338328}
Patch Set 1 #
Total comments: 75
Patch Set 2 : Added unittests and cleaned up format of C++ code. #
Total comments: 115
Patch Set 3 : Cleaned up fetcher code and added more unit test coverage. #
Total comments: 35
Patch Set 4 : Changed file names to remove ambiguity and cleaned up code. #
Total comments: 26
Patch Set 5 : Cleaned up code and changed DVLOG to VLOG. #
Total comments: 4
Patch Set 6 : Modified DVLOG to LOG and change constant naming in unittest. #Patch Set 7 : Final clean up. #Patch Set 8 : Added missing offline_host check. #
Messages
Total messages: 30 (7 generated)
tonychun@google.com changed reviewers: + joedow@chromium.org
Please run cpplint.py, run clang-format, and add unit tests for next revision. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:17: if(listValue != nullptr) { add space between if and open paren https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:21: for(int i = 0; i < size; ++i) { add space between for and open paren (please run cpplint.py) https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:22: listValue->GetString(i,&tokenUrlPattern); add space between comma and ampersand https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:23: if(!tokenUrlPattern.empty()) { Add space between if and open paren (please run cpplint.py) https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:30: item.GetString("hostId", &id); tokenUrlPatterns is under 'data' per the code on line 14, are the rest of the params siblings to 'data' or is there a namespace bug here? https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:38: is_online = (status == "ONLINE") ? true : false; Are there other statuses? I'm thinking that an enum of the statuses would be more useful than an is_online member (interested consumers can check for a specific status such as disabled without you have to extend the struct). https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:38: is_online = (status == "ONLINE") ? true : false; "ONLINE" should be a constant in a namespace at the top of the file for the comparison either way. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:17: ChromotingHostInfo(const base::DictionaryValue& item); Single argument constructors must be marked explicit. Have you run cpplint.py on the C++ files you changed/added? It is in the depot_tools folder and will catch stuff like this before the code goes out for review. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:20: std::vector<std::string> tokenUrlPatterns; nit: why is this the first member? It seems like this should be lower down in the list (with host_id being first) https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:21: std::string id; nit: host_id and host_jid would be more readable. The struct is HostInfo, but someone will probably create an instance of the struct and call it 'info' and then this member will be info.id which is not readable. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:26: bool is_online; nit: move this above 'offline_reason'? https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:64: printf(" chromoting_test_driver --username=<example@gmail.com> [options]\n"); Doesn't hostname need to be included in the example usage as well? https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:78: " %s: Specifies the optional logging level of the tool (0-3)." Why is this indented down? I think there is enough space to move it up. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:117: switches::kUserNameSwitchName, switches::kAuthCodeSwitchName); hostname should be shown here, to reduce confusion. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:136: switches::kRefreshTokenPathSwitchName); hostname? https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:142: std::string temp_access_token; A cleaner way to do this would be to add an extra var to your OnAccessTokenRetrieved callback which is a std::string*, then you can bind that pointer to your callback as the first param (bind supports currying) and then you don't need this here. Just saw that you did this for OnHostlistRetrieved, do it here too! https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:164: DCHECK(hostlist); https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:251: base::RunLoop access_token_run_loop; nit: if you use a scoped_ptr<base::RunLoop> you can reset it before each logical chunk of code that needs a new RunLoop. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:267: // Thread to handle callback from Directory service. This isn't a thread, it's a runloop which just yields to the thread's messageloop. Please update the comment. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... File remoting/test/hostlist_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:7: #include <iostream> You should not be using iostream, use printf or DVLOG/LOG macros instead. Why was this included anyway? https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:13: #include "base/message_loop/message_loop.h" Where is message_loop used? Remove it if not used. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:14: #include "base/strings/stringprintf.h" remove header and StringPrintf call since it isn't needed https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:34: // Message the Directory service for the hostlist I don't think this comment is useful, please remove. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:36: Please add some additional DCHECKS here. You should validate that access_token is not empty, service_environment is valid, callback is not null, and hostlist_callback_ is null (i.e. you are not in the middle of a request). https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:37: Why so many empty newlines? Please reduce to one. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:43: service_url = base::StringPrintf(kProdServiceEnvironmentUrlFormat); Why are you using StringPrintf here? You aren't replacing any values. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:61: request_->SetUploadData("application/json; charset=UTF-8", "{}"); Is this line needed for this call? I'm not sure this is used at all. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:76: base::ResetAndReturn(&hostlist_callback_).Run(hostlist); I think a ScopedClosureRunner() would make the multiple return statements cleaner, you'd have to do a little work though to create a closure which binds the hostlist param. It would make everything cleaner though. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:103: // add hosts to hostlist Is this comment useful? Please remove it or update it to conform to style guide (Add hosts to hostlist.) https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:107: int size = items->GetSize(); What if items is a nullptr, you should make sure it is valid before dereferencing. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:109: const base::DictionaryValue* item; it's a good idea to initialize pointers to nullptr even if you overwrite that value. Later on, the code may be moved around and a bug could be introduced. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:111: items->GetDictionary(i,&item); add space between comma and ampersand (and run cpplint.py) https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... File remoting/test/hostlist_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:29: const char kProdServiceEnvironmentUrlFormat[] = "https://www.googleapis.com/" Can you rename this? It isn't a format string because it has no placeholders, this is just an Url. kChromotingHostListProdRequestUrl is a better name. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:35: enum ServiceEnvironment { This is going to collide with the app remoting ServiceEnvironment struct if I ever add a Prod environment to that list. I vote that you simplify the chromoting side of the code and only target prod. If the team ever decides to add another environment, the code can be updated. For now, this tool is only going to target prod so why have extra complexity. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:41: // initialized Hostlist object on success. The callback doesn't return a Hostlist object, it updates the hostlist vector that is passed in. Since hostlist is actually an out param, I think it should be a pointer (per style guide). https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:45: // Calls the App Remoting service API to request connection info for a remote I'm guessing this class does not call the App Remoting Service API :) Can you update the comments please? https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:50: class HostlistFetcher : public net::URLFetcherDelegate { Can you rename this class ChromotingHostListFetcher (the files should be renamed to chromoting_host_list_fetcher.*) https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:65: // Holds the URLFetcher for the RemoteHostInfo request. RemoteHostInfo request?
Added unit test for chromoting host list fetcher and cleaned up C++ code. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:17: if(listValue != nullptr) { On 2015/07/01 03:39:33, joedow wrote: > add space between if and open paren Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:21: for(int i = 0; i < size; ++i) { On 2015/07/01 03:39:33, joedow wrote: > add space between for and open paren (please run cpplint.py) Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:22: listValue->GetString(i,&tokenUrlPattern); On 2015/07/01 03:39:33, joedow wrote: > add space between comma and ampersand Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:23: if(!tokenUrlPattern.empty()) { On 2015/07/01 03:39:33, joedow wrote: > Add space between if and open paren (please run cpplint.py) Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:30: item.GetString("hostId", &id); On 2015/07/01 03:39:33, joedow wrote: > tokenUrlPatterns is under 'data' per the code on line 14, are the rest of the > params siblings to 'data' or is there a namespace bug here? This is a bug. My unit test would have flagged this error. Thanks! https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.cc:38: is_online = (status == "ONLINE") ? true : false; On 2015/07/01 03:39:33, joedow wrote: > Are there other statuses? I'm thinking that an enum of the statuses would be > more useful than an is_online member (interested consumers can check for a > specific status such as disabled without you have to extend the struct). Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:17: ChromotingHostInfo(const base::DictionaryValue& item); On 2015/07/01 03:39:33, joedow wrote: > Single argument constructors must be marked explicit. > > Have you run cpplint.py on the C++ files you changed/added? It is in the > depot_tools folder and will catch stuff like this before the code goes out for > review. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:20: std::vector<std::string> tokenUrlPatterns; On 2015/07/01 03:39:34, joedow wrote: > nit: why is this the first member? It seems like this should be lower down in > the list (with host_id being first) Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:21: std::string id; On 2015/07/01 03:39:34, joedow wrote: > nit: host_id and host_jid would be more readable. The struct is HostInfo, but > someone will probably create an instance of the struct and call it 'info' and > then this member will be info.id which is not readable. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_ho... remoting/test/chromoting_host_info.h:26: bool is_online; On 2015/07/01 03:39:33, joedow wrote: > nit: move this above 'offline_reason'? Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:64: printf(" chromoting_test_driver --username=<example@gmail.com> [options]\n"); On 2015/07/01 03:39:34, joedow wrote: > Doesn't hostname need to be included in the example usage as well? Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:78: " %s: Specifies the optional logging level of the tool (0-3)." On 2015/07/01 03:39:34, joedow wrote: > Why is this indented down? I think there is enough space to move it up. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:117: switches::kUserNameSwitchName, switches::kAuthCodeSwitchName); On 2015/07/01 03:39:34, joedow wrote: > hostname should be shown here, to reduce confusion. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:136: switches::kRefreshTokenPathSwitchName); On 2015/07/01 03:39:34, joedow wrote: > hostname? Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:142: std::string temp_access_token; On 2015/07/01 03:39:34, joedow wrote: > A cleaner way to do this would be to add an extra var to your > OnAccessTokenRetrieved callback which is a std::string*, then you can bind that > pointer to your callback as the first param (bind supports currying) and then > you don't need this here. > > Just saw that you did this for OnHostlistRetrieved, do it here too! Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:164: On 2015/07/01 03:39:34, joedow wrote: > DCHECK(hostlist); Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:251: base::RunLoop access_token_run_loop; On 2015/07/01 03:39:34, joedow wrote: > nit: if you use a scoped_ptr<base::RunLoop> you can reset it before each logical > chunk of code that needs a new RunLoop. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver.cc:267: // Thread to handle callback from Directory service. On 2015/07/01 03:39:34, joedow wrote: > This isn't a thread, it's a runloop which just yields to the thread's > messageloop. Please update the comment. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... File remoting/test/hostlist_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:7: #include <iostream> On 2015/07/01 03:39:35, joedow wrote: > You should not be using iostream, use printf or DVLOG/LOG macros instead. Why > was this included anyway? I put it in because I wanted to output the hostlist JSON message. printf has a character limit (at least it did when I tried printing out the content to screen). I have removed it now. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:13: #include "base/message_loop/message_loop.h" On 2015/07/01 03:39:34, joedow wrote: > Where is message_loop used? Remove it if not used. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:14: #include "base/strings/stringprintf.h" On 2015/07/01 03:39:35, joedow wrote: > remove header and StringPrintf call since it isn't needed Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:34: // Message the Directory service for the hostlist On 2015/07/01 03:39:35, joedow wrote: > I don't think this comment is useful, please remove. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:36: On 2015/07/01 03:39:35, joedow wrote: > Please add some additional DCHECKS here. You should validate that access_token > is not empty, service_environment is valid, callback is not null, and > hostlist_callback_ is null (i.e. you are not in the middle of a request). I removed service_environment because it is only targeting prod. I've added the rest of the checks however. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:37: On 2015/07/01 03:39:35, joedow wrote: > Why so many empty newlines? Please reduce to one. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:43: service_url = base::StringPrintf(kProdServiceEnvironmentUrlFormat); On 2015/07/01 03:39:35, joedow wrote: > Why are you using StringPrintf here? You aren't replacing any values. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:61: request_->SetUploadData("application/json; charset=UTF-8", "{}"); On 2015/07/01 03:39:35, joedow wrote: > Is this line needed for this call? I'm not sure this is used at all. Removed. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:76: base::ResetAndReturn(&hostlist_callback_).Run(hostlist); On 2015/07/01 03:39:35, joedow wrote: > I think a ScopedClosureRunner() would make the multiple return statements > cleaner, you'd have to do a little work though to create a closure which binds > the hostlist param. It would make everything cleaner though. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:103: // add hosts to hostlist On 2015/07/01 03:39:35, joedow wrote: > Is this comment useful? Please remove it or update it to conform to style guide > (Add hosts to hostlist.) Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:107: int size = items->GetSize(); On 2015/07/01 03:39:35, joedow wrote: > What if items is a nullptr, you should make sure it is valid before > dereferencing. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:109: const base::DictionaryValue* item; On 2015/07/01 03:39:35, joedow wrote: > it's a good idea to initialize pointers to nullptr even if you overwrite that > value. Later on, the code may be moved around and a bug could be introduced. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.cc:111: items->GetDictionary(i,&item); On 2015/07/01 03:39:35, joedow wrote: > add space between comma and ampersand (and run cpplint.py) Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... File remoting/test/hostlist_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:29: const char kProdServiceEnvironmentUrlFormat[] = "https://www.googleapis.com/" On 2015/07/01 03:39:36, joedow wrote: > Can you rename this? It isn't a format string because it has no placeholders, > this is just an Url. kChromotingHostListProdRequestUrl is a better name. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:35: enum ServiceEnvironment { On 2015/07/01 03:39:36, joedow wrote: > This is going to collide with the app remoting ServiceEnvironment struct if I > ever add a Prod environment to that list. I vote that you simplify the > chromoting side of the code and only target prod. If the team ever decides to > add another environment, the code can be updated. For now, this tool is only > going to target prod so why have extra complexity. Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:41: // initialized Hostlist object on success. On 2015/07/01 03:39:36, joedow wrote: > The callback doesn't return a Hostlist object, it updates the hostlist vector > that is passed in. Since hostlist is actually an out param, I think it should > be a pointer (per style guide). Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:45: // Calls the App Remoting service API to request connection info for a remote On 2015/07/01 03:39:35, joedow wrote: > I'm guessing this class does not call the App Remoting Service API :) Can you > update the comments please? Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:50: class HostlistFetcher : public net::URLFetcherDelegate { On 2015/07/01 03:39:35, joedow wrote: > Can you rename this class ChromotingHostListFetcher (the files should be renamed > to chromoting_host_list_fetcher.*) Done. https://codereview.chromium.org/1212333011/diff/1/remoting/test/hostlist_fetc... remoting/test/hostlist_fetcher.h:65: // Holds the URLFetcher for the RemoteHostInfo request. On 2015/07/01 03:39:36, joedow wrote: > RemoteHostInfo request? Done.
joedow@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:17: ChromotingHostInfo::ChromotingHostInfo(const base::DictionaryValue& item) { instead of item, can you call it host_info or something more descriptive? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:18: const base::ListValue* listValue = nullptr; listValue should be list_value. This codebase does not use camel case for var names. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:22: if (listValue != nullptr) { This is cleaner as 'if (listValue) {' https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:25: std::string tokenUrlPattern; token_url_pattern https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:46: DCHECK(response_status == kOfflineStatusResponse); A switch might be cleaner here (with a default case that logs the response_status value and adds an UNREACHED macro. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:18: kChromotingHostStatusOffline A good practice with enums is to add a value at the end that you can use to test for validity. An example of this is in app_remoting_service_urls.h https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:22: explicit ChromotingHostInfo(const base::DictionaryValue& item); item? can you make this more descriptive? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:31: std::vector<std::string> tokenUrlPatterns; token_url_patterns https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:44: net::URLFetcher::GET, this); Will this line fit on the line above? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:49: return true; Why return a value here if it is always true? I'd update this so it is void and simplify the calling pattern. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:57: std::vector<ChromotingHostInfo> hostlist_; no trailing underscore is used for local vars, only for class members. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:59: // Allows executing a callback after the original callback is Reset(). Can you update the comment a bit, basically state that you are saving off the object's stored callback so you can use a ScopedClosureRunner to ensure the callback is run regardless of when the function returns. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:60: HostlistCallback reset_hostlist_callback(hostlist_callback_); Can you rename reset_hostlist_callback? You could just use 'hostlist_callback' without the trailing underscore since it is a local var. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:62: remove newline https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:63: base::Closure cb = base::Bind(reset_hostlist_callback, &hostlist_); rename cb, something like bound_hostlist_callback would be good. You could also just do this inline in the ScopedClosureRunner which would probably be cleaner. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:63: base::Closure cb = base::Bind(reset_hostlist_callback, &hostlist_); You can use base::ConstRef(hostlist) here instead of a raw pointer. Raw pointers are used for out params, in params are passed by const ref. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:94: remove newline https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:100: const base::ListValue* items = nullptr; would hosts be a better name? items is pretty generic https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:102: remove newline https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:109: const base::DictionaryValue* item = nullptr; host_info instead of item? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:37: // Calls the Directory service to request for a host list for an access token. s/to request for a/to request a https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:47: // Makes a service call to retrieve the details for a hostlist. The Makes a service call to retrieve a hostlist. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:56: // Holds the URLFetcher for the ChromotingHostInfo request. s/ChromotingHostInfo/ChromotingHostList https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:15: remove extra newlines https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:22: std::vector<remoting::test::ChromotingHostInfo>* retrieved_hostlist) { retrieved_hostlist should be const ref https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:65: "}"; kChromotingHostListReadyResponse is a good start, but you probably also want additional scenarios: - empty items dict data: { kind: items: {} } } - no token url patterns (not sure if this is expected or an error) data: { kind: items: { tokenUrlPatterns: [] ...details... } } } - invalid status, possibly missing fields as is interesting https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:76: // ChromotingHostListFetcher. This fixture also creates an IO MessageLoop, if nit: it looks like it always creates a MessageLoop, this fixture doesn't check to see if one exists. That's fine to do, but the comment should be updated. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:137: const unsigned int expectedHLSize = 2; no camel case, this var should be called expected_hostlist_size https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:140: ChromotingHostInfo onlineChromotingHostInfo = hostlist.front(); If you know the size, I would just use array indexing here instead of front/back https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:140: ChromotingHostInfo onlineChromotingHostInfo = hostlist.front(); Also, no camel case. online_chromoting_host_info. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:141: const unsigned int expectedTUPSize = 3; expected_pattern_num? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:143: EXPECT_TRUE(!onlineChromotingHostInfo.host_id.empty()); I think it's more readable when you think the string should be valid to use: EXPECT_FALSE(my_fake_string.empty()); https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:151: ChromotingHostInfo offlineChromotingHostInfo = hostlist.back(); offline_chromoting_host_info https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:162: TEST_F(ChromotingHostListFetcherTest, RetrieveHostListInvalidEnvironment) { The test names is "...InvalidEnvironment" but it looks like this is more of a network error test. Can you fix the name? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:181: You should also add a test which makes two requests using the same hostlist fetcher since it was written to allow that. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:202: // should be populated. "connection details"? Copy/paste error? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:160: std::vector<remoting::test::ChromotingHostInfo>* retrieved_hostlist) { retrieved_hostlist should be const ref https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:162: DVLOG(1) << "OnHostlistRetrieved() Called"; FYI, I've switched over to using VLOG instead of DVLOG as we run release bits on our waterfall and DVLOG will make it difficult to debug failures. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:260: remove newline https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:277: // RunLoop to handle callback from Directory service. update comment "Reset runloop to use it to retrieve hostlist from directory service." https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:281: remove newline https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:282: remoting::test::HostlistCallback hostlist_fetch_callback = base::Bind( hostlist_fetch_callback can be rename hostlist_request_callback. Technically the callback is used for a single request whereas the fetcher can be used multiple times. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:283: &OnHostlistRetrieved, run_loop->QuitClosure(), &hostlist); base::ConstRef(hostlist) https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:287: Remove newline
https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:10: const char kOnlineStatusResponse[] = "ONLINE"; In general it's not useful to declare consts for strings like this, except when they are used in multiple places. So I suggest moving "ONLINE" inline - that would make the code more readable. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:22: if (listValue != nullptr) { On 2015/07/06 22:19:12, joedow wrote: > This is cleaner as 'if (listValue) {' DictionaryValue::GetList() returns bool, so this can be if (item.GetList("tokenUrlPatterns", &listValue)) { ... } https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:35: item.GetString("hostId", &host_id); Need to handle the case when any of the fields is missing. To do that you need to be able to return errors from this function, and it's not possible to return an error from a constructor. I suggest moving this parsing code to a separate Parse() method and initializing all fields to defaults in the constructor. E.g. see JingleMessage struct for example: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/... https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:46: DCHECK(response_status == kOfflineStatusResponse); You shouldn't DCHECK on server response (or on any information received from outside). Logging a message and returning an error would be more appropriate here. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:18: kChromotingHostStatusOffline On 2015/07/06 22:19:12, joedow wrote: > A good practice with enums is to add a value at the end that you can use to test > for validity. An example of this is in app_remoting_service_urls.h IMO it's better to avoid "null" values, unless they are actually useful. (for the similar reasons reason null values are evil, e.g. see http://programmers.stackexchange.com/questions/12777/are-null-references-real... . It's one more case for the code to handle and it's easy to miss it). E.g. one case when null/unknown values can be useful in enums is when parsing user input to indicate invalid parameter. In this particular enum it might be useful to indicate that the service didn't supply status for the host, but I'm not sure it's necessary to distinguish that case from the host being offline OFFLINE. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:28: ChromotingHostStatus status; Specify default value here (i.e. add = kChromotingHostStatusOffline) as this is a simple type and will not be initialized automatically. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:64: base::ScopedClosureRunner runner(cb); I don't think this is a good way to use ScopedClosureRunner. It makes error handling very confusing here. Another problem here is that the callback can'd distinguish list being empty from failed request. I suggest adding another argument in the callback to solve this problem. I suggest separating parsing from calling the callback. E.g. this function could look as follows: std::vector<ChromotingHostInfo> hostlist; if (!ProcessResponse(&hostlist)) { hostlist.clear(); base::ResetAndReturnCallback(false, hostlist_callback_).Run(hostlist); return; } base::ResetAndReturnCallback(true, hostlist_callback_).Run(hostlist); https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:95: if (!data) { use GetDictionary() result here https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:103: if (!items) { See my comment in chromoting_host_info.cc: DictionaryValue::GetList() returns a bool value and you can use it here https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:110: for (int i = 0; i < size; ++i) { List values support iterators, use them here, i.e.: for (base::Value* item : *items) { hostlist_.push_back(ChromotingHostInfo(*item)); } https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:34: typedef base::Callback<void(std::vector<ChromotingHostInfo>* hostlist)> move this inside the class. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:34: typedef base::Callback<void(std::vector<ChromotingHostInfo>* hostlist)> I think the argument can be passed as const reference instead of a pointer https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:60: scoped_refptr<remoting::URLRequestContextGetter> request_context_getter_; this doesn't need to be remoting::URLRequestContextGetter. Use net::URLRequestContextGetter. remoting::URLRequestContextGetter is a remoting-specific implementation of net::URLRequestContextGetter, but this class doesn't need to know about it
I think the description for this CL can be improved. In general it's better to follow the following format: Short one line description. Longer description of the CL and its purpose. Can contain multiple paragraphs if necessary. BUG=<bug_id>
On 2015/07/07 00:06:17, Sergey Ulanov wrote: > I think the description for this CL can be improved. In general it's better to > follow the following format: > > Short one line description. > > Longer description of the CL and its purpose. Can contain > multiple paragraphs if necessary. > > BUG=<bug_id> Just to make this clear: it's not necessary to include per-file description for each file you are changing. If you need to provide some comments for reviwers you can do it by adding separate comments in the code
https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:10: const char kOnlineStatusResponse[] = "ONLINE"; On 2015/07/07 00:01:45, Sergey Ulanov wrote: > In general it's not useful to declare consts for strings like this, except when > they are used in multiple places. So I suggest moving "ONLINE" inline - that > would make the code more readable. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:10: const char kOnlineStatusResponse[] = "ONLINE"; On 2015/07/07 00:01:45, Sergey Ulanov wrote: > In general it's not useful to declare consts for strings like this, except when > they are used in multiple places. So I suggest moving "ONLINE" inline - that > would make the code more readable. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:17: ChromotingHostInfo::ChromotingHostInfo(const base::DictionaryValue& item) { On 2015/07/06 22:19:12, joedow wrote: > instead of item, can you call it host_info or something more descriptive? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:18: const base::ListValue* listValue = nullptr; On 2015/07/06 22:19:12, joedow wrote: > listValue should be list_value. This codebase does not use camel case for var > names. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:22: if (listValue != nullptr) { On 2015/07/06 22:19:12, joedow wrote: > This is cleaner as 'if (listValue) {' Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:22: if (listValue != nullptr) { On 2015/07/07 00:01:45, Sergey Ulanov wrote: > On 2015/07/06 22:19:12, joedow wrote: > > This is cleaner as 'if (listValue) {' > > DictionaryValue::GetList() returns bool, so this can be > if (item.GetList("tokenUrlPatterns", &listValue)) { > ... > } Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:25: std::string tokenUrlPattern; On 2015/07/06 22:19:12, joedow wrote: > token_url_pattern Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:35: item.GetString("hostId", &host_id); On 2015/07/07 00:01:45, Sergey Ulanov wrote: > Need to handle the case when any of the fields is missing. To do that you need > to be able to return errors from this function, and it's not possible to return > an error from a constructor. I suggest moving this parsing code to a separate > Parse() method and initializing all fields to defaults in the constructor. E.g. > see JingleMessage struct for example: > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/... Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:46: DCHECK(response_status == kOfflineStatusResponse); On 2015/07/06 22:19:12, joedow wrote: > A switch might be cleaner here (with a default case that logs the > response_status value and adds an UNREACHED macro. I put in NOTREACHED(). https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:46: DCHECK(response_status == kOfflineStatusResponse); On 2015/07/07 00:01:45, Sergey Ulanov wrote: > You shouldn't DCHECK on server response (or on any information received from > outside). Logging a message and returning an error would be more appropriate > here. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:18: kChromotingHostStatusOffline On 2015/07/06 22:19:12, joedow wrote: > A good practice with enums is to add a value at the end that you can use to test > for validity. An example of this is in app_remoting_service_urls.h The reason I didn't do this is because currently there are only two. I will add it in anyway. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:18: kChromotingHostStatusOffline On 2015/07/07 00:01:45, Sergey Ulanov wrote: > On 2015/07/06 22:19:12, joedow wrote: > > A good practice with enums is to add a value at the end that you can use to > test > > for validity. An example of this is in app_remoting_service_urls.h > > IMO it's better to avoid "null" values, unless they are actually useful. (for > the similar reasons reason null values are evil, e.g. see > http://programmers.stackexchange.com/questions/12777/are-null-references-real... > . It's one more case for the code to handle and it's easy to miss it). > E.g. one case when null/unknown values can be useful in enums is when parsing > user input to indicate invalid parameter. In this particular enum it might be > useful to indicate that the service didn't supply status for the host, but I'm > not sure it's necessary to distinguish that case from the host being offline > OFFLINE. I have added LOG(ERROR) and NOTREACHED() to print a status that is neither "online" or "offline". https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:22: explicit ChromotingHostInfo(const base::DictionaryValue& item); On 2015/07/06 22:19:12, joedow wrote: > item? can you make this more descriptive? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:28: ChromotingHostStatus status; On 2015/07/07 00:01:45, Sergey Ulanov wrote: > Specify default value here (i.e. add = kChromotingHostStatusOffline) as this is > a simple type and will not be initialized automatically. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:31: std::vector<std::string> tokenUrlPatterns; On 2015/07/06 22:19:12, joedow wrote: > token_url_patterns Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:44: net::URLFetcher::GET, this); On 2015/07/06 22:19:13, joedow wrote: > Will this line fit on the line above? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:49: return true; On 2015/07/06 22:19:13, joedow wrote: > Why return a value here if it is always true? I'd update this so it is void and > simplify the calling pattern. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:57: std::vector<ChromotingHostInfo> hostlist_; On 2015/07/06 22:19:13, joedow wrote: > no trailing underscore is used for local vars, only for class members. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:59: // Allows executing a callback after the original callback is Reset(). On 2015/07/06 22:19:12, joedow wrote: > Can you update the comment a bit, basically state that you are saving off the > object's stored callback so you can use a ScopedClosureRunner to ensure the > callback is run regardless of when the function returns. The ScopedClosureRunner has been removed. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:60: HostlistCallback reset_hostlist_callback(hostlist_callback_); On 2015/07/06 22:19:13, joedow wrote: > Can you rename reset_hostlist_callback? You could just use 'hostlist_callback' > without the trailing underscore since it is a local var. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:62: On 2015/07/06 22:19:12, joedow wrote: > remove newline Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:63: base::Closure cb = base::Bind(reset_hostlist_callback, &hostlist_); On 2015/07/06 22:19:13, joedow wrote: > You can use base::ConstRef(hostlist) here instead of a raw pointer. Raw > pointers are used for out params, in params are passed by const ref. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:63: base::Closure cb = base::Bind(reset_hostlist_callback, &hostlist_); On 2015/07/06 22:19:12, joedow wrote: > rename cb, something like bound_hostlist_callback would be good. You could also > just do this inline in the ScopedClosureRunner which would probably be cleaner. Acknowledged. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:64: base::ScopedClosureRunner runner(cb); On 2015/07/07 00:01:46, Sergey Ulanov wrote: > I don't think this is a good way to use ScopedClosureRunner. It makes error > handling very confusing here. > > Another problem here is that the callback can'd distinguish list being empty > from failed request. I suggest adding another argument in the callback to solve > this problem. > > I suggest separating parsing from calling the callback. E.g. this function could > look as follows: > std::vector<ChromotingHostInfo> hostlist; > if (!ProcessResponse(&hostlist)) { > hostlist.clear(); > base::ResetAndReturnCallback(false, hostlist_callback_).Run(hostlist); > return; > } > base::ResetAndReturnCallback(true, hostlist_callback_).Run(hostlist); > Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:94: On 2015/07/06 22:19:13, joedow wrote: > remove newline Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:95: if (!data) { On 2015/07/07 00:01:46, Sergey Ulanov wrote: > use GetDictionary() result here Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:100: const base::ListValue* items = nullptr; On 2015/07/06 22:19:13, joedow wrote: > would hosts be a better name? items is pretty generic Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:102: On 2015/07/06 22:19:13, joedow wrote: > remove newline Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:103: if (!items) { On 2015/07/07 00:01:46, Sergey Ulanov wrote: > See my comment in chromoting_host_info.cc: DictionaryValue::GetList() returns a > bool value and you can use it here Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:109: const base::DictionaryValue* item = nullptr; On 2015/07/06 22:19:12, joedow wrote: > host_info instead of item? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:110: for (int i = 0; i < size; ++i) { On 2015/07/07 00:01:46, Sergey Ulanov wrote: > List values support iterators, use them here, i.e.: > for (base::Value* item : *items) { > hostlist_.push_back(ChromotingHostInfo(*item)); > } Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:34: typedef base::Callback<void(std::vector<ChromotingHostInfo>* hostlist)> On 2015/07/07 00:01:46, Sergey Ulanov wrote: > move this inside the class. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:34: typedef base::Callback<void(std::vector<ChromotingHostInfo>* hostlist)> On 2015/07/07 00:01:46, Sergey Ulanov wrote: > I think the argument can be passed as const reference instead of a pointer Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:37: // Calls the Directory service to request for a host list for an access token. On 2015/07/06 22:19:13, joedow wrote: > s/to request for a/to request a Requests a host list from the Directory service for an access token. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:47: // Makes a service call to retrieve the details for a hostlist. The On 2015/07/06 22:19:13, joedow wrote: > Makes a service call to retrieve a hostlist. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:56: // Holds the URLFetcher for the ChromotingHostInfo request. On 2015/07/06 22:19:13, joedow wrote: > s/ChromotingHostInfo/ChromotingHostList Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:60: scoped_refptr<remoting::URLRequestContextGetter> request_context_getter_; On 2015/07/07 00:01:46, Sergey Ulanov wrote: > this doesn't need to be remoting::URLRequestContextGetter. Use > net::URLRequestContextGetter. > remoting::URLRequestContextGetter is a remoting-specific implementation of > net::URLRequestContextGetter, but this class doesn't need to know about it I do not think it is possible to just use net::URLRequestContextGetter because it has two pure virtual methods: GetURLRequestContext() and GetNetworkTaskRunner(). Do I have to create my own chromoting-specific implementation? https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:15: On 2015/07/06 22:19:13, joedow wrote: > remove extra newlines Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:22: std::vector<remoting::test::ChromotingHostInfo>* retrieved_hostlist) { On 2015/07/06 22:19:14, joedow wrote: > retrieved_hostlist should be const ref Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:65: "}"; On 2015/07/06 22:19:13, joedow wrote: > kChromotingHostListReadyResponse is a good start, but you probably also want > additional scenarios: > > - empty items dict > data: { kind: items: {} } } > > - no token url patterns (not sure if this is expected or an error) > data: { kind: items: { tokenUrlPatterns: [] ...details... } } } > > - invalid status, possibly missing fields as is interesting Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:76: // ChromotingHostListFetcher. This fixture also creates an IO MessageLoop, if On 2015/07/06 22:19:14, joedow wrote: > nit: it looks like it always creates a MessageLoop, this fixture doesn't check > to see if one exists. That's fine to do, but the comment should be updated. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:137: const unsigned int expectedHLSize = 2; On 2015/07/06 22:19:14, joedow wrote: > no camel case, this var should be called expected_hostlist_size Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:140: ChromotingHostInfo onlineChromotingHostInfo = hostlist.front(); On 2015/07/06 22:19:14, joedow wrote: > If you know the size, I would just use array indexing here instead of front/back Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:140: ChromotingHostInfo onlineChromotingHostInfo = hostlist.front(); On 2015/07/06 22:19:13, joedow wrote: > Also, no camel case. online_chromoting_host_info. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:141: const unsigned int expectedTUPSize = 3; On 2015/07/06 22:19:14, joedow wrote: > expected_pattern_num? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:143: EXPECT_TRUE(!onlineChromotingHostInfo.host_id.empty()); On 2015/07/06 22:19:14, joedow wrote: > I think it's more readable when you think the string should be valid to use: > EXPECT_FALSE(my_fake_string.empty()); Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:151: ChromotingHostInfo offlineChromotingHostInfo = hostlist.back(); On 2015/07/06 22:19:14, joedow wrote: > offline_chromoting_host_info Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:162: TEST_F(ChromotingHostListFetcherTest, RetrieveHostListInvalidEnvironment) { On 2015/07/06 22:19:14, joedow wrote: > The test names is "...InvalidEnvironment" but it looks like this is more of a > network error test. Can you fix the name? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:181: On 2015/07/06 22:19:14, joedow wrote: > You should also add a test which makes two requests using the same hostlist > fetcher since it was written to allow that. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:202: // should be populated. On 2015/07/06 22:19:14, joedow wrote: > "connection details"? Copy/paste error? Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:160: std::vector<remoting::test::ChromotingHostInfo>* retrieved_hostlist) { On 2015/07/06 22:19:15, joedow wrote: > retrieved_hostlist should be const ref Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:162: DVLOG(1) << "OnHostlistRetrieved() Called"; On 2015/07/06 22:19:14, joedow wrote: > FYI, I've switched over to using VLOG instead of DVLOG as we run release bits on > our waterfall and DVLOG will make it difficult to debug failures. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:260: On 2015/07/06 22:19:14, joedow wrote: > remove newline Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:277: // RunLoop to handle callback from Directory service. On 2015/07/06 22:19:14, joedow wrote: > update comment "Reset runloop to use it to retrieve hostlist from directory > service." Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:281: On 2015/07/06 22:19:14, joedow wrote: > remove newline Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:282: remoting::test::HostlistCallback hostlist_fetch_callback = base::Bind( On 2015/07/06 22:19:14, joedow wrote: > hostlist_fetch_callback can be rename hostlist_request_callback. Technically > the callback is used for a single request whereas the fetcher can be used > multiple times. Done. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:283: &OnHostlistRetrieved, run_loop->QuitClosure(), &hostlist); On 2015/07/06 22:19:14, joedow wrote: > base::ConstRef(hostlist) If you look at my new function, void OnHostlistRetrieved( base::Closure done_closure, std::vector<remoting::test::ChromotingHostInfo>* hostlist, const std::vector<remoting::test::ChromotingHostInfo>& retrieved_hostlist); The &hostlist parameter allows for the retrieved_hostlist to override hostlist's value to retrieved_hostlist's. If I make the hostlist passed by const reference, then the value cannot be set. I believe this needs to remain a pointer unless we can show another way to save the hostlist. However, I have changed the retrieved_hostlist to be passed by const reference, which you can see in the function parameters above. https://codereview.chromium.org/1212333011/diff/20001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:287: On 2015/07/06 22:19:14, joedow wrote: > Remove newline Done.
https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:21: // Add TokenUrlPatterns to ChromotingHostInfo comments need to end in periods, please add one here. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:49: } nit: I'd prefer to see newlines in between each chunk of code which handles a property, makes it easier to parse https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:60: if (!host_info.GetString("jabberId", &host_jid)) { Should you check to see if online is set here? It sounds like no jid for an online host would be a failure. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:30: ChromotingHostStatus status = kChromotingHostStatusOffline; This would be better if you set this in the C'Tor initializer list https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:91: // be added to the hostlist. Why is this a TODO? It seems like documentation vs. a comment about changes that need to be made in the future. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:95: *reinterpret_cast<base::DictionaryValue*>(host_info))) { Instead of using a reinterpret cast here, can you use host_info->GetAsDictionary(&host_dict) here? https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:114: return; nit: I think it is better to have a single return statement (if possible). Why not just set a response_processed bool if processing failed (and clear the hostlist) then have a single ResetAndReturn call at the bottom here? https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:55: // a failed request. I think this comment is a little off, this function processes the reponse from the directory service. It could be used w/o a callback if the internal implementation changed. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:21: bool request_success) { What is request_success used for? Can you remove it if not needed? https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:132: "}"; Good mixture of data for testing, thanks for adding that! https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:172: // ChromotingHostListFetcher. This fixture also creates an IO MessageLoop nit: You can just say MessageLoop here, you don't really need to say IO since it is in the class definition. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:241: ChromotingHostStatus::kChromotingHostStatusOnline); nit: this line would look better if the params were lined up (same with the rest of them in the file) https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:161: bool request_success) { I actually don't think the bool here provides much value, you already log errors if the parsing fails, I don't think adding another log statement here is worth expanding the param list. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:259: // A RunLoop that yields to the thread's MessageLoopForIO nit: MessageLoop, also, comments should end with periods.
https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:43: NOTREACHED(); return false? NOTREACHED is equivalent to DCHECK(false) and you should never DCHECK on the data received from outside. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:21: struct ChromotingHostInfo { I think this can be called HostInfo. it's already in remoting:: namespace so Chromoting prefix seems redundant. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:30: ChromotingHostStatus status = kChromotingHostStatusOffline; On 2015/07/08 17:19:07, joedow wrote: > This would be better if you set this in the C'Tor initializer list Style guide says: Use in-class member initialization for simple initializations, especially when a member variable must be initialized the same way in more than one constructor. IMO it's good idea to use in-class initialization here. It ensures that the field is always initialized and makes it clear what's the default value for this field. We didn't have this feature before C++11 that's why a lot of code still uses initializer lists. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:45: bool request_success)> nit: it's better to reverse the order of the parameters. hostlist is meaniningful only if request_success=true;
I have changed the file names of chromoting_host_info and chromoting_host_list_fetcher to host_info and host_list_fetcher respectively. I have also cleaned up the code based on the suggestions provided. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_info.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:21: // Add TokenUrlPatterns to ChromotingHostInfo On 2015/07/08 17:19:07, joedow wrote: > comments need to end in periods, please add one here. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:43: NOTREACHED(); On 2015/07/08 19:57:33, Sergey Ulanov wrote: > return false? > NOTREACHED is equivalent to DCHECK(false) and you should never DCHECK on the > data received from outside. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:49: } On 2015/07/08 17:19:07, joedow wrote: > nit: I'd prefer to see newlines in between each chunk of code which handles a > property, makes it easier to parse Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.cc:60: if (!host_info.GetString("jabberId", &host_jid)) { On 2015/07/08 17:19:07, joedow wrote: > Should you check to see if online is set here? It sounds like no jid for an > online host would be a failure. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_info.h (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:21: struct ChromotingHostInfo { On 2015/07/08 19:57:33, Sergey Ulanov wrote: > I think this can be called HostInfo. it's already in remoting:: namespace so > Chromoting prefix seems redundant. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_info.h:30: ChromotingHostStatus status = kChromotingHostStatusOffline; On 2015/07/08 19:57:34, Sergey Ulanov wrote: > On 2015/07/08 17:19:07, joedow wrote: > > This would be better if you set this in the C'Tor initializer list > > Style guide says: > Use in-class member initialization for simple initializations, especially when > a member variable must be initialized the same way in more than one constructor. > > IMO it's good idea to use in-class initialization here. It ensures that the > field is always initialized and makes it clear what's the default value for this > field. We didn't have this feature before C++11 that's why a lot of code still > uses initializer lists. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:91: // be added to the hostlist. On 2015/07/08 17:19:07, joedow wrote: > Why is this a TODO? It seems like documentation vs. a comment about changes > that need to be made in the future. That was a mistake. Changed it. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:95: *reinterpret_cast<base::DictionaryValue*>(host_info))) { On 2015/07/08 17:19:07, joedow wrote: > Instead of using a reinterpret cast here, can you use > host_info->GetAsDictionary(&host_dict) here? Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.cc:114: return; On 2015/07/08 17:19:07, joedow wrote: > nit: I think it is better to have a single return statement (if possible). Why > not just set a response_processed bool if processing failed (and clear the > hostlist) then have a single ResetAndReturn call at the bottom here? Good thinking. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:45: bool request_success)> On 2015/07/08 19:57:34, Sergey Ulanov wrote: > nit: it's better to reverse the order of the parameters. hostlist is > meaniningful only if request_success=true; I've decided to take this out for two reasons: 1) Not knowing whether or not there was a parsing error or if the response was empty is not critical to know at the callback. There is already logging for when an parsing error occurs and neither case will have any hosts in their hostlist to connect to. 2) If in the future this needs to be added, it is very simple to do. However, as of now, it does not serve a meaningful purpose for my driver. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher.h:55: // a failed request. On 2015/07/08 17:19:07, joedow wrote: > I think this comment is a little off, this function processes the reponse from > the directory service. It could be used w/o a callback if the internal > implementation changed. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:21: bool request_success) { On 2015/07/08 17:19:08, joedow wrote: > What is request_success used for? Can you remove it if not needed? Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:132: "}"; On 2015/07/08 17:19:08, joedow wrote: > Good mixture of data for testing, thanks for adding that! Acknowledged. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:172: // ChromotingHostListFetcher. This fixture also creates an IO MessageLoop On 2015/07/08 17:19:08, joedow wrote: > nit: You can just say MessageLoop here, you don't really need to say IO since it > is in the class definition. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_host_list_fetcher_unittest.cc:241: ChromotingHostStatus::kChromotingHostStatusOnline); On 2015/07/08 17:19:08, joedow wrote: > nit: this line would look better if the params were lined up (same with the rest > of them in the file) Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:161: bool request_success) { On 2015/07/08 17:19:08, joedow wrote: > I actually don't think the bool here provides much value, you already log errors > if the parsing fails, I don't think adding another log statement here is worth > expanding the param list. Done. https://codereview.chromium.org/1212333011/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:259: // A RunLoop that yields to the thread's MessageLoopForIO On 2015/07/08 17:19:08, joedow wrote: > nit: MessageLoop, also, comments should end with periods. Done.
https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:159: std::vector<remoting::test::HostInfo>* hostlist, Why do you need to use this parameter. It's not used in main(), i.e. main() doesn't do anything with it after that. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:286: run_loop->Run(); I think it would be cleaner to run message loop only once and do everything from there. I.e. you can query host list from OnAccessTokenRetrieved(). No need to quit and restart the message loop for that. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:24: if (size > 0) { if (!list_value->empty()) https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:26: for (int i = 0; i < size; ++i) { iterate the list using iterator: for (net::Value* item : *list_value) { std::string patter; if (!item->GetAsString(pattern)) return false; token_url_patterns.push_back(token_url_pattern); } https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher_unittest.cc:16: // Used as a HostListCallback for testing. nit: empty line above this one.
https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:150: DVLOG(1) << "Access Token: " << retrieved_access_token; Can you use VLOG consistently? DVLOG won't show up in release builds which is what we run on the waterfall. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher.cc:30: DVLOG(2) << "HostListFetcher::RetrieveHostlist() called"; VLOG https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher.cc:109: // processing the response failed. This comment is kind of confusing. How will malformed data notify the callback? https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher.h:53: // Processes the response from the directory service nit: add period to end of comment https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher_unittest.cc:301: const unsigned int expected_patterns_size = 3; This should be a const in the file scoped namespace at the top of this file, I don't think it needs to be inlined. Same goes for all of the inlined const values, it'd be easier to define them once at the top and refer to them in each test.
https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:150: DVLOG(1) << "Access Token: " << retrieved_access_token; On 2015/07/09 02:41:18, joedow wrote: > Can you use VLOG consistently? DVLOG won't show up in release builds which is > what we run on the waterfall. Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:159: std::vector<remoting::test::HostInfo>* hostlist, On 2015/07/09 01:25:29, Sergey Ulanov wrote: > Why do you need to use this parameter. It's not used in main(), i.e. main() > doesn't do anything with it after that. I will use it in my next CL, which will connect to a host through the information provided by the host list. The purpose of this CL is to create the HostListFetcher. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:286: run_loop->Run(); On 2015/07/09 01:25:29, Sergey Ulanov wrote: > I think it would be cleaner to run message loop only once and do everything from > there. I.e. you can query host list from OnAccessTokenRetrieved(). No need to > quit and restart the message loop for that. I have to wait for the response from GAIA for the access token. Otherwise, the driver crashes because we didn't wait for the access token to be set through the callback function OnAccessTokenRetrieved(); https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:24: if (size > 0) { On 2015/07/09 01:25:29, Sergey Ulanov wrote: > if (!list_value->empty()) Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:26: for (int i = 0; i < size; ++i) { On 2015/07/09 01:25:29, Sergey Ulanov wrote: > iterate the list using iterator: > for (net::Value* item : *list_value) { > std::string patter; > if (!item->GetAsString(pattern)) > return false; > token_url_patterns.push_back(token_url_pattern); > } Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher.cc:30: DVLOG(2) << "HostListFetcher::RetrieveHostlist() called"; On 2015/07/09 02:41:18, joedow wrote: > VLOG Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher.cc:109: // processing the response failed. On 2015/07/09 02:41:18, joedow wrote: > This comment is kind of confusing. How will malformed data notify the callback? I just removed it entirely. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher.h (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher.h:53: // Processes the response from the directory service On 2015/07/09 02:41:18, joedow wrote: > nit: add period to end of comment Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... File remoting/test/host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher_unittest.cc:16: // Used as a HostListCallback for testing. On 2015/07/09 01:25:29, Sergey Ulanov wrote: > nit: empty line above this one. Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_list... remoting/test/host_list_fetcher_unittest.cc:301: const unsigned int expected_patterns_size = 3; On 2015/07/09 02:41:18, joedow wrote: > This should be a const in the file scoped namespace at the top of this file, I > don't think it needs to be inlined. Same goes for all of the inlined const > values, it'd be easier to define them once at the top and refer to them in each > test. Done.
LGTM. Once last few comments are addressed... https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... File remoting/test/host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... remoting/test/host_list_fetcher.cc:30: DVLOG(2) << "HostListFetcher::RetrieveHostlist() called"; VLOG? https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... File remoting/test/host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... remoting/test/host_list_fetcher_unittest.cc:165: const unsigned int expected_empty_patterns_host_list_size = 1; const values should use the kNoUnderscoresNamingScheme
https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... File remoting/test/host_list_fetcher.cc (right): https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... remoting/test/host_list_fetcher.cc:30: DVLOG(2) << "HostListFetcher::RetrieveHostlist() called"; On 2015/07/09 03:09:56, joedow wrote: > VLOG? Done. https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... File remoting/test/host_list_fetcher_unittest.cc (right): https://codereview.chromium.org/1212333011/diff/80001/remoting/test/host_list... remoting/test/host_list_fetcher_unittest.cc:165: const unsigned int expected_empty_patterns_host_list_size = 1; On 2015/07/09 03:09:56, joedow wrote: > const values should use the kNoUnderscoresNamingScheme Done.
LGTM, but please see my comments https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:286: run_loop->Run(); On 2015/07/09 03:02:46, tonychun wrote: > On 2015/07/09 01:25:29, Sergey Ulanov wrote: > > I think it would be cleaner to run message loop only once and do everything > from > > there. I.e. you can query host list from OnAccessTokenRetrieved(). No need to > > quit and restart the message loop for that. > > I have to wait for the response from GAIA for the access token. Otherwise, the > driver crashes because we didn't wait for the access token to be set through the > callback function OnAccessTokenRetrieved(); Right. That doesn't mean you need to quit the loop every time. Once the access token is received you can query hostlist from OnAccessTokenRetrieved() instead of quitting the message loop https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:64: LOG(INFO) << "jabberId was not found for " << host_name; Don't really need this message. JID is useless if the host is offline. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:75: LOG(INFO) << "hostOfflineReason was not found for " << host_name; I don't think you really need this - there will be a lot of host that don't have this field.
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/1212333011/#ps120001 (title: "Final clean up.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212333011/120001
https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... File remoting/test/chromoting_test_driver.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/chromotin... remoting/test/chromoting_test_driver.cc:286: run_loop->Run(); On 2015/07/09 20:15:14, Sergey Ulanov wrote: > On 2015/07/09 03:02:46, tonychun wrote: > > On 2015/07/09 01:25:29, Sergey Ulanov wrote: > > > I think it would be cleaner to run message loop only once and do everything > > from > > > there. I.e. you can query host list from OnAccessTokenRetrieved(). No need > to > > > quit and restart the message loop for that. > > > > I have to wait for the response from GAIA for the access token. Otherwise, the > > driver crashes because we didn't wait for the access token to be set through > the > > callback function OnAccessTokenRetrieved(); > > Right. That doesn't mean you need to quit the loop every time. Once the access > token is received you can query hostlist from OnAccessTokenRetrieved() instead > of quitting the message loop Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info.cc File remoting/test/host_info.cc (right): https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:64: LOG(INFO) << "jabberId was not found for " << host_name; On 2015/07/09 20:15:14, Sergey Ulanov wrote: > Don't really need this message. JID is useless if the host is offline. Done. https://codereview.chromium.org/1212333011/diff/60001/remoting/test/host_info... remoting/test/host_info.cc:75: LOG(INFO) << "hostOfflineReason was not found for " << host_name; On 2015/07/09 20:15:14, Sergey Ulanov wrote: > I don't think you really need this - there will be a lot of host that don't have > this field. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) (exceeded global retry quota)
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/1212333011/#ps140001 (title: "Added missing offline_host check.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212333011/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e3fe98489be2fe1f9e504fa5c6ac1f2e205fd561 Cr-Commit-Position: refs/heads/master@{#338328} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
