Remove UrlFetcher from remoting and use the one in net instead.

remoting/host/gaia_oauth_client.cc

 // Copyright (c) 2012 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.
 
 #include "remoting/host/gaia_oauth_client.h"
 
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
 #include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
-#include "remoting/host/url_fetcher.h"
 
 namespace {
 
 const char kDefaultOAuth2TokenUrl[] =
     "https://accounts.google.com/o/oauth2/token";
 const char kDefaultOAuth2UserInfoUrl[] =
     "https://www.googleapis.com/oauth2/v1/userinfo";
 
 // Values used to parse token response.
 const char kAccessTokenValue[] = "access_token";
 const char kRefreshTokenValue[] = "refresh_token";
 const char kExpiresInValue[] = "expires_in";
 
 // Values used when parsing userinfo response.
 const char kEmailValue[] = "email";
 
 }  // namespace
 
 namespace remoting {
 
 // static
 OAuthProviderInfo OAuthProviderInfo::GetDefault() {
   OAuthProviderInfo result;
   result.access_token_url = kDefaultOAuth2TokenUrl;
   result.user_info_url = kDefaultOAuth2UserInfoUrl;
   return result;
 }
 
 class GaiaOAuthClient::Core
-    : public base::RefCountedThreadSafe<GaiaOAuthClient::Core> {
+    : public base::RefCountedThreadSafe<GaiaOAuthClient::Core>,
+      public net::URLFetcherDelegate {

[Wez, 2012/06/22 20:14:39]

Do we actually need this implementation to be split into a caller-owned object
and ref-counted core? It doesn't look like the Core is ref-counted any more, and
even in the previous implementation, it looks like Unretained would have been
valid, since the object was being Bind()ed for callback, not task posting.

[Jamie, 2012/06/22 22:42:02]

This implementation is based on one used by cloud print. I think you're right
that it doesn't need the separation any more, but I think that should be a
separate CL. I want this to be just about switching over from the old to the new
URLFetcher class so that it's easier to check that it's been done correctly.

[Wez, 2012/06/22 23:58:35]

SGTM

  public:
   Core(const OAuthProviderInfo& info,
        net::URLRequestContextGetter* request_context_getter)
       : provider_info_(info),
         request_context_getter_(request_context_getter),
-        delegate_(NULL) {
+        delegate_(NULL),
+        request_is_oauth_refresh_(false) {
   }
 
   void RefreshToken(const OAuthClientInfo& oauth_client_info,
                     const std::string& refresh_token,
                     GaiaOAuthClient::Delegate* delegate);
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;

[Wez, 2012/06/22 20:14:39]

nit: Prefix this with a blank line and comment "net::URLFetcherDelegate
interface"

[Jamie, 2012/06/22 22:42:02]

Done.

 
  private:
   friend class base::RefCountedThreadSafe<Core>;
   virtual ~Core() {}
 
   void OnAuthTokenFetchComplete(const net::URLRequestStatus& status,
                                 int response_code,
                                 const std::string& response);
   void FetchUserInfoAndInvokeCallback();
   void OnUserInfoFetchComplete(const net::URLRequestStatus& status,
                                int response_code,
                                const std::string& response);
 
   OAuthProviderInfo provider_info_;
 
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
   GaiaOAuthClient::Delegate* delegate_;
-  scoped_ptr<UrlFetcher> request_;
+  scoped_ptr<net::URLFetcher> request_;
+  bool request_is_oauth_refresh_;

[Wez, 2012/06/22 20:14:39]

Looking at the rest of the class, I think it'd be cleaner to keep distinct
request pointers for the two request types.

[Jamie, 2012/06/22 22:42:02]

That's what I had at first, but we check that the request is not already set
when refreshing a token. We could change that to check both fetchers, but I
don't think that's any clearer, and this design makes explicit the fact that you
can only have one request in flight at a time.

[Wez, 2012/06/22 23:58:35]

I take your point, but you should really use a "state" enum here, rather than a
Boolean to express that.

[Jamie, 2012/06/23 00:20:23]

Done.

 
   std::string access_token_;
   int expires_in_seconds_;
 };
 
 void GaiaOAuthClient::Core::RefreshToken(
     const OAuthClientInfo& oauth_client_info,
     const std::string& refresh_token,
     GaiaOAuthClient::Delegate* delegate) {
   DCHECK(!request_.get()) << "Tried to fetch two things at once!";
 
   delegate_ = delegate;
 
   access_token_.clear();
   expires_in_seconds_ = 0;
 
   std::string post_body =
       "refresh_token=" + net::EscapeUrlEncodedData(refresh_token, true) +
       "&client_id=" + net::EscapeUrlEncodedData(oauth_client_info.client_id,
                                                 true) +
       "&client_secret=" +
       net::EscapeUrlEncodedData(oauth_client_info.client_secret, true) +
       "&grant_type=refresh_token";
-  request_.reset(new UrlFetcher(GURL(provider_info_.access_token_url),
-                                UrlFetcher::POST));
+  request_.reset(net::URLFetcher::Create(
+      GURL(provider_info_.access_token_url), net::URLFetcher::POST, this));
   request_->SetRequestContext(request_context_getter_);
   request_->SetUploadData("application/x-www-form-urlencoded", post_body);
-  request_->Start(
-      base::Bind(&GaiaOAuthClient::Core::OnAuthTokenFetchComplete, this));
+  request_is_oauth_refresh_ = true;
+  request_->Start();
+}
+
+void GaiaOAuthClient::Core::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  std::string response_string;
+  source->GetResponseAsString(&response_string);
+  if (request_is_oauth_refresh_) {
+    OnAuthTokenFetchComplete(source->GetStatus(),
+                             source->GetResponseCode(),
+                             response_string);
+  } else {
+    OnUserInfoFetchComplete(source->GetStatus(),
+                            source->GetResponseCode(),
+                            response_string);
+  }
 }
 
 void GaiaOAuthClient::Core::OnAuthTokenFetchComplete(
     const net::URLRequestStatus& status,
     int response_code,
     const std::string& response) {
   request_.reset();
 
   if (!status.is_success()) {
     delegate_->OnNetworkError(response_code);
@@ skipping 23 matching lines @@
   }
 
   if (access_token_.empty()) {
     delegate_->OnNetworkError(response_code);
   } else {
     FetchUserInfoAndInvokeCallback();
   }
 }
 
 void GaiaOAuthClient::Core::FetchUserInfoAndInvokeCallback() {
-  request_.reset(new UrlFetcher(
-      GURL(provider_info_.user_info_url), UrlFetcher::GET));
+  request_.reset(net::URLFetcher::Create(
+      GURL(provider_info_.user_info_url), net::URLFetcher::GET, this));
   request_->SetRequestContext(request_context_getter_);
-  request_->SetHeader("Authorization", "Bearer " + access_token_);
-  request_->Start(
-      base::Bind(&GaiaOAuthClient::Core::OnUserInfoFetchComplete, this));
+  request_->AddExtraRequestHeader("Authorization: Bearer " + access_token_);

[Wez, 2012/06/22 20:14:39]

Do we need to sanitize access_token_, or will AddExtraRequestHeader() verify
that we're not adding a malformed header if it contains bad data?

[Jamie, 2012/06/22 22:42:02]

It DCHECKs that it doesn't contain CRLF. Something stronger would be nice, so
I've added code that causes the token refresh operation to fail if the returned
token contains CRLF.

+  request_is_oauth_refresh_ = false;
+  request_->Start();
 }
 
 void GaiaOAuthClient::Core::OnUserInfoFetchComplete(
     const net::URLRequestStatus& status,
     int response_code,
     const std::string& response) {
+  request_.reset();
   std::string email;
   if (response_code == net::HTTP_OK) {
     scoped_ptr<Value> message_value(base::JSONReader::Read(response));
     if (message_value.get() &&
         message_value->IsType(Value::TYPE_DICTIONARY)) {
       scoped_ptr<DictionaryValue> response_dict(
           static_cast<DictionaryValue*>(message_value.release()));
       response_dict->GetString(kEmailValue, &email);
     }
   }
@@ skipping 16 matching lines @@
 
 void GaiaOAuthClient::RefreshToken(const OAuthClientInfo& oauth_client_info,
                                    const std::string& refresh_token,
                                    Delegate* delegate) {
   return core_->RefreshToken(oauth_client_info,
                              refresh_token,
                              delegate);
 }
 
 }  // namespace remoting