Change ProfileDownloader to use AccountTrackerService.

chrome/browser/profiles/profile_downloader.cc

Index: chrome/browser/profiles/profile_downloader.cc
diff --git a/chrome/browser/profiles/profile_downloader.cc b/chrome/browser/profiles/profile_downloader.cc
index 2377772ebde2ee9a379e2153f591755b8e6a4d2e..8a8387f71c6bf0d1c6d0819eccba973db8c276cc 100644
--- a/chrome/browser/profiles/profile_downloader.cc
+++ b/chrome/browser/profiles/profile_downloader.cc
@@ -13,10 +13,12 @@
 #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"
@@ -39,20 +41,6 @@ namespace {
 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.
@@ -126,47 +114,6 @@ bool GetImageURLWithSize(const GURL& old_url, int size, GURL* new_url) {
 
 }  // 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())
@@ -205,9 +152,14 @@ 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(
@@ -230,22 +182,28 @@ void ProfileDownloader::StartForAccount(const std::string& account_id) {
   } 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 {
@@ -258,14 +216,25 @@ ProfileDownloader::PictureStatus ProfileDownloader::GetProfilePictureStatus()
 }
 
 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() {
@@ -291,45 +260,44 @@ ProfileDownloader::~ProfileDownloader() {
     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()));
@@ -337,19 +305,6 @@ void ProfileDownloader::OnGetUserInfoResponse(
   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;
@@ -365,11 +320,11 @@ void ProfileDownloader::OnURLFetchComplete(const net::URLFetcher* source) {
     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) {
@@ -424,3 +379,20 @@ void ProfileDownloader::OnGetTokenFailure(
   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);
+  }
+}