Reland "Add support for FIFE Avatar URL format"

This is a reland of fa0a6456

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}

Bug: 911332, 978578
Change-Id: I86414178c22600da40c6175b2d12ee8e311416a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1678497Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Thomas Tangl <tangltom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672510}

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

@@ -138,7 +138,7 @@ class UserImageManagerTest : public LoginManagerTest,
  public:
   std::unique_ptr<net::test_server::BasicHttpResponse> HandleRequest(
       const net::test_server::HttpRequest& request) {
-    if (request.relative_url != "/avatar.jpg")
+    if (request.relative_url.find("/avatar.jpg") == std::string::npos)
       return nullptr;
 
     // Check whether the token string is the same.

components/signin/core/browser/avatar_icon_util.cc

@@ -5,6 +5,7 @@
 #include "components/signin/core/browser/avatar_icon_util.h"
 
 #include <string>
+#include <vector>
 
 #include "base/stl_util.h"
 #include "base/strings/string_split.h"
@@ -16,20 +17,25 @@
 namespace {
 
 // Separator of URL path components.
-const char kURLPathSeparator[] = "/";
+constexpr char kURLPathSeparator[] = "/";
 
-// Constants describing image URL format.
+// Constants describing legacy image URL format.
 // See https://crbug.com/733306#c3 for details.
-const size_t kImageURLPathComponentsCount = 6;
-const size_t kImageURLPathComponentsCountWithOptions = 7;
-const size_t kImageURLPathOptionsComponentPosition = 5;
+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 = '=';
 // Various options that can be embedded in image URL.
-const char kImageURLOptionSeparator[] = "-";
-const char kImageURLOptionSizePattern[] = R"(s\d+)";
-const char kImageURLOptionSizeFormat[] = "s%d";
-const char kImageURLOptionSquareCrop[] = "c";
+constexpr char kImageURLOptionSeparator[] = "-";
+constexpr char kImageURLOptionSizePattern[] = R"(s\d+)";
+constexpr char kImageURLOptionSizeFormat[] = "s%d";
+constexpr char kImageURLOptionSquareCrop[] = "c";
 // Option to disable default avatar if user doesn't have a custom one.
-const char kImageURLOptionNoSilhouette[] = "ns";
+constexpr char kImageURLOptionNoSilhouette[] = "ns";
 
 std::string BuildImageURLOptionsString(int image_size,
                                        bool no_silhouette,
@@ -53,6 +59,62 @@ 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 {
@@ -66,24 +128,20 @@ GURL GetAvatarImageURLWithOptions(const GURL& old_url,
       base::SplitString(old_url.path(), kURLPathSeparator,
                         base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
 
-  if (components.size() < kImageURLPathComponentsCount ||
-      components.size() > kImageURLPathComponentsCountWithOptions ||
-      components.back().empty()) {
-    return old_url;
+  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.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);
+  if (new_components.empty()) {
+    // URL doesn't match any known patterns, so return unchanged.
+    return old_url;
   }
 
-  std::string new_path = base::JoinString(components, kURLPathSeparator);
+  std::string new_path = base::JoinString(new_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) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoInitialSize_LegacyURL) {
   GURL in("http://example.com/-A/AAAAAAAAAAI/AAAAAAAAACQ/B/photo.jpg");
   GURL result =
       signin::GetAvatarImageURLWithOptions(in, 128, false /* no_silhouette */);
@@ -18,7 +18,8 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoInitialSize) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSizeAlreadySpecified) {
+TEST(AvatarIconUtilTest,
+     GetAvatarImageURLWithOptionsSizeAlreadySpecified_LegacyURL) {
   // 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");
@@ -29,7 +30,8 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSizeAlreadySpecified) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsOtherSizeSpecified) {
+TEST(AvatarIconUtilTest,
+     GetAvatarImageURLWithOptionsOtherSizeSpecified_LegacyURL) {
   // 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");
@@ -40,7 +42,7 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsOtherSizeSpecified) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSameSize) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSameSize_LegacyURL) {
   // 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.
@@ -52,7 +54,8 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSameSize) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoFileNameInPath) {
+TEST(AvatarIconUtilTest,
+     GetAvatarImageURLWithOptionsNoFileNameInPath_LegacyURL) {
   // 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/");
@@ -61,7 +64,7 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoFileNameInPath) {
   EXPECT_EQ(result, in);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoSilhouette) {
+TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoSilhouette_LegacyURL) {
   // 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");
@@ -72,7 +75,8 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsNoSilhouette) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSizeReplaceNoSilhouette) {
+TEST(AvatarIconUtilTest,
+     GetAvatarImageURLWithOptionsSizeReplaceNoSilhouette_LegacyURL) {
   // 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");
@@ -83,7 +87,8 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsSizeReplaceNoSilhouette) {
   EXPECT_EQ(result, expected);
 }
 
-TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsUnknownShouldBePreserved) {
+TEST(AvatarIconUtilTest,
+     GetAvatarImageURLWithOptionsUnknownShouldBePreserved_LegacyURL) {
   // 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");
@@ -94,4 +99,20 @@ TEST(AvatarIconUtilTest, GetAvatarImageURLWithOptionsUnknownShouldBePreserved) {
   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