[Profiles] Prefer colors with right saturation and lightness

This CL further improves the algorithm of color selection to:
 1) Disallow colors with very low saturation (currently the first line
    in the color picker).
 2) Prefer colors with lightness similar to the lightness of the current
    profile (this disallows the very dark colors for the very light
    colors and vice versa).

Bug: 1108295
Change-Id: I3adcc347b84285d5d2659f1972f075e992e0bebf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532565
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Auto-Submit: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827443}

chrome/browser/profiles/profile_attributes_entry.cc

@@ -440,33 +440,44 @@ size_t ProfileAttributesEntry::GetAvatarIconIndex() const {
   return icon_index;
 }
 
-ProfileThemeColors ProfileAttributesEntry::GetProfileThemeColors() const {
-#if defined(OS_ANDROID)
-  // Profile theme colors shouldn't be queried on Android.
-  NOTREACHED();
-  return {SK_ColorRED, SK_ColorRED, SK_ColorRED};
-#else
+base::Optional<ProfileThemeColors>
+ProfileAttributesEntry::GetProfileThemeColorsIfSet() const {
   base::Optional<SkColor> profile_highlight_color =
       GetProfileThemeColor(kProfileHighlightColorKey);
   base::Optional<SkColor> default_avatar_fill_color =
       GetProfileThemeColor(kDefaultAvatarFillColorKey);
   base::Optional<SkColor> default_avatar_stroke_color =
       GetProfileThemeColor(kDefaultAvatarStrokeColorKey);
+
+  DCHECK_EQ(profile_highlight_color.has_value(),
+            default_avatar_stroke_color.has_value());
+  DCHECK_EQ(profile_highlight_color.has_value(),
+            default_avatar_fill_color.has_value());
+
   if (!profile_highlight_color.has_value()) {
-    DCHECK(!default_avatar_fill_color.has_value() &&
-           !default_avatar_stroke_color.has_value());
-    return GetDefaultProfileThemeColors(
-        ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors());
+    return base::nullopt;
   }
 
-  DCHECK(default_avatar_fill_color.has_value() &&
-         default_avatar_stroke_color.has_value());
-
   ProfileThemeColors colors;
   colors.profile_highlight_color = profile_highlight_color.value();
   colors.default_avatar_fill_color = default_avatar_fill_color.value();
   colors.default_avatar_stroke_color = default_avatar_stroke_color.value();
   return colors;
+}
+
+ProfileThemeColors ProfileAttributesEntry::GetProfileThemeColors() const {
+#if defined(OS_ANDROID)
+  // Profile theme colors shouldn't be queried on Android.
+  NOTREACHED();
+  return {SK_ColorRED, SK_ColorRED, SK_ColorRED};
+#else
+  base::Optional<ProfileThemeColors> theme_colors =
+      GetProfileThemeColorsIfSet();
+  if (theme_colors)
+    return *theme_colors;
+
+  return GetDefaultProfileThemeColors(
+      ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors());
 #endif
 }
 

chrome/browser/profiles/profile_attributes_entry.h

@@ -145,6 +145,9 @@ class ProfileAttributesEntry {
   // Returns the colors specified by the profile theme, or default colors if no
   // theme is specified for the profile.
   ProfileThemeColors GetProfileThemeColors() const;
+  // Returns the colors specified by the profile theme, or empty if no theme is
+  // set for the profile.
+  base::Optional<ProfileThemeColors> GetProfileThemeColorsIfSet() const;
   // Returns the metrics bucket this profile should be recorded in.
   // Note: The bucket index is assigned once and remains the same all time. 0 is
   // reserved for the guest profile.

chrome/browser/signin/dice_web_signin_interceptor.cc

@@ -365,7 +365,11 @@ void DiceWebSigninInterceptor::OnExtendedAccountInfoUpdated(
     return;
   }
 
-  SkColor profile_color = GenerateNewProfileColor().color;
+  ProfileAttributesEntry* entry;
+  g_browser_process->profile_manager()
+      ->GetProfileAttributesStorage()
+      .GetProfileAttributesWithPath(profile_->GetPath(), &entry);
+  SkColor profile_color = GenerateNewProfileColor(entry).color;
   Delegate::BubbleParameters bubble_parameters{
       *interception_type, info, GetPrimaryAccountInfo(identity_manager_),
       GetAutogeneratedThemeColors(profile_color).frame_color};

chrome/browser/ui/signin/profile_colors_util.cc

@@ -18,6 +18,14 @@
 
 namespace {
 
+// Minimum saturation for a color to be autoselected (as picking 'colorful'
+// colors is needed to distinguish colors from each other).
+constexpr double kMinimumSaturation = 0.25;
+
+// Maximum diff in lightness of an autoselected color to the color of the
+// current profile (so that the interception UI does not look bad).
+constexpr double kMaximumLightnessDiff = 0.35;
+
 // This is the core definition of how ProfileThemeColors are obtained.
 ProfileThemeColors GetProfileThemeColorsFromHighlightColor(
     SkColor highlight_color) {
@@ -42,6 +50,43 @@ size_t GenerateRandomIndex(size_t size) {
   return static_cast<size_t>(base::RandInt(0, size - 1));
 }
 
+std::vector<int> GetAvailableColorIndices(
+    const std::set<ProfileThemeColors>& used_theme_colors,
+    base::Optional<double> current_color_lightness) {
+  std::vector<int> available_color_indices;
+  for (size_t i = 0; i < base::size(chrome_colors::kGeneratedColorsInfo); ++i) {
+    ProfileThemeColors theme_colors =
+        GetProfileThemeColorsForAutogeneratedColor(
+            chrome_colors::kGeneratedColorsInfo[i].color);
+    if (base::Contains(used_theme_colors, theme_colors))
+      continue;
+
+    const SkColor highlight = theme_colors.profile_highlight_color;
+    if (!IsSaturatedForAutoselection(highlight))
+      continue;
+    if (current_color_lightness &&
+        !IsLightForAutoselection(highlight, *current_color_lightness))
+      continue;
+
+    available_color_indices.push_back(i);
+  }
+  return available_color_indices;
+}
+
+base::Optional<double> ExtractCurrentColorLightness(
+    ProfileAttributesEntry* current_profile) {
+  if (!current_profile)
+    return base::nullopt;
+  base::Optional<ProfileThemeColors> current_colors =
+      current_profile->GetProfileThemeColorsIfSet();
+  if (!current_colors)
+    return base::nullopt;
+
+  color_utils::HSL hsl;
+  color_utils::SkColorToHSL(current_colors->profile_highlight_color, &hsl);
+  return hsl.l;
+}
+
 }  // namespace
 
 bool ProfileThemeColors::operator<(const ProfileThemeColors& other) const {
@@ -104,48 +149,61 @@ SkColor GetAvatarStrokeColor(SkColor avatar_fill_color) {
   return color_utils::HSLToSkColor(color_hsl, SkColorGetA(avatar_fill_color));
 }
 
+bool IsSaturatedForAutoselection(SkColor color) {
+  color_utils::HSL hsl;
+  color_utils::SkColorToHSL(color, &hsl);
+  return hsl.s >= kMinimumSaturation;
+}
+
+bool IsLightForAutoselection(SkColor color, double reference_lightness) {
+  color_utils::HSL hsl;
+  color_utils::SkColorToHSL(color, &hsl);
+  return std::abs(hsl.l - reference_lightness) <= kMaximumLightnessDiff;
+}
+
 chrome_colors::ColorInfo GenerateNewProfileColorWithGenerator(
     ProfileAttributesStorage& storage,
-    base::OnceCallback<size_t(size_t count)> random_generator) {
-  // TODO(crbug.com/1108295):
-  // - Exclude colors from the first line of the picker.
-  // - For sign-in interception, prefer colors from the same line (if not
-  // excluded).
-  // - Both of the previous constraints could be formulated using saturation and
-  //   brightness, instead of assuming a concrete structure of lines.
-  // - Return only a SkColor if the full ColorInfo is not needed.
+    base::OnceCallback<size_t(size_t count)> random_generator,
+    ProfileAttributesEntry* current_profile) {
+  // TODO(crbug.com/1108295): Return only a SkColor if the full ColorInfo is not
+  // needed.
   std::set<ProfileThemeColors> used_theme_colors;
   for (ProfileAttributesEntry* entry : storage.GetAllProfilesAttributes()) {
-    used_theme_colors.insert(entry->GetProfileThemeColors());
+    base::Optional<ProfileThemeColors> current_colors =
+        entry->GetProfileThemeColorsIfSet();
+    if (current_colors)
+      used_theme_colors.insert(*current_colors);
   }
 
-  // Collect indices of profile colors that are not in `used_theme_colors`.
-  std::vector<int> available_color_indices;
-  for (size_t i = 0; i < base::size(chrome_colors::kGeneratedColorsInfo); ++i) {
-    ProfileThemeColors theme_colors =
-        GetProfileThemeColorsForAutogeneratedColor(
-            chrome_colors::kGeneratedColorsInfo[i].color);
-    if (!base::Contains(used_theme_colors, theme_colors))
-      available_color_indices.push_back(i);
-  }
+  base::Optional<double> current_color_lightness =
+      ExtractCurrentColorLightness(current_profile);
 
-  size_t index;
+  // Collect indices of profile colors that match all the filters.
+  std::vector<int> available_color_indices =
+      GetAvailableColorIndices(used_theme_colors, current_color_lightness);
+  // Relax the constraints until some colors become available.
   if (available_color_indices.empty()) {
-    // No suitable color, fallback to random color.
-    size_t size = base::size(chrome_colors::kGeneratedColorsInfo);
-    index = std::move(random_generator).Run(size);
-    DCHECK_LT(index, size);
-  } else {
-    size_t size = available_color_indices.size();
-    size_t available_index = std::move(random_generator).Run(size);
-    DCHECK_LT(available_index, size);
-    index = available_color_indices[available_index];
+    available_color_indices =
+        GetAvailableColorIndices(used_theme_colors, base::nullopt);
   }
+  if (available_color_indices.empty()) {
+    // If needed, we could allow unsaturated colors (shades of grey) before
+    // allowing a duplicate color.
+    available_color_indices =
+        GetAvailableColorIndices(std::set<ProfileThemeColors>(), base::nullopt);
+  }
+  DCHECK(!available_color_indices.empty());
+
+  size_t size = available_color_indices.size();
+  size_t available_index = std::move(random_generator).Run(size);
+  DCHECK_LT(available_index, size);
+  size_t index = available_color_indices[available_index];
   return chrome_colors::kGeneratedColorsInfo[index];
 }
 
-chrome_colors::ColorInfo GenerateNewProfileColor() {
+chrome_colors::ColorInfo GenerateNewProfileColor(
+    ProfileAttributesEntry* current_profile) {
   return GenerateNewProfileColorWithGenerator(
       g_browser_process->profile_manager()->GetProfileAttributesStorage(),
-      base::BindOnce(&GenerateRandomIndex));
+      base::BindOnce(&GenerateRandomIndex), current_profile);
 }

chrome/browser/ui/signin/profile_colors_util.h

@@ -13,6 +13,7 @@ struct ColorInfo;
 }
 
 class CustomThemeSupplier;
+class ProfileAttributesEntry;
 class ProfileAttributesStorage;
 
 struct ProfileThemeColors {
@@ -45,17 +46,27 @@ SkColor GetProfileForegroundIconColor(SkColor profile_highlight_color);
 // Returns the color that should be used to generate the default avatar icon.
 SkColor GetAvatarStrokeColor(SkColor avatar_fill_color);
 
+// Filters used for generating colors for a new profile. Exposed for tests.
+bool IsSaturatedForAutoselection(SkColor color);
+bool IsLightForAutoselection(SkColor color, double reference_lightness);
+
 // Returns a new color for a profile, based on the colors of the existing
 // profiles in `storage`. `random_generator` is called to provide randomness and
 // must return a value smaller than provided `count`. This implementation
 // function is mainly exposed for easier mocking in tests. In production code,
-// GenerateNewProfileColor() should be sufficient.
+// GenerateNewProfileColor() should be sufficient. `current_profile` should be
+// specified if a new profile is created within an existing profile (such as for
+// sign-in interception) and thus the two colors should somehow match.
 chrome_colors::ColorInfo GenerateNewProfileColorWithGenerator(
     ProfileAttributesStorage& storage,
-    base::OnceCallback<size_t(size_t count)> random_generator);
+    base::OnceCallback<size_t(size_t count)> random_generator,
+    ProfileAttributesEntry* current_profile = nullptr);
 
 // Returns a new random color for a profile, based on the colors of the existing
-// profiles.
-chrome_colors::ColorInfo GenerateNewProfileColor();
+// profiles. `current_profile` should be specified if a new profile is created
+// within an existing profile (such as for sign-in interception) and thus the
+// two colors should somehow match.
+chrome_colors::ColorInfo GenerateNewProfileColor(
+    ProfileAttributesEntry* current_profile = nullptr);
 
 #endif  // CHROME_BROWSER_UI_SIGNIN_PROFILE_COLORS_UTIL_H_