Skip to content

GitLab

T
tangled
Commit 911ea514 authored by Reilly Grant's avatar Reilly Grant Committed by Commit Bot
Browse files
Options

Revert "Add support for FIFE Avatar URL format" 

This reverts commit fa0a6456.

Reason for revert: UserImageManagerTest.SaveUserImageFromProfileImage failing in browser_tests and viz_browser_tests.

Bug: 978578

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: I70e2b5b7940158668ce455286cf3d3d8ac679b10
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 911332
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677157Reviewed-by: default avatarReilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672221}
parent f5373ddf
Hide whitespace changes
Inline Side-by-side
Showing with 32 additions and 111 deletions
components/signin/core/browser/avatar_icon_util.cc
View file @ 911ea514
......@@ -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
View file @ 911ea514
......@@ -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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment