Avoid conversion between GURL and std::string

ProfileDownloader::GetProfilePictureURL returns a std::string
corresponding to the representation of a GURL then immediately
converts it back to a GURL.

This is wasteful, so convert this method to return a GURL
instead and convert GetCachedPictureURL to also cache the
GURL instead of the string representation.

Convert static std::string to static char[] and use EXPECT_
instead of ASSERT_ macros when possible in unit tests (since
the EXPECT_ macro do not stop the test in case of failure).

Bug: none
Change-Id: Ibe73f28b38b2061b37cbdb0b9b0696b25ca8c41c
Reviewed-on: https://chromium-review.googlesource.com/1160645
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585426}

chrome/browser/android/profiles/profile_downloader_android.cc

@@ -66,9 +66,7 @@ class AccountInfoRetriever : public ProfileDownloaderDelegate {
     return profile_;
   }
 
-  std::string GetCachedPictureURL() const override {
-    return std::string();
-  }
+  GURL GetCachedPictureURL() const override { return GURL(); }
 
   bool IsPreSignin() const override {
     return is_pre_signin_;

chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc

@@ -792,8 +792,8 @@ Profile* UserImageManagerImpl::GetBrowserProfile() {
   return ProfileHelper::Get()->GetProfileByUserUnsafe(GetUser());
 }
 
-std::string UserImageManagerImpl::GetCachedPictureURL() const {
-  return profile_image_url_.spec();
+GURL UserImageManagerImpl::GetCachedPictureURL() const {
+  return profile_image_url_;
 }
 
 bool UserImageManagerImpl::IsPreSignin() const {
@@ -855,7 +855,8 @@ void UserImageManagerImpl::OnProfileDownloadSuccess(
 
   downloaded_profile_image_ =
       gfx::ImageSkia::CreateFrom1xBitmap(downloader->GetProfilePicture());
-  profile_image_url_ = GURL(downloader->GetProfilePictureURL());
+  profile_image_url_ = downloader->GetProfilePictureURL();
+  DCHECK(profile_image_url_.is_valid());
 
   if (user->image_index() == user_manager::User::USER_IMAGE_PROFILE) {
     VLOG(1) << "Updating profile image for logged-in user.";

chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.h

@@ -95,7 +95,7 @@ class UserImageManagerImpl : public UserImageManager,
   bool NeedsProfilePicture() const override;
   int GetDesiredImageSideLength() const override;
   Profile* GetBrowserProfile() override;
-  std::string GetCachedPictureURL() const override;
+  GURL GetCachedPictureURL() const override;
   bool IsPreSignin() const override;
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override;
   void OnProfileDownloadFailure(

chrome/browser/profiles/gaia_info_update_service.cc

@@ -86,8 +86,9 @@ Profile* GAIAInfoUpdateService::GetBrowserProfile() {
   return profile_;
 }
 
-std::string GAIAInfoUpdateService::GetCachedPictureURL() const {
-  return profile_->GetPrefs()->GetString(prefs::kProfileGAIAInfoPictureURL);
+GURL GAIAInfoUpdateService::GetCachedPictureURL() const {
+  return GURL(
+      profile_->GetPrefs()->GetString(prefs::kProfileGAIAInfoPictureURL));
 }
 
 bool GAIAInfoUpdateService::IsPreSignin() const {
@@ -111,7 +112,8 @@ void GAIAInfoUpdateService::OnProfileDownloadSuccess(
   SkBitmap bitmap = downloader->GetProfilePicture();
   ProfileDownloader::PictureStatus picture_status =
       downloader->GetProfilePictureStatus();
-  std::string picture_url = downloader->GetProfilePictureURL();
+  GURL picture_url = downloader->GetProfilePictureURL();
+  DCHECK(picture_url.is_valid());
 
   ProfileAttributesEntry* entry;
   if (!g_browser_process->profile_manager()->GetProfileAttributesStorage().
@@ -124,7 +126,7 @@ void GAIAInfoUpdateService::OnProfileDownloadSuccess(
 
   if (picture_status == ProfileDownloader::PICTURE_SUCCESS) {
     profile_->GetPrefs()->SetString(prefs::kProfileGAIAInfoPictureURL,
-                                    picture_url);
+                                    picture_url.spec());
     gfx::Image gfx_image = gfx::Image::CreateFrom1xBitmap(bitmap);
     entry->SetGAIAPicture(&gfx_image);
   } else if (picture_status == ProfileDownloader::PICTURE_DEFAULT) {

chrome/browser/profiles/gaia_info_update_service.h

@@ -38,7 +38,7 @@ class GAIAInfoUpdateService : public KeyedService,
   bool NeedsProfilePicture() const override;
   int GetDesiredImageSideLength() const override;
   Profile* GetBrowserProfile() override;
-  std::string GetCachedPictureURL() const override;
+  GURL GetCachedPictureURL() const override;
   bool IsPreSignin() const override;
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override;
   void OnProfileDownloadFailure(

chrome/browser/profiles/gaia_info_update_service_unittest.cc

@@ -53,7 +53,7 @@ class ProfileDownloaderMock : public ProfileDownloader {
   MOCK_CONST_METHOD0(GetProfilePicture, SkBitmap());
   MOCK_CONST_METHOD0(GetProfilePictureStatus,
                      ProfileDownloader::PictureStatus());
-  MOCK_CONST_METHOD0(GetProfilePictureURL, std::string());
+  MOCK_CONST_METHOD0(GetProfilePictureURL, GURL());
   MOCK_CONST_METHOD0(GetProfileHostedDomain, base::string16());
 };
 
@@ -120,12 +120,11 @@ class GAIAInfoUpdateServiceTest : public ProfileInfoCacheTest {
     return base::UTF8ToUTF16(FullName(id));
   }
 
-  void ProfileDownloadSuccess(
-      const base::string16& full_name,
-      const base::string16& given_name,
-      const gfx::Image& image,
-      const std::string& url,
-      const base::string16& hosted_domain) {
+  void ProfileDownloadSuccess(const base::string16& full_name,
+                              const base::string16& given_name,
+                              const gfx::Image& image,
+                              const GURL& url,
+                              const base::string16& hosted_domain) {
     EXPECT_CALL(*downloader(), GetProfileFullName()).
         WillOnce(Return(full_name));
     EXPECT_CALL(*downloader(), GetProfileGivenName()).
@@ -144,7 +143,7 @@ class GAIAInfoUpdateServiceTest : public ProfileInfoCacheTest {
   void RenameProfile(const base::string16& full_name,
                      const base::string16& given_name) {
     gfx::Image image = gfx::test::CreateImage(256, 256);
-    std::string url("foo.com");
+    GURL url("https://foo.com");
     ProfileDownloadSuccess(full_name, given_name, image, url, base::string16());
 
     // Make sure the right profile was updated correctly.
@@ -181,14 +180,14 @@ void GAIAInfoUpdateServiceTest::TearDown() {
 
 TEST_F(GAIAInfoUpdateServiceTest, DownloadSuccess) {
   // No URL should be cached yet.
-  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
+  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
   EXPECT_EQ(std::string(), profile()->GetPrefs()->
       GetString(prefs::kGoogleServicesHostedDomain));
 
   base::string16 name = base::ASCIIToUTF16("Pat Smith");
   base::string16 given_name = base::ASCIIToUTF16("Pat");
   gfx::Image image = gfx::test::CreateImage(256, 256);
-  std::string url("foo.com");
+  GURL url("https://foo.com");
   base::string16 hosted_domain(base::ASCIIToUTF16(""));
   ProfileDownloadSuccess(name, given_name, image, url, hosted_domain);
 
@@ -212,7 +211,7 @@ TEST_F(GAIAInfoUpdateServiceTest, DownloadFailure) {
   base::string16 old_name = entry->GetName();
   gfx::Image old_image = entry->GetAvatarIcon();
 
-  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
+  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
 
   service()->OnProfileDownloadFailure(downloader(),
                                       ProfileDownloaderDelegate::SERVICE_ERROR);
@@ -223,19 +222,19 @@ TEST_F(GAIAInfoUpdateServiceTest, DownloadFailure) {
   EXPECT_EQ(base::string16(), entry->GetGAIAGivenName());
   EXPECT_TRUE(gfx::test::AreImagesEqual(old_image, entry->GetAvatarIcon()));
   EXPECT_EQ(nullptr, entry->GetGAIAPicture());
-  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
+  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
   EXPECT_EQ(std::string(),
       profile()->GetPrefs()->GetString(prefs::kGoogleServicesHostedDomain));
 }
 
 TEST_F(GAIAInfoUpdateServiceTest, ProfileLockEnabledForWhitelist) {
   // No URL should be cached yet.
-  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
+  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
 
   base::string16 name = base::ASCIIToUTF16("Pat Smith");
   base::string16 given_name = base::ASCIIToUTF16("Pat");
   gfx::Image image = gfx::test::CreateImage(256, 256);
-  std::string url("foo.com");
+  GURL url("https://foo.com");
   base::string16 hosted_domain(base::ASCIIToUTF16("google.com"));
   ProfileDownloadSuccess(name, given_name, image, url, hosted_domain);
 
@@ -316,9 +315,9 @@ TEST_F(GAIAInfoUpdateServiceTest, LogOut) {
 
   // Set a fake picture URL.
   profile()->GetPrefs()->SetString(prefs::kProfileGAIAInfoPictureURL,
-                                   "example.com");
+                                   "https://example.com");
 
-  EXPECT_FALSE(service()->GetCachedPictureURL().empty());
+  EXPECT_FALSE(service()->GetCachedPictureURL().is_empty());
 
   // Log out.
   identity::ClearPrimaryAccount(
@@ -327,7 +326,7 @@ TEST_F(GAIAInfoUpdateServiceTest, LogOut) {
   // Verify that the GAIA name and picture, and picture URL are unset.
   EXPECT_TRUE(entry->GetGAIAName().empty());
   EXPECT_EQ(nullptr, entry->GetGAIAPicture());
-  EXPECT_TRUE(service()->GetCachedPictureURL().empty());
+  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
 }
 
 TEST_F(GAIAInfoUpdateServiceTest, LogIn) {

chrome/browser/profiles/profile_downloader.cc

@@ -107,14 +107,12 @@ ProfileDownloader::PictureStatus ProfileDownloader::GetProfilePictureStatus()
   return picture_status_;
 }
 
-std::string ProfileDownloader::GetProfilePictureURL() const {
-  GURL url(account_info_.picture_url);
+GURL ProfileDownloader::GetProfilePictureURL() const {
+  const GURL url(account_info_.picture_url);
   if (!url.is_valid())
-    return std::string();
+    return GURL();
   return signin::GetAvatarImageURLWithOptions(
-             GURL(account_info_.picture_url),
-             delegate_->GetDesiredImageSideLength(), true /* no_silhouette */)
-      .spec();
+      url, delegate_->GetDesiredImageSideLength(), true /* no_silhouette */);
 }
 
 void ProfileDownloader::StartFetchingImage() {
@@ -172,16 +170,15 @@ void ProfileDownloader::FetchImageData() {
     return;
   }
 
-  std::string image_url_with_size = GetProfilePictureURL();
-  if (!image_url_with_size.empty() &&
-      image_url_with_size == delegate_->GetCachedPictureURL()) {
+  GURL image_url_to_fetch = GetProfilePictureURL();
+  if (image_url_to_fetch.is_valid() &&
+      image_url_to_fetch == delegate_->GetCachedPictureURL()) {
     VLOG(1) << "Picture URL matches cached picture URL";
     picture_status_ = PICTURE_CACHED;
     delegate_->OnProfileDownloadSuccess(this);
     return;
   }
 
-  GURL image_url_to_fetch(image_url_with_size);
   if (!image_url_to_fetch.is_valid()) {
     VLOG(1) << "Profile picture URL with size |" << image_url_to_fetch << "| "
             << "is not valid (the account picture URL is "

chrome/browser/profiles/profile_downloader.h

@@ -23,6 +23,7 @@
 namespace identity {
 class PrimaryAccountAccessTokenFetcher;
 }
+class GURL;
 class ProfileDownloaderDelegate;
 
 // Downloads user profile information. The profile picture is decoded in a
@@ -76,7 +77,7 @@ class ProfileDownloader : public ImageDecoder::ImageRequest,
   // Gets the URL for the profile picture. This can be cached so that the same
   // picture is not downloaded multiple times. This value should only be used
   // when the picture status is PICTURE_SUCCESS.
-  virtual std::string GetProfilePictureURL() const;
+  virtual GURL GetProfilePictureURL() const;
 
  private:
   friend class ProfileDownloaderTest;

chrome/browser/profiles/profile_downloader_delegate.h

@@ -35,9 +35,9 @@ class ProfileDownloaderDelegate {
   virtual int GetDesiredImageSideLength() const = 0;
 
   // Returns the cached URL. If the cache URL matches the new image URL
-  // the image will not be downloaded. Return an empty string when there is no
+  // the image will not be downloaded. Return an empty URL when there is no
   // cached URL.
-  virtual std::string GetCachedPictureURL() const = 0;
+  virtual GURL GetCachedPictureURL() const = 0;
 
   // Returns the browser profile associated with this download request.
   virtual Profile* GetBrowserProfile() = 0;

chrome/browser/profiles/profile_downloader_unittest.cc

@@ -23,14 +23,14 @@
 
 namespace {
 
-const std::string kTestEmail = "test@example.com";
-const std::string kTestGaia = "gaia";
-const std::string kTestHostedDomain = "google.com";
-const std::string kTestFullName = "full_name";
-const std::string kTestGivenName = "given_name";
-const std::string kTestLocale = "locale";
-const std::string kTestValidPictureURL = "http://www.google.com/";
-const std::string kTestInvalidPictureURL = "invalid_picture_url";
+const char kTestEmail[] = "test@example.com";
+const char kTestGaia[] = "gaia";
+const char kTestHostedDomain[] = "google.com";
+const char kTestFullName[] = "full_name";
+const char kTestGivenName[] = "given_name";
+const char kTestLocale[] = "locale";
+const char kTestValidPictureURL[] = "http://www.google.com/";
+const char kTestInvalidPictureURL[] = "invalid_picture_url";
 
 } // namespace
 
@@ -57,7 +57,7 @@ class ProfileDownloaderTest : public testing::Test,
 
   bool NeedsProfilePicture() const override { return true; };
   int GetDesiredImageSideLength() const override { return 128; };
-  std::string GetCachedPictureURL() const override { return ""; };
+  GURL GetCachedPictureURL() const override { return GURL(); };
   Profile* GetBrowserProfile() override { return profile_.get(); };
   bool IsPreSignin() const override { return false; }
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override {
@@ -87,23 +87,25 @@ TEST_F(ProfileDownloaderTest, AccountInfoReady) {
       account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
   SimulateUserInfoSuccess(kTestValidPictureURL);
 
-  ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
+  EXPECT_EQ(ProfileDownloader::PICTURE_FAILED,
             profile_downloader_->GetProfilePictureStatus());
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
-  ASSERT_EQ(kTestValidPictureURL, profile_downloader_->GetProfilePictureURL());
+  EXPECT_EQ(GURL(kTestValidPictureURL),
+            profile_downloader_->GetProfilePictureURL());
 }
 
 TEST_F(ProfileDownloaderTest, AccountInfoNotReady) {
   std::string account_id =
       account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
 
-  ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
+  EXPECT_EQ(ProfileDownloader::PICTURE_FAILED,
             profile_downloader_->GetProfilePictureStatus());
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
   SimulateUserInfoSuccess(kTestValidPictureURL);
-  ASSERT_EQ(kTestValidPictureURL, profile_downloader_->GetProfilePictureURL());
+  EXPECT_EQ(GURL(kTestValidPictureURL),
+            profile_downloader_->GetProfilePictureURL());
 }
 
 // Regression test for http://crbug.com/854907
@@ -115,8 +117,8 @@ TEST_F(ProfileDownloaderTest, AccountInfoNoPictureDoesNotCrash) {
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
 
-  EXPECT_TRUE(profile_downloader_->GetProfilePictureURL().empty());
-  ASSERT_EQ(ProfileDownloader::PICTURE_DEFAULT,
+  EXPECT_FALSE(profile_downloader_->GetProfilePictureURL().is_valid());
+  EXPECT_EQ(ProfileDownloader::PICTURE_DEFAULT,
             profile_downloader_->GetProfilePictureStatus());
 }
 
@@ -129,7 +131,7 @@ TEST_F(ProfileDownloaderTest, AccountInfoInvalidPictureURLDoesNotCrash) {
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
 
-  EXPECT_TRUE(profile_downloader_->GetProfilePictureURL().empty());
-  ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
+  EXPECT_FALSE(profile_downloader_->GetProfilePictureURL().is_valid());
+  EXPECT_EQ(ProfileDownloader::PICTURE_FAILED,
             profile_downloader_->GetProfilePictureStatus());
 }

chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.cc

@@ -54,8 +54,8 @@ int CustodianProfileDownloaderService::GetDesiredImageSideLength() const {
   return 0;
 }
 
-std::string CustodianProfileDownloaderService::GetCachedPictureURL() const {
-  return std::string();
+GURL CustodianProfileDownloaderService::GetCachedPictureURL() const {
+  return GURL();
 }
 
 Profile* CustodianProfileDownloaderService::GetBrowserProfile() {

chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h

@@ -33,7 +33,7 @@ class CustodianProfileDownloaderService : public KeyedService,
   // ProfileDownloaderDelegate:
   bool NeedsProfilePicture() const override;
   int GetDesiredImageSideLength() const override;
-  std::string GetCachedPictureURL() const override;
+  GURL GetCachedPictureURL() const override;
   Profile* GetBrowserProfile() override;
   bool IsPreSignin() const override;
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override;