Revert "Avoid conversion between GURL and std::string"

This reverts commit 87182d18.

Reason for revert: Breaks browser_tests and viz_browser_tests.

Example:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/28716

Triggers DCHECK on line 859:
https://chromium.googlesource.com/chromium/src/+blame/87182d18c9a56d1860288e4e92e726e1f399bf97/chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc#859


Original change's description:
> 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}

TBR=bauerb@chromium.org,msarda@chromium.org,sdefresne@chromium.org,jdufault@chromium.org

Change-Id: Ib1cc81bef47d279214f63528e78d4d885a7aa0e5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: none
Reviewed-on: https://chromium-review.googlesource.com/1186761Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585477}

chrome/browser/android/profiles/profile_downloader_android.cc

@@ -66,7 +66,9 @@ class AccountInfoRetriever : public ProfileDownloaderDelegate {
     return profile_;
   }
 
-  GURL GetCachedPictureURL() const override { return GURL(); }
+  std::string GetCachedPictureURL() const override {
+    return std::string();
+  }
 
   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());
 }
 
-GURL UserImageManagerImpl::GetCachedPictureURL() const {
-  return profile_image_url_;
+std::string UserImageManagerImpl::GetCachedPictureURL() const {
+  return profile_image_url_.spec();
 }
 
 bool UserImageManagerImpl::IsPreSignin() const {
@@ -855,8 +855,7 @@ void UserImageManagerImpl::OnProfileDownloadSuccess(
 
   downloaded_profile_image_ =
       gfx::ImageSkia::CreateFrom1xBitmap(downloader->GetProfilePicture());
-  profile_image_url_ = downloader->GetProfilePictureURL();
-  DCHECK(profile_image_url_.is_valid());
+  profile_image_url_ = GURL(downloader->GetProfilePictureURL());
 
   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;
-  GURL GetCachedPictureURL() const override;
+  std::string GetCachedPictureURL() const override;
   bool IsPreSignin() const override;
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override;
   void OnProfileDownloadFailure(

chrome/browser/profiles/gaia_info_update_service.cc

@@ -86,9 +86,8 @@ Profile* GAIAInfoUpdateService::GetBrowserProfile() {
   return profile_;
 }
 
-GURL GAIAInfoUpdateService::GetCachedPictureURL() const {
-  return GURL(
-      profile_->GetPrefs()->GetString(prefs::kProfileGAIAInfoPictureURL));
+std::string GAIAInfoUpdateService::GetCachedPictureURL() const {
+  return profile_->GetPrefs()->GetString(prefs::kProfileGAIAInfoPictureURL);
 }
 
 bool GAIAInfoUpdateService::IsPreSignin() const {
@@ -112,8 +111,7 @@ void GAIAInfoUpdateService::OnProfileDownloadSuccess(
   SkBitmap bitmap = downloader->GetProfilePicture();
   ProfileDownloader::PictureStatus picture_status =
       downloader->GetProfilePictureStatus();
-  GURL picture_url = downloader->GetProfilePictureURL();
-  DCHECK(picture_url.is_valid());
+  std::string picture_url = downloader->GetProfilePictureURL();
 
   ProfileAttributesEntry* entry;
   if (!g_browser_process->profile_manager()->GetProfileAttributesStorage().
@@ -126,7 +124,7 @@ void GAIAInfoUpdateService::OnProfileDownloadSuccess(
 
   if (picture_status == ProfileDownloader::PICTURE_SUCCESS) {
     profile_->GetPrefs()->SetString(prefs::kProfileGAIAInfoPictureURL,
-                                    picture_url.spec());
+                                    picture_url);
     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;
-  GURL GetCachedPictureURL() const override;
+  std::string 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, GURL());
+  MOCK_CONST_METHOD0(GetProfilePictureURL, std::string());
   MOCK_CONST_METHOD0(GetProfileHostedDomain, base::string16());
 };
 
@@ -120,11 +120,12 @@ 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 GURL& url,
-                              const base::string16& hosted_domain) {
+  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) {
     EXPECT_CALL(*downloader(), GetProfileFullName()).
         WillOnce(Return(full_name));
     EXPECT_CALL(*downloader(), GetProfileGivenName()).
@@ -143,7 +144,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);
-    GURL url("https://foo.com");
+    std::string url("foo.com");
     ProfileDownloadSuccess(full_name, given_name, image, url, base::string16());
 
     // Make sure the right profile was updated correctly.
@@ -180,14 +181,14 @@ void GAIAInfoUpdateServiceTest::TearDown() {
 
 TEST_F(GAIAInfoUpdateServiceTest, DownloadSuccess) {
   // No URL should be cached yet.
-  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
+  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
   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);
-  GURL url("https://foo.com");
+  std::string url("foo.com");
   base::string16 hosted_domain(base::ASCIIToUTF16(""));
   ProfileDownloadSuccess(name, given_name, image, url, hosted_domain);
 
@@ -211,7 +212,7 @@ TEST_F(GAIAInfoUpdateServiceTest, DownloadFailure) {
   base::string16 old_name = entry->GetName();
   gfx::Image old_image = entry->GetAvatarIcon();
 
-  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
+  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
 
   service()->OnProfileDownloadFailure(downloader(),
                                       ProfileDownloaderDelegate::SERVICE_ERROR);
@@ -222,19 +223,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_TRUE(service()->GetCachedPictureURL().is_empty());
+  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
   EXPECT_EQ(std::string(),
       profile()->GetPrefs()->GetString(prefs::kGoogleServicesHostedDomain));
 }
 
 TEST_F(GAIAInfoUpdateServiceTest, ProfileLockEnabledForWhitelist) {
   // No URL should be cached yet.
-  EXPECT_TRUE(service()->GetCachedPictureURL().is_empty());
+  EXPECT_EQ(std::string(), service()->GetCachedPictureURL());
 
   base::string16 name = base::ASCIIToUTF16("Pat Smith");
   base::string16 given_name = base::ASCIIToUTF16("Pat");
   gfx::Image image = gfx::test::CreateImage(256, 256);
-  GURL url("https://foo.com");
+  std::string url("foo.com");
   base::string16 hosted_domain(base::ASCIIToUTF16("google.com"));
   ProfileDownloadSuccess(name, given_name, image, url, hosted_domain);
 
@@ -315,9 +316,9 @@ TEST_F(GAIAInfoUpdateServiceTest, LogOut) {
 
   // Set a fake picture URL.
   profile()->GetPrefs()->SetString(prefs::kProfileGAIAInfoPictureURL,
-                                   "https://example.com");
+                                   "example.com");
 
-  EXPECT_FALSE(service()->GetCachedPictureURL().is_empty());
+  EXPECT_FALSE(service()->GetCachedPictureURL().empty());
 
   // Log out.
   identity::ClearPrimaryAccount(
@@ -326,7 +327,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().is_empty());
+  EXPECT_TRUE(service()->GetCachedPictureURL().empty());
 }
 
 TEST_F(GAIAInfoUpdateServiceTest, LogIn) {

chrome/browser/profiles/profile_downloader.cc

@@ -107,12 +107,14 @@ ProfileDownloader::PictureStatus ProfileDownloader::GetProfilePictureStatus()
   return picture_status_;
 }
 
-GURL ProfileDownloader::GetProfilePictureURL() const {
-  const GURL url(account_info_.picture_url);
+std::string ProfileDownloader::GetProfilePictureURL() const {
+  GURL url(account_info_.picture_url);
   if (!url.is_valid())
-    return GURL();
+    return std::string();
   return signin::GetAvatarImageURLWithOptions(
-      url, delegate_->GetDesiredImageSideLength(), true /* no_silhouette */);
+             GURL(account_info_.picture_url),
+             delegate_->GetDesiredImageSideLength(), true /* no_silhouette */)
+      .spec();
 }
 
 void ProfileDownloader::StartFetchingImage() {
@@ -170,15 +172,16 @@ void ProfileDownloader::FetchImageData() {
     return;
   }
 
-  GURL image_url_to_fetch = GetProfilePictureURL();
-  if (image_url_to_fetch.is_valid() &&
-      image_url_to_fetch == delegate_->GetCachedPictureURL()) {
+  std::string image_url_with_size = GetProfilePictureURL();
+  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;
   }
 
+  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,7 +23,6 @@
 namespace identity {
 class PrimaryAccountAccessTokenFetcher;
 }
-class GURL;
 class ProfileDownloaderDelegate;
 
 // Downloads user profile information. The profile picture is decoded in a
@@ -77,7 +76,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 GURL GetProfilePictureURL() const;
+  virtual std::string 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 URL when there is no
+  // the image will not be downloaded. Return an empty string when there is no
   // cached URL.
-  virtual GURL GetCachedPictureURL() const = 0;
+  virtual std::string 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 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";
+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";
 
 } // namespace
 
@@ -57,7 +57,7 @@ class ProfileDownloaderTest : public testing::Test,
 
   bool NeedsProfilePicture() const override { return true; };
   int GetDesiredImageSideLength() const override { return 128; };
-  GURL GetCachedPictureURL() const override { return GURL(); };
+  std::string GetCachedPictureURL() const override { return ""; };
   Profile* GetBrowserProfile() override { return profile_.get(); };
   bool IsPreSignin() const override { return false; }
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override {
@@ -87,25 +87,23 @@ TEST_F(ProfileDownloaderTest, AccountInfoReady) {
       account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
   SimulateUserInfoSuccess(kTestValidPictureURL);
 
-  EXPECT_EQ(ProfileDownloader::PICTURE_FAILED,
+  ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
             profile_downloader_->GetProfilePictureStatus());
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
-  EXPECT_EQ(GURL(kTestValidPictureURL),
-            profile_downloader_->GetProfilePictureURL());
+  ASSERT_EQ(kTestValidPictureURL, profile_downloader_->GetProfilePictureURL());
 }
 
 TEST_F(ProfileDownloaderTest, AccountInfoNotReady) {
   std::string account_id =
       account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
 
-  EXPECT_EQ(ProfileDownloader::PICTURE_FAILED,
+  ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
             profile_downloader_->GetProfilePictureStatus());
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
   SimulateUserInfoSuccess(kTestValidPictureURL);
-  EXPECT_EQ(GURL(kTestValidPictureURL),
-            profile_downloader_->GetProfilePictureURL());
+  ASSERT_EQ(kTestValidPictureURL, profile_downloader_->GetProfilePictureURL());
 }
 
 // Regression test for http://crbug.com/854907
@@ -117,8 +115,8 @@ TEST_F(ProfileDownloaderTest, AccountInfoNoPictureDoesNotCrash) {
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
 
-  EXPECT_FALSE(profile_downloader_->GetProfilePictureURL().is_valid());
-  EXPECT_EQ(ProfileDownloader::PICTURE_DEFAULT,
+  EXPECT_TRUE(profile_downloader_->GetProfilePictureURL().empty());
+  ASSERT_EQ(ProfileDownloader::PICTURE_DEFAULT,
             profile_downloader_->GetProfilePictureStatus());
 }
 
@@ -131,7 +129,7 @@ TEST_F(ProfileDownloaderTest, AccountInfoInvalidPictureURLDoesNotCrash) {
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
 
-  EXPECT_FALSE(profile_downloader_->GetProfilePictureURL().is_valid());
-  EXPECT_EQ(ProfileDownloader::PICTURE_FAILED,
+  EXPECT_TRUE(profile_downloader_->GetProfilePictureURL().empty());
+  ASSERT_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;
 }
 
-GURL CustodianProfileDownloaderService::GetCachedPictureURL() const {
-  return GURL();
+std::string CustodianProfileDownloaderService::GetCachedPictureURL() const {
+  return std::string();
 }
 
 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;
-  GURL GetCachedPictureURL() const override;
+  std::string GetCachedPictureURL() const override;
   Profile* GetBrowserProfile() override;
   bool IsPreSignin() const override;
   void OnProfileDownloadSuccess(ProfileDownloader* downloader) override;