Retrieved Hostlist information from Directory service.

remoting/test/hostlist_fetcher.h

Index: remoting/test/hostlist_fetcher.h
diff --git a/remoting/test/hostlist_fetcher.h b/remoting/test/hostlist_fetcher.h
new file mode 100644
index 0000000000000000000000000000000000000000..67bf0a22d146b82280ae67dff3449237abca13c5
--- /dev/null
+++ b/remoting/test/hostlist_fetcher.h
@@ -0,0 +1,80 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_TEST_HOSTLIST_FETCHER_H_
+#define REMOTING_TEST_HOSTLIST_FETCHER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "remoting/test/chromoting_host_info.h"
+
+namespace net {
+class UrlFetcher;
+}
+namespace remoting {
+class URLRequestContextGetter;
+}
+
+namespace remoting {
+namespace test {
+
+// Used by the HostlistFetcher to make HTTP requests and also by the
+// unittests for this class to set fake response data for these URLs.
+const char kProdServiceEnvironmentUrlFormat[] = "https://www.googleapis.com/"

[joedow, 2015/07/01 03:39:36]

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.

[tonychun, 2015/07/06 20:11:51]

Done.

+    "chromoting/v1/@me/hosts";
+
+// Specifies the service API to call for the hostlist.
+// Note: When adding new environments, add them before kUnknownEnvironment as
+// the last entry is used for bounds checking.
+enum ServiceEnvironment {

[joedow, 2015/07/01 03:39:36]

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.

[tonychun, 2015/07/06 20:11:51]

Done.

+  kProductionEnvironment,
+  kUnknownEnvironment
+};
+
+// Supplied by the client for each hostlist request and returns a valid,
+// initialized Hostlist object on success.

[joedow, 2015/07/01 03:39:36]

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).

[tonychun, 2015/07/06 20:11:51]

Done.

+typedef base::Callback<void(const std::vector<ChromotingHostInfo>& hostlist)>
+    HostlistCallback;
+
+// Calls the App Remoting service API to request connection info for a remote

[joedow, 2015/07/01 03:39:35]

I'm guessing this class does not call the App Remoting Service API :)  Can you
update the comments please?

[tonychun, 2015/07/06 20:11:51]

Done.

+// host.  Destroying the RemoteHostInfoFetcher while a request is outstanding
+// will cancel the request. It is safe to delete the fetcher from within a
+// completion callback.  Must be used from a thread running an IO message loop.
+// The public method is virtual to allow for mocking and fakes.
+class HostlistFetcher : public net::URLFetcherDelegate {

[joedow, 2015/07/01 03:39:35]

Can you rename this class ChromotingHostListFetcher (the files should be renamed
to chromoting_host_list_fetcher.*)

[tonychun, 2015/07/06 20:11:51]

Done.

+ public:
+  HostlistFetcher();
+  ~HostlistFetcher() override;
+
+  // Makes a service call to retrieve the details for a hostlist. The
+  // callback will be called once the HTTP request has completed.
+  virtual bool RetrieveHostlist(const std::string& access_token,
+                                ServiceEnvironment service_environment,
+                                const HostlistCallback& callback);
+
+ private:
+  // net::URLFetcherDelegate interface.
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+  // Holds the URLFetcher for the RemoteHostInfo request.

[joedow, 2015/07/01 03:39:36]

RemoteHostInfo request?

[tonychun, 2015/07/06 20:11:51]

Done.

+  scoped_ptr<net::URLFetcher> request_;
+
+  // Provides application-specific context for the network request.
+  scoped_refptr<remoting::URLRequestContextGetter> request_context_getter_;
+
+  // Caller-supplied callback used to return hostlist on success.
+  HostlistCallback hostlist_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(HostlistFetcher);
+};
+
+}  // namespace test
+}  // namespace remoting
+
+#endif // REMOTING_TEST_HOSTLIST_FETCHER_H_