Revert "Add support for FIFE Avatar URL format"

This reverts commit fa0a6456.

Reason for revert: Reverting again after confirming that this change turned a flaky test into a consistently crashing test.

Original change's description:
> Add support for FIFE Avatar URL format
> 
> The method GetAvatarImageURLWithOptions() is updated to support
> image URLs coming in the FIFE Avatar URL format.
> 
> This should resolve issues related to color-inverted account images
> on the Chrome surface.
> 
> Bug: 911332
> Change-Id: I8c0a10be69bc87a40a39cf46e0428b418d97b021
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1670888
> Commit-Queue: Thomas Tangl <tangltom@chromium.org>
> Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#672116}

TBR=msarda@chromium.org,bsazonov@chromium.org,tangltom@chromium.org

Change-Id: Ic6d0b1b4080c2c795a2c60ae7bdec85324ad1751
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 911332,978578
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1676836Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672252}

components/signin/core/browser/avatar_icon_util.cc

@@ -5,7 +5,6 @@
 #include "components/signin/core/browser/avatar_icon_util.h"
 
 #include <string>
-#include <vector>
 
 #include "base/stl_util.h"
 #include "base/strings/string_split.h"
@@ -17,25 +16,20 @@
 namespace {
 
 // Separator of URL path components.
-constexpr char kURLPathSeparator[] = "/";
+const char kURLPathSeparator[] = "/";
 
-// Constants describing legacy image URL format.
+// Constants describing image URL format.
 // See https://crbug.com/733306#c3 for details.
-constexpr size_t kLegacyURLPathComponentsCount = 6;
-constexpr size_t kLegacyURLPathComponentsCountWithOptions = 7;
-constexpr size_t kLegacyURLPathOptionsComponentPosition = 5;
-// Constants describing content image URL format.
-// See https://crbug.com/911332#c15 for details.
-constexpr size_t kContentURLPathMinComponentsCount = 2;
-constexpr size_t kContentURLPathMaxComponentsCount = 3;
-constexpr char kContentURLOptionsStartChar = '=';
+const size_t kImageURLPathComponentsCount = 6;
+const size_t kImageURLPathComponentsCountWithOptions = 7;
+const size_t kImageURLPathOptionsComponentPosition = 5;
 // Various options that can be embedded in image URL.
-constexpr char kImageURLOptionSeparator[] = "-";
-constexpr char kImageURLOptionSizePattern[] = R"(s\d+)";
-constexpr char kImageURLOptionSizeFormat[] = "s%d";
-constexpr char kImageURLOptionSquareCrop[] = "c";
+const char kImageURLOptionSeparator[] = "-";
+const char kImageURLOptionSizePattern[] = R"(s\d+)";
+const char kImageURLOptionSizeFormat[] = "s%d";
+const char kImageURLOptionSquareCrop[] = "c";
 // Option to disable default avatar if user doesn't have a custom one.
-constexpr char kImageURLOptionNoSilhouette[] = "ns";
+const char kImageURLOptionNoSilhouette[] = "ns";
 
 std::string BuildImageURLOptionsString(int image_size,
                                        bool no_silhouette,
@@ -59,62 +53,6 @@ std::string BuildImageURLOptionsString(int image_size,
   return base::JoinString(url_options, kImageURLOptionSeparator);
 }
 
-// Returns an empty vector if |url_components| couldn't be processed as a legacy
-// image URL.
-std::vector<std::string> TryProcessAsLegacyImageURL(
-    std::vector<std::string> url_components,
-    int image_size,
-    bool no_silhouette) {
-  if (url_components.back().empty())
-    return {};
-
-  if (url_components.size() == kLegacyURLPathComponentsCount) {
-    url_components.insert(
-        url_components.begin() + kLegacyURLPathOptionsComponentPosition,
-        BuildImageURLOptionsString(image_size, no_silhouette, std::string()));
-    return url_components;
-  }
-
-  if (url_components.size() == kLegacyURLPathComponentsCountWithOptions) {
-    std::string options =
-        url_components.at(kLegacyURLPathOptionsComponentPosition);
-    url_components[kLegacyURLPathOptionsComponentPosition] =
-        BuildImageURLOptionsString(image_size, no_silhouette, options);
-    return url_components;
-  }
-
-  return {};
-}
-
-// Returns an empty vector if |url_components| couldn't be processed as a
-// content image URL.
-std::vector<std::string> TryProcessAsContentImageURL(
-    std::vector<std::string> url_components,
-    int image_size,
-    bool no_silhouette) {
-  if (url_components.size() < kContentURLPathMinComponentsCount ||
-      url_components.size() > kContentURLPathMaxComponentsCount ||
-      url_components.back().empty()) {
-    return {};
-  }
-
-  std::string* options_component = &url_components.back();
-  // Extract existing options from |options_component|.
-  const size_t options_pos =
-      options_component->find(kContentURLOptionsStartChar);
-  std::string component_without_options =
-      options_component->substr(0, options_pos);
-  std::string existing_options =
-      options_pos == std::string::npos
-          ? ""
-          : options_component->substr(options_pos + 1);
-  // Update options in |options_component|.
-  *options_component =
-      component_without_options + kContentURLOptionsStartChar +
-      BuildImageURLOptionsString(image_size, no_silhouette, existing_options);
-  return url_components;
-}
-
 }  // namespace
 
 namespace signin {
@@ -128,20 +66,24 @@ GURL GetAvatarImageURLWithOptions(const GURL& old_url,
       base::SplitString(old_url.path(), kURLPathSeparator,
                         base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
 
-  auto new_components =
-      TryProcessAsContentImageURL(components, image_size, no_silhouette);
-
-  if (new_components.empty()) {
-    new_components =
-        TryProcessAsLegacyImageURL(components, image_size, no_silhouette);
+  if (components.size() < kImageURLPathComponentsCount ||
+      components.size() > kImageURLPathComponentsCountWithOptions ||
+      components.back().empty()) {
+    return old_url;
   }
 
-  if (new_components.empty()) {
-    // URL doesn't match any known patterns, so return unchanged.
-    return old_url;
+  if (components.size() == kImageURLPathComponentsCount) {
+    components.insert(
+        components.begin() + kImageURLPathOptionsComponentPosition,
+        BuildImageURLOptionsString(image_size, no_silhouette, std::string()));
+  } else {
+    DCHECK_EQ(kImageURLPathComponentsCountWithOptions, components.size());
+    std::string options = components.at(kImageURLPathOptionsComponentPosition);
+    components[kImageURLPathOptionsComponentPosition] =
+        BuildImageURLOptionsString(image_size, no_silhouette, options);
   }
 
-  std::string new_path = base::JoinString(new_components, kURLPathSeparator);
+  std::string new_path = base::JoinString(components, kURLPathSeparator);
   GURL::Replacements replacement;
   replacement.SetPathStr(new_path);
   return old_url.ReplaceComponents(replacement);

components/signin/core/browser/avatar_icon_util_unittest.cc

@@ -9,7 +9,7 @@
 
 namespace {
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoInitialSize_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoInitialSize) {
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/photo.jpg");
   GURL result =
       signin::GetAvatarImageURLWithOptions(in, 128, false /* no_silhouette */);
@@ -18,8 +18,7 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoInitialSize_LegacyURL) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest,
-     GetAvatarImageURLWithOptionsSizeAlreadySpecified_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSizeAlreadySpecified) {
   // If there's already a size specified in the URL, it should be changed to the
   // specified size in the resulting URL.
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/s64-c/photo.jpg");
@@ -30,8 +29,7 @@ TEST(AvatarIconUtilTest,
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest,
-     GetAvatarImageURLWithOptionsOtherSizeSpecified_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsOtherSizeSpecified) {
   // If there's already a size specified in the URL, it should be changed to the
   // specified size in the resulting URL.
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/s128-c/photo.jpg");
@@ -42,7 +40,7 @@ TEST(AvatarIconUtilTest,
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSameSize_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSameSize) {
   // If there's already a size specified in the URL, and it's already the
   // requested size, true should be returned and the URL should remain
   // unchanged.
@@ -54,8 +52,7 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSameSize_LegacyURL) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest,
-     GetAvatarImageURLWithOptionsNoFileNameInPath_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoFileNameInPath) {
   // If there is no file path component in the URL path, we should return
   // the input URL.
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/");
@@ -64,7 +61,7 @@ TEST(AvatarIconUtilTest,
   EXPECT_EQ(result, in);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoSilhouette_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoSilhouette) {
   // If there's already a size specified in the URL, it should be changed to the
   // specified size in the resulting URL.
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/photo.jpg");
@@ -75,8 +72,7 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoSilhouette_LegacyURL) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest,
-     GetAvatarImageURLWithOptionsSizeReplaceNoSilhouette_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSizeReplaceNoSilhouette) {
   // If there's already a no_silhouette option specified in the URL, it should
   // be removed if necessary.
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/s64-c-ns/photo.jpg");
@@ -87,8 +83,7 @@ TEST(AvatarIconUtilTest,
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest,
-     GetAvatarImageURLWithOptionsUnknownShouldBePreserved_LegacyURL) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsUnknownShouldBePreserved) {
   // If there are any unknown options encoded in URL,
   // GetAvatarImageURLWithOptions should preserve them.
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/s64-mo-k/photo.jpg");
@@ -99,20 +94,4 @@ TEST(AvatarIconUtilTest,
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithExistingOptions_ContentURL) {
-  GURL in("http://example.com/-A/ABcdefghijk1l2mN3=s256");
-  GURL result =
-      signin::GetAvatarImageURLWithOptions(in, 128, false /* no_silhouette */);
-  GURL expected("http://example.com/-A/ABcdefghijk1l2mN3=s128-c");
-  EXPECT_EQ(result, expected);
-}
-
-TEST(AvatarIconUtilTest, GetAvatarImageURLNoExistingOptions_ContentURL) {
-  GURL in("http://example.com/-A/ABcdefghijk1l2mN3");
-  GURL result =
-      signin::GetAvatarImageURLWithOptions(in, 128, true /* no_silhouette */);
-  GURL expected("http://example.com/-A/ABcdefghijk1l2mN3=s128-c-ns");
-  EXPECT_EQ(result, expected);
-}
-
 }  // namespace