Dynamically compute tab/frame separator color.

chrome/browser/themes/theme_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/themes/theme_service.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 
@@ skipping 59 matching lines @@
 // realize we've installed the default theme, we already have an extension
 // unpacked on the filesystem.)
 const char kDefaultThemeGalleryID[] = "hkacjpbfdknhflllbcmjibkdeoafencn";
 
 // Wait this many seconds after startup to garbage collect unused themes.
 // Removing unused themes is done after a delay because there is no
 // reason to do it at startup.
 // 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);
   result.l += (1 - result.l) * percent;
   return color_utils::HSLToSkColor(result, SkColorGetA(color));
 }
 
 // Writes the theme pack to disk on a separate thread.
 void WritePackToDiskCallback(BrowserThemePack* pack,
                              const base::FilePath& path) {
@@ skipping 328 matching lines @@
   switch (id) {
     case ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON:
       return color_utils::HSLShift(
           gfx::kChromeIconGrey,
           GetTint(ThemeProperties::TINT_BUTTONS, incognito));
     case ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON_INACTIVE:
       // The active color is overridden in Gtk2UI.
       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
       // lightening the frame images.  The tints here create solid colors for
       // background tabs by darkening the foreground tab (toolbar) color.
       const color_utils::HSL kTint = {-1, -1, 0.4296875};
       const color_utils::HSL kTintIncognito = {-1, -1, 0.34375};
       return color_utils::HSLShift(
           GetColor(ThemeProperties::COLOR_TOOLBAR, incognito),
           incognito ? kTintIncognito : kTint);
@@ skipping 341 matching lines @@
 
 #if defined(ENABLE_SUPERVISED_USERS)
 bool ThemeService::IsSupervisedUser() const {
   return profile_->IsSupervised();
 }
 
 void ThemeService::SetSupervisedUserTheme() {
   SetCustomDefaultTheme(new SupervisedUserTheme);
 }
 #endif