Change ProfileDownloader to use AccountTrackerService.

chrome/browser/profiles/profile_downloader.cc

 // Copyright (c) 2013 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 "chrome/browser/profiles/profile_downloader.h"
 
 #include <string>
 #include <vector>
 
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_downloader_delegate.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/gaia/gaia_constants.h"

[Mike Lerman, 2015/04/20 13:25:22]

I imagine these google_apis/gaia includes aren't needed anymore.

[anthonyvd, 2015/04/29 15:15:08]

gaia_constants still is but gaia_urls isn't anymore. Thanks!

 #include "google_apis/gaia/gaia_urls.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_status.h"
 #include "skia/ext/image_operations.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 
 namespace {
 
 // Template for optional authorization header when using an OAuth access token.
 const char kAuthorizationHeader[] =
     "Authorization: Bearer %s";
 
-// Path in JSON dictionary to user's photo thumbnail URL.
-const char kPhotoThumbnailURLPath[] = "picture";
-
-// Path in JSON dictionary to user's hosted domain.
-const char kHostedDomainPath[] = "hd";
-
-// From the user info API, this field corresponds to the full name of the user.
-const char kFullNamePath[] = "name";
-
-const char kGivenNamePath[] = "given_name";
-
-// Path in JSON dictionary to user's preferred locale.
-const char kLocalePath[] = "locale";
-
 // Path format for specifying thumbnail's size.
 const char kThumbnailSizeFormat[] = "s%d-c";
 // Default thumbnail size.
 const int kDefaultThumbnailSize = 64;
 
 // Separator of URL path components.
 const char kURLPathSeparator = '/';
 
 // Photo ID of the Picasa Web Albums profile picture (base64 of 0).
 const char kPicasaPhotoId[] = "AAAAAAAAAAA";
@@ skipping 53 matching lines @@
     return new_url->is_valid();
   }
 
   // We can't set the image size, just use the default size.
   *new_url = old_url;
   return true;
 }
 
 }  // namespace
 
-// Parses the entry response and gets the name and profile image URL.
-// |data| should be the JSON formatted data return by the response.
-// Returns false to indicate a parsing error.
-bool ProfileDownloader::ParseProfileJSON(base::DictionaryValue* root_dictionary,
-                                         base::string16* full_name,
-                                         base::string16* given_name,
-                                         std::string* url,
-                                         int image_size,
-                                         std::string* profile_locale,
-                                         base::string16* hosted_domain) {
-  DCHECK(full_name);
-  DCHECK(given_name);
-  DCHECK(url);
-  DCHECK(profile_locale);
-  DCHECK(hosted_domain);
-
-  *full_name = base::string16();
-  *given_name = base::string16();
-  *url = std::string();
-  *profile_locale = std::string();
-  *hosted_domain = base::string16();
-
-  root_dictionary->GetString(kFullNamePath, full_name);
-  root_dictionary->GetString(kGivenNamePath, given_name);
-  root_dictionary->GetString(kLocalePath, profile_locale);
-  root_dictionary->GetString(kHostedDomainPath, hosted_domain);
-
-  std::string url_string;
-  if (root_dictionary->GetString(kPhotoThumbnailURLPath, &url_string)) {
-    GURL new_url;
-    if (!GetImageURLWithSize(GURL(url_string), image_size, &new_url)) {
-      LOG(ERROR) << "GetImageURLWithSize failed for url: " << url_string;
-      return false;
-    }
-    *url = new_url.spec();
-  }
-
-  // The profile data is considered valid as long as it has a name or a picture.
-  return !full_name->empty() || !url->empty();
-}
-
 // static
 bool ProfileDownloader::IsDefaultProfileImageURL(const std::string& url) {
   if (url.empty())
     return true;
 
   GURL image_url_object(url);
   DCHECK(image_url_object.is_valid());
   VLOG(1) << "URL to check for default image: " << image_url_object.spec();
   std::vector<std::string> path_components;
   base::SplitString(image_url_object.path(),
@@ skipping 18 matching lines @@
       OAuth2TokenService::Consumer("profile_downloader"),
       delegate_(delegate),
       picture_status_(PICTURE_FAILED) {
   DCHECK(delegate_);
 }
 
 void ProfileDownloader::Start() {
   StartForAccount(std::string());
 }
 
-void ProfileDownloader::StartForAccount(const std::string& account_id) {
+void ProfileDownloader::StartForAccount(
+    const std::string& account_id,
+    AccountTrackerService* account_tracker_service) {
   VLOG(1) << "Starting profile downloader...";
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Ensure that these methods are called on the UI thread, except for
+  // unittests where a UI thread might not have been created.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsMessageLoopValid(BrowserThread::UI));

[Mike Lerman, 2015/04/20 13:25:23]

I don't understand why you need IsMessageLoopValid. Generally we just use
CurrentlyOn and everything tests fine?

[Roger Tawa OOO till Jul 10th, 2015/04/21 14:00:33]

Use a TestBrowserThreadBundle in the test.

[anthonyvd, 2015/04/29 15:15:08]

Done.

 
   ProfileOAuth2TokenService* service =
       ProfileOAuth2TokenServiceFactory::GetForProfile(
           delegate_->GetBrowserProfile());
   if (!service) {
     // This can happen in some test paths.
     LOG(WARNING) << "User has no token service";
     delegate_->OnProfileDownloadFailure(
         this, ProfileDownloaderDelegate::TOKEN_ERROR);
     return;
   }
 
   SigninManagerBase* signin_manager =
       SigninManagerFactory::GetForProfile(delegate_->GetBrowserProfile());
   account_id_ =
       account_id.empty() ?
           signin_manager->GetAuthenticatedAccountId() : account_id;
   if (service->RefreshTokenIsAvailable(account_id_)) {
     StartFetchingOAuth2AccessToken();
   } else {
     service->AddObserver(this);
   }
+
+  if (account_tracker_service)

[Mike Lerman, 2015/04/20 13:25:22]

Since we now have a reference to a PKS, I wonder if this should be a PKS.

Also - we can set this in the constructor using the delegate.

[Roger Tawa OOO till Jul 10th, 2015/04/21 14:00:33]

I think it might be worth making this a PKS, since I have seen crashes in the
past during shutdown due to this class.  I don't think this CL makes that more
problematic, since this class today depends directly on the Profile pointer. 
Depending on how big this CL gets, I think making it into a PKS could be done in
a follow up CL.

+    account_tracker_service_ = account_tracker_service;
+  else
+    account_tracker_service_ = AccountTrackerServiceFactory::GetForProfile(
+        delegate_->GetBrowserProfile());
 }
 
 base::string16 ProfileDownloader::GetProfileHostedDomain() const {
-  return profile_hosted_domain_;
+  return base::UTF8ToUTF16(account_info_.hosted_domain);

[Mike Lerman, 2015/04/20 13:25:23]

I'm not convinced we still need these methods at all. The store of truth for
these pieces of data is supposed to be the ATS. We could either remove them
completely and make all things that references them reference the ATS instead
(though that requires consumers of Profile information to know quite a bit about
Signin classes), or have these methods get the value from the ATS.

[Roger Tawa OOO till Jul 10th, 2015/04/21 14:00:33]

Agreed we don't need these methods.  As is, the code is already getting the
values from ATS, since they come from the account_info_ member.  Again,
depending on hos big this CL gets, this cleanup could be done in a follow up CL.

 }
 
 base::string16 ProfileDownloader::GetProfileFullName() const {
-  return profile_full_name_;
+  return base::UTF8ToUTF16(account_info_.full_name);
 }
 
 base::string16 ProfileDownloader::GetProfileGivenName() const {
-  return profile_given_name_;
+  return base::UTF8ToUTF16(account_info_.given_name);
 }
 
 std::string ProfileDownloader::GetProfileLocale() const {
-  return profile_locale_;
+  return account_info_.locale;
 }
 
 SkBitmap ProfileDownloader::GetProfilePicture() const {
   return profile_picture_;
 }
 
 ProfileDownloader::PictureStatus ProfileDownloader::GetProfilePictureStatus()
     const {
   return picture_status_;
 }
 
 std::string ProfileDownloader::GetProfilePictureURL() const {
-  return picture_url_;
+  return account_info_.picture_url;

[Mike Lerman, 2015/04/20 13:25:23]

This used to be the result of GetImageURLWithSize. Now it's the raw url from the
ATS - these aren't the same. I expect this would have side effects.

[anthonyvd, 2015/04/29 15:15:08]

You're right, fixed!

+}
+
+void ProfileDownloader::StartFetchingImageForTesting(
+    net::URLFetcher* image_fetcher) {
+  profile_image_fetcher_.reset(image_fetcher);
+  StartFetchingImage();
 }
 
 void ProfileDownloader::StartFetchingImage() {
   VLOG(1) << "Fetching user entry with token: " << auth_token_;
-  gaia_client_.reset(new gaia::GaiaOAuthClient(
-      delegate_->GetBrowserProfile()->GetRequestContext()));
-  gaia_client_->GetUserInfo(auth_token_, 0, this);
+

[Mike Lerman, 2015/04/20 13:25:23]

nit: Delete this empty line.

[anthonyvd, 2015/04/29 15:15:08]

Done.

+  account_info_ = account_tracker_service_->GetAccountInfo(account_id_);
+
+  if (account_info_.IsValid()) {
+    FetchImageData(account_info_.picture_url);
+  } else {
+    account_tracker_service_->AddObserver(this);

[Mike Lerman, 2015/04/20 13:25:23]

I think the idea here is that the ATS is still fetching the AccountInfo but will
soon, so we wait for that fetch to complete. Two concerns:
1) How do we know that the ATS is actually working?
2) How do we know that the result we will observe will be for the Primary
account?

We should start observing the ATS as soon as we get a reference to it, Remove
ourselves as an observer when appropriate (on deletion of this object? Perhaps
the ATS will have to be responsible, I'm not sure if there's any consistent
guidelines around the lifetime of the ProfileDownloader, unless we make it a
PKS). That way the ProfileDownloader stays up to date with the ATS's updates.
Also, this would imply the observer methods need to verify we're being notified
for the Primary Account, which is a good thing.

[Roger Tawa OOO till Jul 10th, 2015/04/21 14:00:33]

Right, would be better to get pointer to ATS in ctor and start observing right
away.

I don't think we want to add knowledge specific about the primary account.
Instead the observer should check that the notification is for the account
|account_id_|.

[anthonyvd, 2015/04/29 15:15:08]

Makes sense, done. The new version starts observing right away and keeps
account_info_ up to date considering the account id the downloader was started
with.

+  }
 }
 
 void ProfileDownloader::StartFetchingOAuth2AccessToken() {
   Profile* profile = delegate_->GetBrowserProfile();
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kGoogleUserInfoProfile);
   // Increase scope to get hd attribute to determine if lock should be enabled.
   if (switches::IsNewProfileManagement())
     scopes.insert(GaiaConstants::kGoogleUserInfoEmail);
   ProfileOAuth2TokenService* token_service =
       ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
   oauth2_access_token_request_ = token_service->StartRequest(
       account_id_, scopes, this);
 }
 
 ProfileDownloader::~ProfileDownloader() {
   // Ensures PO2TS observation is cleared when ProfileDownloader is destructed
   // before refresh token is available.
   ProfileOAuth2TokenService* service =
       ProfileOAuth2TokenServiceFactory::GetForProfile(
           delegate_->GetBrowserProfile());
   if (service)
     service->RemoveObserver(this);
 }
 
-void ProfileDownloader::OnGetUserInfoResponse(
-    scoped_ptr<base::DictionaryValue> user_info) {
-  std::string image_url;
-  if (!ParseProfileJSON(user_info.get(),
-                        &profile_full_name_,
-                        &profile_given_name_,
-                        &image_url,
-                        delegate_->GetDesiredImageSideLength(),
-                        &profile_locale_,
-                        &profile_hosted_domain_)) {
-    delegate_->OnProfileDownloadFailure(
-        this, ProfileDownloaderDelegate::SERVICE_ERROR);
+void ProfileDownloader::FetchImageData(std::string image_url) {

[Roger Tawa OOO till Jul 10th, 2015/04/21 14:00:33]

This should probably be a no-arg function.  All callers pass
|account_info_.picture_url| as arg.

[anthonyvd, 2015/04/29 15:15:08]

Done.

+  GURL url;
+  if (!GetImageURLWithSize(GURL(image_url),
+                          delegate_->GetDesiredImageSideLength(),
+                          &url)) {
     return;
   }
+  std::string image_url_with_size = url.spec();
+
   if (!delegate_->NeedsProfilePicture()) {
     VLOG(1) << "Skipping profile picture download";
     delegate_->OnProfileDownloadSuccess(this);
     return;
   }
-  if (IsDefaultProfileImageURL(image_url)) {
+  if (IsDefaultProfileImageURL(image_url_with_size)) {
     VLOG(1) << "User has default profile picture";
     picture_status_ = PICTURE_DEFAULT;
     delegate_->OnProfileDownloadSuccess(this);
     return;
   }
-  if (!image_url.empty() && image_url == delegate_->GetCachedPictureURL()) {
+  if (!image_url_with_size.empty() &&
+      image_url_with_size == delegate_->GetCachedPictureURL()) {
     VLOG(1) << "Picture URL matches cached picture URL";
     picture_status_ = PICTURE_CACHED;
     delegate_->OnProfileDownloadSuccess(this);
     return;
   }
-  VLOG(1) << "Fetching profile image from " << image_url;
-  picture_url_ = image_url;
-  profile_image_fetcher_.reset(net::URLFetcher::Create(
-      GURL(image_url), net::URLFetcher::GET, this));
-  profile_image_fetcher_->SetRequestContext(
-      delegate_->GetBrowserProfile()->GetRequestContext());
-  profile_image_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                                       net::LOAD_DO_NOT_SAVE_COOKIES);
+  VLOG(1) << "Fetching profile image from " << image_url_with_size;
+
+  if (!profile_image_fetcher_) {

[Mike Lerman, 2015/04/20 13:25:23]

Why not reset and Create a new fetcher? That way, if the ImageURL or other data
gets changed for some reason, we'll fetch the Newest and Latest image rather
than ignore the update and sticking with the old image.

[anthonyvd, 2015/04/29 15:15:08]

That makes a lot of sense. I wasn't resetting the fetcher inside this function
to be able to mock it in unit tests (see
ProfileDownloader::StartFetchingImageForTesting above) but I've taken a
different approach now. Let me know what you think!

[Mike Lerman, 2015/04/30 19:09:06]

commented in the header - use the TestSigninClient in tests, and use the
SigninClient's URLRequestContext in your URLFetcher rather than the Profile's.

[anthonyvd, 2015/05/01 19:16:33]

See header, done!

+    profile_image_fetcher_.reset(net::URLFetcher::Create(
+        GURL(image_url_with_size), net::URLFetcher::GET, this));
+    profile_image_fetcher_->SetRequestContext(
+        delegate_->GetBrowserProfile()->GetRequestContext());
+    profile_image_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                                         net::LOAD_DO_NOT_SAVE_COOKIES);
+  }
+
   if (!auth_token_.empty()) {
     profile_image_fetcher_->SetExtraRequestHeaders(
         base::StringPrintf(kAuthorizationHeader, auth_token_.c_str()));
   }
   profile_image_fetcher_->Start();
 }
 
-void ProfileDownloader::OnOAuthError() {
-  LOG(WARNING) << "OnOAuthError: Fetching profile data failed";
-  delegate_->OnProfileDownloadFailure(
-      this, ProfileDownloaderDelegate::SERVICE_ERROR);
-}
-
-void ProfileDownloader::OnNetworkError(int response_code) {
-  LOG(WARNING) << "OnNetworkError: Fetching profile data failed";
-  DVLOG(1) << "  Response code: " << response_code;
-  delegate_->OnProfileDownloadFailure(
-      this, ProfileDownloaderDelegate::NETWORK_ERROR);
-}
-
 void ProfileDownloader::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   std::string data;
   source->GetResponseAsString(&data);
   bool network_error =
       source->GetStatus().status() != net::URLRequestStatus::SUCCESS;
   if (network_error || source->GetResponseCode() != 200) {

[Mike Lerman, 2015/04/20 13:25:22]

We should include automatic retry with exponential backoff for transient errors.

Also, if our Access Token was stale/invalid (such as if OnAccountUpdated was
called and we didn't have a good access token at the time) we should fetch a new
token and try again with it.

[anthonyvd, 2015/04/29 15:15:08]

RE: automatic retry: What's the desired behavior on failure? Should we retry
transparently or should we still call OnProfileDownloadFailure and then retry? I
imagine the former is the way to go but just to be sure.

As for exponential backoff, which time values should be used?

[Mike Lerman, 2015/04/30 19:09:07]

Don't call OnProfileDownloadFailure - that means any dependent service would
think this had failed fully, which is not the case. I'd agree with the former.

Time values? There are some options in GaiaCookieManagerService or in
ProfileSyncService in the net::Backoff constant.

[anthonyvd, 2015/05/01 19:16:33]

Awesome. I suggest doing it in a follow up CL since this one already changes a
lot of things and there are multiple sites were there needs to be retries. Makes
sense?

     LOG(WARNING) << "Fetching profile data failed";
     DVLOG(1) << "  Status: " << source->GetStatus().status();
     DVLOG(1) << "  Error: " << source->GetStatus().error();
     DVLOG(1) << "  Response code: " << source->GetResponseCode();
     DVLOG(1) << "  Url: " << source->GetURL().spec();
     delegate_->OnProfileDownloadFailure(this, network_error ?
         ProfileDownloaderDelegate::NETWORK_ERROR :
         ProfileDownloaderDelegate::SERVICE_ERROR);
-    return;
+  } else {
+    VLOG(1) << "Decoding the image...";
+    ImageDecoder::Start(this, data);
   }
-
-  VLOG(1) << "Decoding the image...";
-  ImageDecoder::Start(this, data);
+  profile_image_fetcher_.reset();

[Mike Lerman, 2015/04/20 13:25:23]

Do we need to reset() the profile_image_fetcher_ before calling
OnProfileDownloadFailure(), in case the delegate's reaction is to Synchronously
retry the fetch?

[anthonyvd, 2015/04/29 15:15:08]

Can fetchers be reused? Let's say the delegate decided to retry, would calling
Start() a second time on the same fetcher have the expected behavior?

In this case the right thing is probably to reset() before calling
OnProfileDownloadFailure in case the image url changed anyways but I'm just
wondering.

[Mike Lerman, 2015/04/30 19:09:07]

You're now always reset()ing at line 295, so now it's not an issue, I think.

[anthonyvd, 2015/05/01 19:16:33]

Right, it was just a generally useful thing to know :)

 }
 
 void ProfileDownloader::OnImageDecoded(const SkBitmap& decoded_image) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   int image_size = delegate_->GetDesiredImageSideLength();
   profile_picture_ = skia::ImageOperations::Resize(
       decoded_image,
       skia::ImageOperations::RESIZE_BEST,
       image_size,
       image_size);
@@ skipping 34 matching lines @@
 void ProfileDownloader::OnGetTokenFailure(
     const OAuth2TokenService::Request* request,
     const GoogleServiceAuthError& error) {
   DCHECK_EQ(request, oauth2_access_token_request_.get());
   oauth2_access_token_request_.reset();
   LOG(WARNING) << "ProfileDownloader: token request using refresh token failed:"
                << error.ToString();
   delegate_->OnProfileDownloadFailure(
       this, ProfileDownloaderDelegate::TOKEN_ERROR);
 }
+
+void ProfileDownloader::OnAccountUpdated(
+    const AccountTrackerService::AccountInfo& info) {
+  if (info.account_id == account_id_ && info.IsValid()) {
+    account_tracker_service_->RemoveObserver(this);
+    account_info_ = info;
+    FetchImageData(account_info_.picture_url);

[Mike Lerman, 2015/04/20 13:25:23]

How do we know the Access Token is valid when FetchImageData is called? Either
fetch a new one here, or ensure OnURLFetchComplete is robust to failures.

[Roger Tawa OOO till Jul 10th, 2015/04/21 14:00:34]

I think this CL needs to turn the order of operations around.  Looking at the
old code, it did this:

. when StartForAccount() is called, look for a refresh token.  If available, use
it.  Otherwise wait an indefinite amount of time for the token.
. when the token is available, call userinfo.
. when userinfo returns, fetch the image.

The new code does this:

. when StartForAccount() is called, look for a refresh token.  If available, use
it.  Otherwise wait an indefinite amount of time for the token.
. when the the token is available, see if the ATS has valid info.  If not, wait
for an indefinite amount of time.
. once the info is available, fetch the image.

It might be better to do the following

[anthonyvd, 2015/04/29 15:15:08]

The last part of that comment seems to have been cut off, can you clarify?

+  }
+}
+
+void ProfileDownloader::OnAccountUpdateFailed(const std::string& account_id) {
+  if (account_id == account_id_) {
+    account_tracker_service_->RemoveObserver(this);
+    delegate_->OnProfileDownloadFailure(
+        this, ProfileDownloaderDelegate::SERVICE_ERROR);
+  }
+}