Dynamically compute tab/frame separator color.

chrome/browser/themes/theme_service.cc

Index: chrome/browser/themes/theme_service.cc
diff --git a/chrome/browser/themes/theme_service.cc b/chrome/browser/themes/theme_service.cc
index 208f11fd22b1aa85fe82b1913ddc74daca91a0a2..728684ae322cb3acbbbe444b32bb4de5f34d6a3f 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -77,6 +77,63 @@ const char kDefaultThemeGalleryID[] = "hkacjpbfdknhflllbcmjibkdeoafencn";
 // ExtensionService::GarbageCollectExtensions() does something similar.
 const int kRemoveUnusedThemesStartupDelay = 30;
 
+// Computes the "toolbar top separator" color.  This color is drawn atop the
+// frame to separate it from tabs, the toolbar, and the new tab button, as well
+// as atop background tabs to separate them from other tabs or the toolbar.  We
+// use semitransparent black or white so as to darken or lighten the frame, with
+// the goal of contrasting with both the frame color and the active tab (i.e.
+// toolbar) color.  (It's too difficult to try to find colors that will contrast
+// with both of these as well as the background tab color, and contrasting with
+// the foreground tab is the most important).
+SkColor GetSeparatorColor(SkColor tab_color, SkColor frame_color) {
+  // We use this alpha value for the separator if possible.
+  const SkAlpha kAlpha = 0x40;
+
+  // In most cases, if the tab is lighter than the frame, we darken the
+  // frame; if the tab is darker than the frame, we lighten the frame.
+  // However, if the frame is already very dark or very light, respectively,
+  // this won't contrast sufficiently with the frame color, so we'll need to
+  // reverse when we're lightening and darkening.
+  const double tab_luminance = color_utils::GetRelativeLuminance(tab_color);
+  const double frame_luminance = color_utils::GetRelativeLuminance(frame_color);
+  const bool lighten = tab_luminance < frame_luminance;
+  SkColor separator_color = lighten ? SK_ColorWHITE : SK_ColorBLACK;
+  double separator_luminance = color_utils::GetRelativeLuminance(
+      color_utils::AlphaBlend(separator_color, frame_color, kAlpha));
+  const double kMinContrastRatio = 1.146484375;

[Evan Stade, 2016/03/14 20:30:05]

this value seems very specific... where did it come from?

[Peter Kasting, 2016/03/15 01:30:51]

I was looking for a value that was as close as possible to the default incognito
contrast ratio (since Sebastien mandated that that still darken, but it already
seems unreadable to me so I definitely don't want to go any lower), as well as
something that was exactly representable in hex (although I guess there are no
significant roundoff issues a nonrepresentable value would cause).

[Evan Stade, 2016/03/15 02:06:42]

Can you put a comment to that effect? "This is 0x** / 0x** which is about the
same as the default incognito contrast ratio."

[Peter Kasting, 2016/03/15 11:49:03]

Good idea.  Done.

+  if (color_utils::GetContrastRatio(separator_luminance, frame_luminance) >=
+      kMinContrastRatio)
+    return SkColorSetA(separator_color, kAlpha);
+
+  // We need to reverse whether we're darkening or lightening.  We know the new
+  // separator color will contrast with the frame; check whether it also
+  // contrasts at least as well with the tab.
+  separator_color = color_utils::InvertColor(separator_color);
+  separator_luminance = color_utils::GetRelativeLuminance(
+      color_utils::AlphaBlend(separator_color, frame_color, kAlpha));
+  if (color_utils::GetContrastRatio(separator_luminance, tab_luminance) >=
+      color_utils::GetContrastRatio(separator_luminance, frame_luminance))
+    return SkColorSetA(separator_color, kAlpha);
+
+  // The reversed separator doesn't contrast enough with the tab.  Compute the
+  // resulting luminance from adjusting the tab color, instead of the frame
+  // color, by the separator color.
+  const double target_luminance = color_utils::GetRelativeLuminance(
+      color_utils::AlphaBlend(separator_color, tab_color, kAlpha));
+
+  // Now try to compute an alpha for the separator such that, when blended with
+  // the frame, it results in the above luminance.
+  SkAlpha alpha = 128;
+  for (int delta = lighten ? 64 : -64; delta != 0; delta /= 2) {

[Evan Stade, 2016/03/14 20:30:05]

reusing lighten here seems a little misleading --- aren't we actually darkening
(the tab color) at this point if |lighten| is true?

I think it might also be nice to have only one place where delta is negated
instead of two.

[Peter Kasting, 2016/03/15 01:30:51]

Yes.  I waffled on this.  I eventually decided that it's already somewhat opaque
how a positive or negative delta affects things, so it's not obviously wrong for
"lighten" to be used for "positive delta" as opposed to "negative delta".
I think the way we'd do that would be something like:

  for (int delta = 64; ...) {
    ...
    alpha += ((luminance < target_luminance) == lighten) ? -delta : delta;

This seems even more cryptic than my existing code to me.  I dunno.  I don't
have a good instinct here because honestly I can't come up with a way of writing
this that doesn't require the reader to basically carefully do the math for
their chosen case if they want to know what's actually going on.

[Evan Stade, 2016/03/15 02:06:42]

Yea, I squinted at it for a while but couldn't come up with a better format.
Just mentioning "binary search" might be helpful for faster understanding of
what's going on.

Perhaps:
  lighten = !lighten;
around L111

then here,

for (int delta = 64; delta != 0; delta /= 2) {
  ...
  bool increase_opacity = ((luminance < target_luminance) == lighten);
  alpha += increase_opacity ? delta : -delta;
}

I don't think this is much of an improvement though. Binary search is usually
rather illegible.

[Peter Kasting, 2016/03/15 11:49:03]

Yeah, it doesn't seem much better to me.  I added a comment about using binary
search, which I should have done to begin with.

+    const double luminance = color_utils::GetRelativeLuminance(
+        color_utils::AlphaBlend(separator_color, frame_color, alpha));
+    if (luminance == target_luminance)
+      break;
+    alpha += (luminance < target_luminance) ? -delta : delta;
+  }
+  return SkColorSetA(separator_color, alpha);
+}
+
 SkColor IncreaseLightness(SkColor color, double percent) {
   color_utils::HSL result;
   color_utils::SkColorToHSL(color, &result);
@@ -425,6 +482,19 @@ SkColor ThemeService::GetDefaultColor(int id, bool incognito) const {
       return SkColorSetA(
           GetColor(ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON, incognito),
           0x33);
+    case ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR: {
+      const SkColor tab_color =
+          GetColor(ThemeProperties::COLOR_TOOLBAR, incognito);
+      const SkColor frame_color =
+          GetColor(ThemeProperties::COLOR_FRAME, incognito);
+      const SeparatorColorKey key(tab_color, frame_color);
+      auto i = separator_color_cache_.find(key);
+      if (i != separator_color_cache_.end())
+        return i->second;
+      const SkColor separator_color = GetSeparatorColor(tab_color, frame_color);
+      separator_color_cache_.insert(std::make_pair(key, separator_color));

[Evan Stade, 2016/03/14 20:30:05]

nit: separator_color_cache_[key] = separator_color;

[Peter Kasting, 2016/03/15 01:30:51]

That would work.  This way is more efficient since I already have the key, so it
skips the lookup phase of operator[].  The question is whether that's worth the
readability cost.  I figured this is still clear enough as to not be unreadable,
so why take an efficiency hit I don't have to, but if you feel strongly the
other way I don't, so I could change it.

[Evan Stade, 2016/03/15 02:06:42]

isn't there a lookup phase either way?

[Peter Kasting, 2016/03/15 11:49:03]

You're right.  Because I'm not providing a hint, this is not more efficient. 
Changed.

+      return separator_color;
+    }
     case ThemeProperties::COLOR_BACKGROUND_TAB: {
       // The tints here serve a different purpose than TINT_BACKGROUND_TAB.
       // That tint is used to create background tab images for custom themes by