Dynamically compute tab/frame separator color.

Dynamically compute tab/frame separator color.

For reference, this color is overlaid atop the frame/toolbar when outlining tabs
and for outlines/shadows when drawing the new tab button.  It's either white or
black with a varying (but usually 0x40) alpha value.

Computing this is surprisingly complicated, beause we're trying to contrast with
two different colors simultaneously (tab and frame), and we also want to set our
magic numbers so as to achieve the colors from the design specs in the default
theme.

After quite a bit of thought, I elected to use a mechanism that defaults to
moving the frame away from the tab luminance; that is, if the tab is "brighter"
than the frame, we make the separator darken the frame, and if the tab is
darker, we try to lighten the frame.  If the frame is already so dark or light
that the result has too low of contrast with the frame color, we reverse
direction.  ("Too low" here is a lot lower than I'd like, but any higher and
we'd end up using light separators for the default theme in incognito mode,
which I think looks good but Sebastien dislikes.)  In the case where we reversed
direction, we have to worry that the result of all this won't contrast enough
with the tab; in that case, we push up the alpha value so the result contrasts
enough with the tab as well.  This last computation is the most expensive
because of how I chose to make it behave, but I think the behavior I selected
(too complicated to explain here, see code/comments for details) will feel more
consistent than any of the simpler methods I considered.

Because computing the separator color can be expensive (every call to
GetRelativeLuminance() can involve floating-point exponentiation among other
things, and in the worst case the color computation computes the desired color
via a 7-iteration loop), I elected to cache the computed value in a map.  This
might be a case of premature optimization, but in debugging it looked like this
color could be requested frequently, and I didn't want to risk performance
problems.

Even this choice presented options.  I used a simple map that I never prune
entries from; at worst, we'll add up to 2 entries per distinct theme the user
switches to while running, which didn't seem too bad.  I considered instead
using base::MRUCache, which would let me cap the size.  I also considered just
keeping a couple member structs containing the relevant information for the
normal and incognito color schemes, but this generally seemed like it ended up
as more code than the other routes.  None of these options seems wildly better
or worse than the others; I'm willing to change course in the face of violent
opinion :)

Finally, because the alpha value of the separator can now vary, I converted the
code in tab_strip.cc that set it to fixed values to instead use scaling
multipliers.  These will compute the same values as before when the separator
has its default (0x40) alpha, but in the case where we've computed some larger
alpha, scaling proportionally seemed like the best thing to do.  I used a
saturated_cast in one place where I wasn't sure the result was guaranteed to
stay <= 255.

BUG=585470
TEST=See bug comment 0

Committed: https://crrev.com/3eb05e63d4dd60dadaa079729f122011b0febdac
Cr-Commit-Position: refs/heads/master@{#381617}

[Peter Kasting, 2016-03-10 10:27:51]

pkasting@chromium.org changed reviewers:
+ estade@chromium.org

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

maybe clear the color cache when the theme changes if you're worried about
memory usage?

The separator color logic is complicated enough to warrant unit tests, I think,
which will:
a) provide documentation by example
b) verify behavior
c) prevent regressions (or at least force the next person to carefully consider
the impact of their changes)

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:103: const double kMinContrastRatio =
1.146484375;
this value seems very specific... where did it come from?

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:127: for (int delta = lighten ? 64 : -64;
delta != 0; delta /= 2) {
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.

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:495:
separator_color_cache_.insert(std::make_pair(key, separator_color));
nit: separator_color_cache_[key] = separator_color;

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

On 2016/03/14 20:30:05, Evan Stade wrote:
> maybe clear the color cache when the theme changes if you're worried about
> memory usage?

I thought about it.  I didn't do it in part because some windows (e.g. popups)
always remain unthemed and so we'd want the default theme values + the new theme
values in every case, and partly because if users change themes they're probably
somewhat likely to just change back (checking out a new theme to see if they
like it).  I didn't figure memory use would be a concern in practice because
people would have to change to hundreds of themes just to have this take up a
few kilobytes.  I don't feel strongly.

> The separator color logic is complicated enough to warrant unit tests, I
think,

Yes, I agree.  Will add.  In the meantime, here are some replies to some of your
other questions.

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:103: const double kMinContrastRatio =
1.146484375;
On 2016/03/14 20:30:05, Evan Stade wrote:
> this value seems very specific... where did it come from?

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).

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:127: for (int delta = lighten ? 64 : -64;
delta != 0; delta /= 2) {
On 2016/03/14 20:30:05, Evan Stade wrote:
> reusing lighten here seems a little misleading --- aren't we actually
darkening
> (the tab color) at this point if |lighten| is true?

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 it might also be nice to have only one place where delta is negated
> instead of two.

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.

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:495:
separator_color_cache_.insert(std::make_pair(key, separator_color));
On 2016/03/14 20:30:05, Evan Stade wrote:
> nit: separator_color_cache_[key] = separator_color;

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:34]

On 2016/03/15 01:30:51, Peter Kasting wrote:
> On 2016/03/14 20:30:05, Evan Stade wrote:
> > maybe clear the color cache when the theme changes if you're worried about
> > memory usage?
> 
> I thought about it.  I didn't do it in part because some windows (e.g. popups)
> always remain unthemed and so we'd want the default theme values + the new
theme
> values in every case, and partly because if users change themes they're
probably
> somewhat likely to just change back (checking out a new theme to see if they
> like it).  I didn't figure memory use would be a concern in practice because
> people would have to change to hundreds of themes just to have this take up a
> few kilobytes.  I don't feel strongly.

I don't feel strongly either. Normal users won't run into problems, but perhaps
there's some bot out there testing all the themes it can find or otherwise doing
something crazy. It seems pretty easy and harmless to clear this cache on theme
switches (I doubt anyone will notice the slowdown caused by the recomputation of
the popup windows).

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

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:103: const double kMinContrastRatio =
1.146484375;
On 2016/03/15 01:30:51, Peter Kasting wrote:
> On 2016/03/14 20:30:05, Evan Stade wrote:
> > this value seems very specific... where did it come from?
> 
> 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).

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

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:127: for (int delta = lighten ? 64 : -64;
delta != 0; delta /= 2) {
On 2016/03/15 01:30:51, Peter Kasting wrote:
> On 2016/03/14 20:30:05, Evan Stade wrote:
> > reusing lighten here seems a little misleading --- aren't we actually
> darkening
> > (the tab color) at this point if |lighten| is true?
> 
> 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 it might also be nice to have only one place where delta is negated
> > instead of two.
> 
> 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.

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.

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:495:
separator_color_cache_.insert(std::make_pair(key, separator_color));
On 2016/03/15 01:30:51, Peter Kasting wrote:
> On 2016/03/14 20:30:05, Evan Stade wrote:
> > nit: separator_color_cache_[key] = separator_color;
> 
> 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.

isn't there a lookup phase either way?

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

I also made a few other tweaks:
(1) Fixed an issue where the opaque frame code used the themed color where it
should have used the default color.  I had to restore the "default separator
color" code for this.
(2) Slightly simplified and commented the tint colors for COLOR_BACKGROUND_TAB,
which doesn't really need to be in this CL but is simple and I thought of after
doing something similar to the contrast ratio constant (below).

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:103: const double kMinContrastRatio =
1.146484375;
On 2016/03/15 02:06:42, Evan Stade wrote:
> On 2016/03/15 01:30:51, Peter Kasting wrote:
> > On 2016/03/14 20:30:05, Evan Stade wrote:
> > > this value seems very specific... where did it come from?
> > 
> > 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).
> 
> Can you put a comment to that effect? "This is 0x** / 0x** which is about the
> same as the default incognito contrast ratio."

Good idea.  Done.

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:127: for (int delta = lighten ? 64 : -64;
delta != 0; delta /= 2) {
On 2016/03/15 02:06:42, Evan Stade wrote:
> On 2016/03/15 01:30:51, Peter Kasting wrote:
> > On 2016/03/14 20:30:05, Evan Stade wrote:
> > > reusing lighten here seems a little misleading --- aren't we actually
> > darkening
> > > (the tab color) at this point if |lighten| is true?
> > 
> > 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 it might also be nice to have only one place where delta is
negated
> > > instead of two.
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/1785613004/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:495:
separator_color_cache_.insert(std::make_pair(key, separator_color));
On 2016/03/15 02:06:42, Evan Stade wrote:
> On 2016/03/15 01:30:51, Peter Kasting wrote:
> > On 2016/03/14 20:30:05, Evan Stade wrote:
> > > nit: separator_color_cache_[key] = separator_color;
> > 
> > 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.
> 
> isn't there a lookup phase either way?

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

[Evan Stade, 2016-03-15 19:05:06]

lgtm

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:80: 
intentional?

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:222: 
nit: is this extra line intentional?

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:225: // well as atop background tabs to
separate them from other tabs or the
hmm, so this color is drawn on top of the background tabs and on top of the
frame where it intersects with the background tabs? I guess it's not clear to me
which thing this separator is actually drawn on top of.

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_unittest.cc:415: SkColor separator_color =
GetSeparatorColor(tab_color, frame_color);
can you add a check to verify this matches what you get from
ThemeService::GetColor(COLOR_TOOLBAR_TOP_SEPARATOR)? It's annoying we have to
keep that in sync with ThemeProperties::GetDefaultColor but we can at least add
a test for it.

[Peter Kasting, 2016-03-16 03:12:59]

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:80: 
On 2016/03/15 19:05:06, Evan Stade wrote:
> intentional?

No.

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:222: 
On 2016/03/15 19:05:06, Evan Stade wrote:
> nit: is this extra line intentional?

No.

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:225: // well as atop background tabs to
separate them from other tabs or the
On 2016/03/15 19:05:06, Evan Stade wrote:
> hmm, so this color is drawn on top of the background tabs and on top of the
> frame where it intersects with the background tabs? I guess it's not clear to
me
> which thing this separator is actually drawn on top of.

It's drawn on both.  It's drawn on the frame around the tabs, and it's drawn on
the background tabs to separate tabs from the toolbar and each other.

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_unittest.cc:415: SkColor separator_color =
GetSeparatorColor(tab_color, frame_color);
On 2016/03/15 19:05:06, Evan Stade wrote:
> can you add a check to verify this matches what you get from
> ThemeService::GetColor(COLOR_TOOLBAR_TOP_SEPARATOR)? It's annoying we have to
> keep that in sync with ThemeProperties::GetDefaultColor but we can at least
add
> a test for it.

Sure.

[Peter Kasting, 2016-03-16 06:41:53]

pkasting@chromium.org changed reviewers:
+ shrike@chromium.org

[Peter Kasting, 2016-03-16 06:41:54]

+shrike for Mac failure.

Right now, Mac uses a very different incognito frame color for MD than
Win/CrOS/Linux.  Is this intentional?  Should I just comment out part or all of
the unittest here for OS_MACOSX?  Or should Mac change to use the color the
other platforms use?

[shrike, 2016-03-16 16:11:56]

On 2016/03/16 06:41:54, Peter Kasting wrote:
> +shrike for Mac failure.
> 
> Right now, Mac uses a very different incognito frame color for MD than
> Win/CrOS/Linux.  Is this intentional?  Should I just comment out part or all
of
> the unittest here for OS_MACOSX?  Or should Mac change to use the color the
> other platforms use?

Hello pkasting@,

Can you describe again where the separator color is used? It sounds like it's
basically the stroke color for the tabs and new tab button?

[Peter Kasting, 2016-03-16 21:37:29]

On 2016/03/16 16:11:56, shrike wrote:
> On 2016/03/16 06:41:54, Peter Kasting wrote:
> > +shrike for Mac failure.
> > 
> > Right now, Mac uses a very different incognito frame color for MD than
> > Win/CrOS/Linux.  Is this intentional?  Should I just comment out part or all
> of
> > the unittest here for OS_MACOSX?  Or should Mac change to use the color the
> > other platforms use?
> 
> Hello pkasting@,
> 
> Can you describe again where the separator color is used? It sounds like it's
> basically the stroke color for the tabs and new tab button?

See codesearch for COLOR_TOOLBAR_TOP_SEPARATOR, but basically:

* The stroke color around tabs and above the toolbar
* The stroke color around the new tab button
* The base color for computing the shadows inside and outside the new tab button

...and only in Material Design mode.

But this isn't so much a question of whether and where Mac would use this color;
instead, this is about the choice of frame colors for Mac vs. other platforms. 
Right now all platforms agree in MD about the frame color for normal windows,
and all platforms but Mac agree about the frame color for incognito windows. 
This makes the different Mac MD incognito frame color look like a possible
mistake.  I'm hoping we can just change that to align with the other platforms,
but if not, I need to disable part of my test in the case of Mac.

[shrike, 2016-03-17 00:05:24]

On 2016/03/16 21:37:29, Peter Kasting wrote:
> On 2016/03/16 16:11:56, shrike wrote:
> > On 2016/03/16 06:41:54, Peter Kasting wrote:
> > > +shrike for Mac failure.
> > > 
> > > Right now, Mac uses a very different incognito frame color for MD than
> > > Win/CrOS/Linux.  Is this intentional?  Should I just comment out part or
all
> > of
> > > the unittest here for OS_MACOSX?  Or should Mac change to use the color
the
> > > other platforms use?
> > 
> > Hello pkasting@,
> > 
> > Can you describe again where the separator color is used? It sounds like
it's
> > basically the stroke color for the tabs and new tab button?
> 
> See codesearch for COLOR_TOOLBAR_TOP_SEPARATOR, but basically:
> 
> * The stroke color around tabs and above the toolbar
> * The stroke color around the new tab button
> * The base color for computing the shadows inside and outside the new tab
button
> 
> ...and only in Material Design mode.
> 
> But this isn't so much a question of whether and where Mac would use this
color;
> instead, this is about the choice of frame colors for Mac vs. other platforms.

> Right now all platforms agree in MD about the frame color for normal windows,
> and all platforms but Mac agree about the frame color for incognito windows. 
> This makes the different Mac MD incognito frame color look like a possible
> mistake.  I'm hoping we can just change that to align with the other
platforms,
> but if not, I need to disable part of my test in the case of Mac.

Thank you for the explanations. I looked into this - on the Mac side this color
is not the same as on other platforms because the Mac spec is a little different
(for starters, on the Mac the area behind the tabs is generally translucent). So
this color should not be changed to match the other platforms.

[Peter Kasting, 2016-03-17 00:12:28]

On 2016/03/17 00:05:24, shrike wrote:
> Thank you for the explanations. I looked into this - on the Mac side this
color
> is not the same as on other platforms because the Mac spec is a little
different
> (for starters, on the Mac the area behind the tabs is generally translucent).
So
> this color should not be changed to match the other platforms.

It's strange, then, that the Mac non-incognito frame color DOES match other
platforms and is not translucent.  You have a translucent incognito frame and an
opaque normal frame?  That seems like it can't be right.

Plus there's the normal question of "now that we've gone far into the
implementation of other platforms, is it still the case that Mac specs which
likely largely predate that implementation are correct, or should they be
updated to track other platforms more closely", but you're aware of that issue
in general already and I assume are taking whatever steps are needed to check on
it in all cases.

I'm going to exclude this test on Mac for now.  If things change such that the
Mac colors come into line with other platforms please re-enable it.

[Peter Kasting, 2016-03-17 00:15:54]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-03-17 00:15:54]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1785613004/#ps60001
(title: "Exclude test on Mac")

[commit-bot: I haz the power, 2016-03-17 00:16:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785613004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785613004/60001

[shrike, 2016-03-17 00:25:28]

On 2016/03/17 00:12:28, Peter Kasting wrote:
> On 2016/03/17 00:05:24, shrike wrote:
> > Thank you for the explanations. I looked into this - on the Mac side this
> color
> > is not the same as on other platforms because the Mac spec is a little
> different
> > (for starters, on the Mac the area behind the tabs is generally
translucent).
> So
> > this color should not be changed to match the other platforms.
> 
> It's strange, then, that the Mac non-incognito frame color DOES match other
> platforms and is not translucent.  You have a translucent incognito frame and
an
> opaque normal frame?  That seems like it can't be right.
> 
> Plus there's the normal question of "now that we've gone far into the
> implementation of other platforms, is it still the case that Mac specs which
> likely largely predate that implementation are correct, or should they be
> updated to track other platforms more closely", but you're aware of that issue
> in general already and I assume are taking whatever steps are needed to check
on
> it in all cases.
> 
> I'm going to exclude this test on Mac for now.  If things change such that the
> Mac colors come into line with other platforms please re-enable it.

Will do. The color is slightly different from what's in the Mac spec - I'm not
sure why that is, but I have a bug to fix that and I will check in with
sgabriel@ when I do to make sure I'm using up-to-date values.

[commit-bot: I haz the power, 2016-03-17 01:02:25]

Description was changed from

==========
Dynamically compute tab/frame separator color.

For reference, this color is overlaid atop the frame/toolbar when outlining tabs
and for outlines/shadows when drawing the new tab button.  It's either white or
black with a varying (but usually 0x40) alpha value.

Computing this is surprisingly complicated, beause we're trying to contrast with
two different colors simultaneously (tab and frame), and we also want to set our
magic numbers so as to achieve the colors from the design specs in the default
theme.

After quite a bit of thought, I elected to use a mechanism that defaults to
moving the frame away from the tab luminance; that is, if the tab is "brighter"
than the frame, we make the separator darken the frame, and if the tab is
darker, we try to lighten the frame.  If the frame is already so dark or light
that the result has too low of contrast with the frame color, we reverse
direction.  ("Too low" here is a lot lower than I'd like, but any higher and
we'd end up using light separators for the default theme in incognito mode,
which I think looks good but Sebastien dislikes.)  In the case where we reversed
direction, we have to worry that the result of all this won't contrast enough
with the tab; in that case, we push up the alpha value so the result contrasts
enough with the tab as well.  This last computation is the most expensive
because of how I chose to make it behave, but I think the behavior I selected
(too complicated to explain here, see code/comments for details) will feel more
consistent than any of the simpler methods I considered.

Because computing the separator color can be expensive (every call to
GetRelativeLuminance() can involve floating-point exponentiation among other
things, and in the worst case the color computation computes the desired color
via a 7-iteration loop), I elected to cache the computed value in a map.  This
might be a case of premature optimization, but in debugging it looked like this
color could be requested frequently, and I didn't want to risk performance
problems.

Even this choice presented options.  I used a simple map that I never prune
entries from; at worst, we'll add up to 2 entries per distinct theme the user
switches to while running, which didn't seem too bad.  I considered instead
using base::MRUCache, which would let me cap the size.  I also considered just
keeping a couple member structs containing the relevant information for the
normal and incognito color schemes, but this generally seemed like it ended up
as more code than the other routes.  None of these options seems wildly better
or worse than the others; I'm willing to change course in the face of violent
opinion :)

Finally, because the alpha value of the separator can now vary, I converted the
code in tab_strip.cc that set it to fixed values to instead use scaling
multipliers.  These will compute the same values as before when the separator
has its default (0x40) alpha, but in the case where we've computed some larger
alpha, scaling proportionally seemed like the best thing to do.  I used a
saturated_cast in one place where I wasn't sure the result was guaranteed to
stay <= 255.

BUG=585470
TEST=See bug comment 0
==========

to

==========
Dynamically compute tab/frame separator color.

For reference, this color is overlaid atop the frame/toolbar when outlining tabs
and for outlines/shadows when drawing the new tab button.  It's either white or
black with a varying (but usually 0x40) alpha value.

Computing this is surprisingly complicated, beause we're trying to contrast with
two different colors simultaneously (tab and frame), and we also want to set our
magic numbers so as to achieve the colors from the design specs in the default
theme.

After quite a bit of thought, I elected to use a mechanism that defaults to
moving the frame away from the tab luminance; that is, if the tab is "brighter"
than the frame, we make the separator darken the frame, and if the tab is
darker, we try to lighten the frame.  If the frame is already so dark or light
that the result has too low of contrast with the frame color, we reverse
direction.  ("Too low" here is a lot lower than I'd like, but any higher and
we'd end up using light separators for the default theme in incognito mode,
which I think looks good but Sebastien dislikes.)  In the case where we reversed
direction, we have to worry that the result of all this won't contrast enough
with the tab; in that case, we push up the alpha value so the result contrasts
enough with the tab as well.  This last computation is the most expensive
because of how I chose to make it behave, but I think the behavior I selected
(too complicated to explain here, see code/comments for details) will feel more
consistent than any of the simpler methods I considered.

Because computing the separator color can be expensive (every call to
GetRelativeLuminance() can involve floating-point exponentiation among other
things, and in the worst case the color computation computes the desired color
via a 7-iteration loop), I elected to cache the computed value in a map.  This
might be a case of premature optimization, but in debugging it looked like this
color could be requested frequently, and I didn't want to risk performance
problems.

Even this choice presented options.  I used a simple map that I never prune
entries from; at worst, we'll add up to 2 entries per distinct theme the user
switches to while running, which didn't seem too bad.  I considered instead
using base::MRUCache, which would let me cap the size.  I also considered just
keeping a couple member structs containing the relevant information for the
normal and incognito color schemes, but this generally seemed like it ended up
as more code than the other routes.  None of these options seems wildly better
or worse than the others; I'm willing to change course in the face of violent
opinion :)

Finally, because the alpha value of the separator can now vary, I converted the
code in tab_strip.cc that set it to fixed values to instead use scaling
multipliers.  These will compute the same values as before when the separator
has its default (0x40) alpha, but in the case where we've computed some larger
alpha, scaling proportionally seemed like the best thing to do.  I used a
saturated_cast in one place where I wasn't sure the result was guaranteed to
stay <= 255.

BUG=585470
TEST=See bug comment 0

Committed: https://crrev.com/3eb05e63d4dd60dadaa079729f122011b0febdac
Cr-Commit-Position: refs/heads/master@{#381617}
==========

[commit-bot: I haz the power, 2016-03-17 01:02:26]

Patchset 4 (id:??) landed as
https://crrev.com/3eb05e63d4dd60dadaa079729f122011b0febdac
Cr-Commit-Position: refs/heads/master@{#381617}

[Evan Stade, 2016-03-17 01:08:00]

sorry --- I forgot to press send on this comment

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:433:
GetColor(ThemeProperties::COLOR_FRAME, incognito);
note that https://codereview.chromium.org/1798323002 changed this value ---
hopefully that's ok

[Peter Kasting, 2016-03-17 03:31:02]

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1785613004/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:433:
GetColor(ThemeProperties::COLOR_FRAME, incognito);
On 2016/03/17 01:07:59, Evan Stade wrote:
> note that https://codereview.chromium.org/1798323002 changed this value ---
> hopefully that's ok

Yeah, it's fine.