Have Chrome draw top window border when using custom titlebar.

Have Chrome draw top window border when using custom titlebar.

This is the only transparent part of the window, so after this the
window can be made opaque. The appearance should be similar to other
native apps, and in particular to edge and other UWP apps that do the
same thing.

BUG=543904
TEST=--windows10-custom-titlebar with background slideshow and accent color taken from background.

Committed: https://crrev.com/13552791a9ce4282df4ec658f07bf59a45ffb385
Cr-Commit-Position: refs/heads/master@{#424931}

[jbauman, 2016-09-30 19:51:40]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 19:52:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 20:45:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 20:45:05]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-09-30 22:29:26]

jbauman@chromium.org changed reviewers:
+ bsep@chromium.org, pkasting@chromium.org

[Bret, 2016-09-30 23:33:36]

Wait, I'm confused. Why do we want this? I know Edge's inactive top border is
opaque but I think that looks bad; we don't have to do everything they do. The
current behavior matches the border of non-UWP apps. Also if the goal is to make
the window totally opaque, the other three sides of the window also have
partially transparent borders+fully transparent window handles. But all that is
drawn by Windows so I'm not sure why we care.

[jbauman, 2016-09-30 23:38:11]

On 2016/09/30 23:33:36, Bret Sepulveda wrote:
> Wait, I'm confused. Why do we want this? I know Edge's inactive top border is
> opaque but I think that looks bad; we don't have to do everything they do. The
> current behavior matches the border of non-UWP apps. Also if the goal is to
make
> the window totally opaque, the other three sides of the window also have
> partially transparent borders+fully transparent window handles. But all that
is
> drawn by Windows so I'm not sure why we care.

We want to be able to make the surface opaque, so DWM won't have to draw the
contents underneath it and blend with that. This reduces power consumption when
playing a maximized 1080p60 video from about 5.5W to 5.1W for me. The other
sides of the window are inset so that the client area doesn't cover the border,
but on the top we can't do that because having a non-zero inset causes
everything to break.

[Peter Kasting, 2016-10-01 05:00:14]

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:204: if
(GetWidget()->non_client_view())
Nit: Consider a temp (maybe with auto*) to avoid duplicated calls here (but if
so, consider also fixing the WM_WINDOWPOSCHANGED case similarly).

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:503: SkColor
inactive_border_color = 0xFFA1A1A1;
Can't hardcode this.  It works for Edge because they also hardcode their frame
color, but we don't, and at least for now, need to respect the user's
active/inactive colors, both of which are customizable (though it's trickier to
customize the inactive one...).

It's possible to look up the current frame color via another Chrome function (I
forget the function name, I told Bret recently in a different code review and
can find it if you can't), and then compute the opaque frame color manually from
this, ideally in a way that matches Windows as much as is feasible.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:519:
canvas->DrawRect(gfx::RectF(0, 0, width() * scale, y), paint);
If we want to do this, we need justification in the comments about why
(basically, the info you responded to Bret with).

What does "causes everything to break" mean regarding having a nonzero top
inset?  I'm less familiar with this stuff maybe than you guys are and it'd be
nice to understand if there's some way of getting a transparent top border, even
if it's complicated, that doesn't cost power.  (It seems instinctively like such
a way ought to exist, but with the DWM, you never know...)

[Bret, 2016-10-01 19:53:08]

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:268:
DwmExtendFrameIntoClientArea(GetHWND(), &margins);
If the client area is fully opaque then this block shouldn't be necessary any
more.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:503: SkColor
inactive_border_color = 0xFFA1A1A1;
On 2016/10/01 05:00:14, Peter Kasting wrote:
> Can't hardcode this.  It works for Edge because they also hardcode their frame
> color, but we don't, and at least for now, need to respect the user's
> active/inactive colors, both of which are customizable (though it's trickier
to
> customize the inactive one...).
> 
> It's possible to look up the current frame color via another Chrome function
(I
> forget the function name, I told Bret recently in a different code review and
> can find it if you can't), and then compute the opaque frame color manually
from
> this, ideally in a way that matches Windows as much as is feasible.

I actually think the inactive border color is fixed. The inactive /frame/ color
is not, but that's not relevant here.

Make this value constexpr.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:517: ?
GetThemeProvider()->GetColor(ThemeProperties::COLOR_FRAME)
This is the wrong color for the border. A good test is to set your background to
a solid #FFFFFF image and then set "Automatically pick an accent color from my
background." It'll give you a white border and a blue frame.

You'll need to add code to theme_service_win to get the correct color. I believe
this is correct:

First get the values "ColorizationColor" and "ColorizationColorBalance" from the
same registry key we're getting "AccentColor" from already. Define alpha as
ColorizationColorBalance / 100. Then for each channel R, G, and B of
ColorizationColor calculate: round(channel * alpha + 217 * (1 - alpha))

Since the border color can be different than the frame color it's stored as a
separate value in ColorizationColor. Then it's blended with grey based on
ColorizationColorBalance (which was settable via the UI in Windows 8 but is now
otherwise inaccessible). 217 is just the gray channel value you get when
ColorizationColorBalance is 0.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:519:
canvas->DrawRect(gfx::RectF(0, 0, width() * scale, y), paint);
On 2016/10/01 05:00:14, Peter Kasting wrote:
> If we want to do this, we need justification in the comments about why
> (basically, the info you responded to Bret with).
> 
> What does "causes everything to break" mean regarding having a nonzero top
> inset?  I'm less familiar with this stuff maybe than you guys are and it'd be
> nice to understand if there's some way of getting a transparent top border,
even
> if it's complicated, that doesn't cost power.  (It seems instinctively like
such
> a way ought to exist, but with the DWM, you never know...)

It's definitely impossible. With the insight that using
DwmExtendFrameIntoClientArea costs power I fully believe that Edge is making the
exact same tradeoff of sacrificing the transparent top border.

If you set the top client inset to >0 then Windows gives you a full default
titlebar, and if you mess with the window styles to remove it then hundreds of
lines of HWNDMessageHandler have to be rewritten. I have some detailed notes
about it if you want to know more.

I second adding some comments about this though.

[Peter Kasting, 2016-10-01 20:53:03]

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:503: SkColor
inactive_border_color = 0xFFA1A1A1;
On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> On 2016/10/01 05:00:14, Peter Kasting wrote:
> > Can't hardcode this.  It works for Edge because they also hardcode their
frame
> > color, but we don't, and at least for now, need to respect the user's
> > active/inactive colors, both of which are customizable (though it's trickier
> to
> > customize the inactive one...).
> > 
> > It's possible to look up the current frame color via another Chrome function
> (I
> > forget the function name, I told Bret recently in a different code review
and
> > can find it if you can't), and then compute the opaque frame color manually
> from
> > this, ideally in a way that matches Windows as much as is feasible.
> 
> I actually think the inactive border color is fixed. The inactive /frame/
color
> is not, but that's not relevant here.

I think you're right: my normal inactive border color is ARGB 0x80565656, which
doesn't seem like it reflects my active/inactive window colors in any way.

For Edge's top border I get 0xFFA2A2A2, not A1 as above.  This is basically the
same color you'd get from the "real" inactive border atop #EFEFEF.  Dunno if
that means anything.

I would recheck the A2 vs. A1 issue and document the non-opaque inactive border
color here.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:517: ?
GetThemeProvider()->GetColor(ThemeProperties::COLOR_FRAME)
On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> This is the wrong color for the border. A good test is to set your background
to
> a solid #FFFFFF image and then set "Automatically pick an accent color from my
> background." It'll give you a white border and a blue frame.
> 
> You'll need to add code to theme_service_win to get the correct color. I
believe
> this is correct:
> 
> First get the values "ColorizationColor" and "ColorizationColorBalance" from
the
> same registry key we're getting "AccentColor" from already. Define alpha as
> ColorizationColorBalance / 100. Then for each channel R, G, and B of
> ColorizationColor calculate: round(channel * alpha + 217 * (1 - alpha))

In other words:

  color_utils::AlphaBlend(ColorizationColor, SkColorSetRGB(0xD9, 0xD9, 0xD9),
                          std::round(255 * ColorizationColorBalance / 100.f));

Bret, do you know what the alpha channel value on the ColorizationColor means? 
For me it's set to 0xc4, but I don't know how that affects anything.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:519:
canvas->DrawRect(gfx::RectF(0, 0, width() * scale, y), paint);
On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> On 2016/10/01 05:00:14, Peter Kasting wrote:
> > If we want to do this, we need justification in the comments about why
> > (basically, the info you responded to Bret with).
> > 
> > What does "causes everything to break" mean regarding having a nonzero top
> > inset?  I'm less familiar with this stuff maybe than you guys are and it'd
be
> > nice to understand if there's some way of getting a transparent top border,
> even
> > if it's complicated, that doesn't cost power.  (It seems instinctively like
> such
> > a way ought to exist, but with the DWM, you never know...)
> 
> It's definitely impossible. With the insight that using
> DwmExtendFrameIntoClientArea costs power I fully believe that Edge is making
the
> exact same tradeoff of sacrificing the transparent top border.
> 
> If you set the top client inset to >0 then Windows gives you a full default
> titlebar, and if you mess with the window styles to remove it then hundreds of
> lines of HWNDMessageHandler have to be rewritten. I have some detailed notes
> about it if you want to know more.

"Have to be rewritten" like, out of scope for this patch, but we should probably
go there eventually, or "have to be rewritten" like, let's never do this, it
makes our lives horrible and things have bugs?

[Bret, 2016-10-01 21:43:10]

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:517: ?
GetThemeProvider()->GetColor(ThemeProperties::COLOR_FRAME)
On 2016/10/01 20:53:02, Peter Kasting wrote:
> On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> > This is the wrong color for the border. A good test is to set your
background
> to
> > a solid #FFFFFF image and then set "Automatically pick an accent color from
my
> > background." It'll give you a white border and a blue frame.
> > 
> > You'll need to add code to theme_service_win to get the correct color. I
> believe
> > this is correct:
> > 
> > First get the values "ColorizationColor" and "ColorizationColorBalance" from
> the
> > same registry key we're getting "AccentColor" from already. Define alpha as
> > ColorizationColorBalance / 100. Then for each channel R, G, and B of
> > ColorizationColor calculate: round(channel * alpha + 217 * (1 - alpha))
> 
> In other words:
> 
>   color_utils::AlphaBlend(ColorizationColor, SkColorSetRGB(0xD9, 0xD9, 0xD9),
>                           std::round(255 * ColorizationColorBalance / 100.f));
> 
> Bret, do you know what the alpha channel value on the ColorizationColor means?

> For me it's set to 0xc4, but I don't know how that affects anything.

Ah yes, I never actually tried to write it into Chromium, I just had the math in
my notes.

COLORREF doesn't encode alpha. The high bits are a channel order code, 0xC4
means RGB and 0 means BGR. So don't worry about it.

https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:519:
canvas->DrawRect(gfx::RectF(0, 0, width() * scale, y), paint);
On 2016/10/01 20:53:03, Peter Kasting wrote:
> On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> > On 2016/10/01 05:00:14, Peter Kasting wrote:
> > > If we want to do this, we need justification in the comments about why
> > > (basically, the info you responded to Bret with).
> > > 
> > > What does "causes everything to break" mean regarding having a nonzero top
> > > inset?  I'm less familiar with this stuff maybe than you guys are and it'd
> be
> > > nice to understand if there's some way of getting a transparent top
border,
> > even
> > > if it's complicated, that doesn't cost power.  (It seems instinctively
like
> > such
> > > a way ought to exist, but with the DWM, you never know...)
> > 
> > It's definitely impossible. With the insight that using
> > DwmExtendFrameIntoClientArea costs power I fully believe that Edge is making
> the
> > exact same tradeoff of sacrificing the transparent top border.
> > 
> > If you set the top client inset to >0 then Windows gives you a full default
> > titlebar, and if you mess with the window styles to remove it then hundreds
of
> > lines of HWNDMessageHandler have to be rewritten. I have some detailed notes
> > about it if you want to know more.
> 
> "Have to be rewritten" like, out of scope for this patch, but we should
probably
> go there eventually, or "have to be rewritten" like, let's never do this, it
> makes our lives horrible and things have bugs?

The latter, definitely the latter.

[jbauman, 2016-10-04 00:38:00]

On 2016/10/01 20:53:03, Peter Kasting wrote:
>
https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
> File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):
> 
>
https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:503: SkColor
> inactive_border_color = 0xFFA1A1A1;
> On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> > On 2016/10/01 05:00:14, Peter Kasting wrote:
> > > Can't hardcode this.  It works for Edge because they also hardcode their
> frame
> > > color, but we don't, and at least for now, need to respect the user's
> > > active/inactive colors, both of which are customizable (though it's
trickier
> > to
> > > customize the inactive one...).
> > > 
> > > It's possible to look up the current frame color via another Chrome
function
> > (I
> > > forget the function name, I told Bret recently in a different code review
> and
> > > can find it if you can't), and then compute the opaque frame color
manually
> > from
> > > this, ideally in a way that matches Windows as much as is feasible.
> > 
> > I actually think the inactive border color is fixed. The inactive /frame/
> color
> > is not, but that's not relevant here.
> 
> I think you're right: my normal inactive border color is ARGB 0x80565656,
which
> doesn't seem like it reflects my active/inactive window colors in any way.
> 
> For Edge's top border I get 0xFFA2A2A2, not A1 as above.  This is basically
the
> same color you'd get from the "real" inactive border atop #EFEFEF.  Dunno if
> that means anything.
> 
> I would recheck the A2 vs. A1 issue and document the non-opaque inactive
border
> color here.
> 
Ok, I'm pretty sure I got 0xFFA1A1A1 when testing originally, as opposed to
0xFFA2A2A2 for the "Food & Drink" app. But testing it now I get 0xFFA2A2A2 for
both. So either I measured incorrectly or an update made the consistent.

>
https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:517: ?
> GetThemeProvider()->GetColor(ThemeProperties::COLOR_FRAME)
> On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> > This is the wrong color for the border. A good test is to set your
background
> to
> > a solid #FFFFFF image and then set "Automatically pick an accent color from
my
> > background." It'll give you a white border and a blue frame.
> > 
> > You'll need to add code to theme_service_win to get the correct color. I
> believe
> > this is correct:
> > 
> > First get the values "ColorizationColor" and "ColorizationColorBalance" from
> the
> > same registry key we're getting "AccentColor" from already. Define alpha as
> > ColorizationColorBalance / 100. Then for each channel R, G, and B of
> > ColorizationColor calculate: round(channel * alpha + 217 * (1 - alpha))
> 
> In other words:
> 
>   color_utils::AlphaBlend(ColorizationColor, SkColorSetRGB(0xD9, 0xD9, 0xD9),
>                           std::round(255 * ColorizationColorBalance / 100.f));

Hmm, this formula generally seems to work, but one weird case is when you choose
a solid color background and then get the accent color from the background.

I get AccentColor 0xff8c8c8c, ColorizationColor 0x008c8c8c, and
ColorizationColorBalance 0xfffffff3. The actual border color is 0xff7d7d7d,
though edge draws its top border as 0xff9b9b9b.
> 
> Bret, do you know what the alpha channel value on the ColorizationColor means?

> For me it's set to 0xc4, but I don't know how that affects anything.
> 
>
https://codereview.chromium.org/2381283003/diff/1/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:519:
> canvas->DrawRect(gfx::RectF(0, 0, width() * scale, y), paint);
> On 2016/10/01 19:53:08, Bret Sepulveda wrote:
> > On 2016/10/01 05:00:14, Peter Kasting wrote:
> > > If we want to do this, we need justification in the comments about why
> > > (basically, the info you responded to Bret with).
> > > 
> > > What does "causes everything to break" mean regarding having a nonzero top
> > > inset?  I'm less familiar with this stuff maybe than you guys are and it'd
> be
> > > nice to understand if there's some way of getting a transparent top
border,
> > even
> > > if it's complicated, that doesn't cost power.  (It seems instinctively
like
> > such
> > > a way ought to exist, but with the DWM, you never know...)
> > 
> > It's definitely impossible. With the insight that using
> > DwmExtendFrameIntoClientArea costs power I fully believe that Edge is making
> the
> > exact same tradeoff of sacrificing the transparent top border.
> > 
> > If you set the top client inset to >0 then Windows gives you a full default
> > titlebar, and if you mess with the window styles to remove it then hundreds
of
> > lines of HWNDMessageHandler have to be rewritten. I have some detailed notes
> > about it if you want to know more.
> 
> "Have to be rewritten" like, out of scope for this patch, but we should
probably
> go there eventually, or "have to be rewritten" like, let's never do this, it
> makes our lives horrible and things have bugs?

[Bret, 2016-10-04 20:46:34]

> Hmm, this formula generally seems to work, but one weird case is when you
choose
> a solid color background and then get the accent color from the background.
> 
> I get AccentColor 0xff8c8c8c, ColorizationColor 0x008c8c8c, and
> ColorizationColorBalance 0xfffffff3. The actual border color is 0xff7d7d7d,
> though edge draws its top border as 0xff9b9b9b.

If I set my background to a solid white, grey, or black I get AccentColor
#0078D7, ColorizationColor #FFFFFF and ColorizationColorBalance 0x59, which is
what I got when I tried this before on a different computer. The border draws as
#FBFBFB and doing the math on those registry values comes out to be the same. So
I'm not sure what's different about your setup. What color are you using?

Hmm, that ColorizationColorBalance value seems out of range. It's supposed to be
a percentage, so it really shouldn't be higher than 0x64, which might explain
why Edge is drawing differently than Windows. Also I don't think 0xFF is a valid
high-bit value for colors in the registry, so that's confusing me too.

[jbauman, 2016-10-04 22:42:30]

On 2016/10/04 20:46:34, Bret Sepulveda wrote:
> > Hmm, this formula generally seems to work, but one weird case is when you
> choose
> > a solid color background and then get the accent color from the background.
> > 
> > I get AccentColor 0xff8c8c8c, ColorizationColor 0x008c8c8c, and
> > ColorizationColorBalance 0xfffffff3. The actual border color is 0xff7d7d7d,
> > though edge draws its top border as 0xff9b9b9b.
> 
> If I set my background to a solid white, grey, or black I get AccentColor
> #0078D7, ColorizationColor #FFFFFF and ColorizationColorBalance 0x59, which is
> what I got when I tried this before on a different computer. The border draws
as
> #FBFBFB and doing the math on those registry values comes out to be the same.
So
> I'm not sure what's different about your setup. What color are you using?

It's the exact same with any background color. The title bar of the
personalization window also turns an obnoxious blue, if that setting's checked,
but I don't know where that's coming from. classic programs have the expected
gray titlebar.

This is on the anniversary update (1607). On another windows 10 machine with
version 1511 I get the same as you. I'm currently upgrading that one, so we'll
see if anything different happens.
> 
> Hmm, that ColorizationColorBalance value seems out of range. It's supposed to
be
> a percentage, so it really shouldn't be higher than 0x64, which might explain
> why Edge is drawing differently than Windows. Also I don't think 0xFF is a
valid
> high-bit value for colors in the registry, so that's confusing me too.

[Bret, 2016-10-04 23:05:41]

On 2016/10/04 22:42:30, jbauman wrote:
> On 2016/10/04 20:46:34, Bret Sepulveda wrote:
> > > Hmm, this formula generally seems to work, but one weird case is when you
> > choose
> > > a solid color background and then get the accent color from the
background.
> > > 
> > > I get AccentColor 0xff8c8c8c, ColorizationColor 0x008c8c8c, and
> > > ColorizationColorBalance 0xfffffff3. The actual border color is
0xff7d7d7d,
> > > though edge draws its top border as 0xff9b9b9b.
> > 
> > If I set my background to a solid white, grey, or black I get AccentColor
> > #0078D7, ColorizationColor #FFFFFF and ColorizationColorBalance 0x59, which
is
> > what I got when I tried this before on a different computer. The border
draws
> as
> > #FBFBFB and doing the math on those registry values comes out to be the
same.
> So
> > I'm not sure what's different about your setup. What color are you using?
> 
> It's the exact same with any background color. The title bar of the
> personalization window also turns an obnoxious blue, if that setting's
checked,
> but I don't know where that's coming from. classic programs have the expected
> gray titlebar.
> 
> This is on the anniversary update (1607). On another windows 10 machine with
> version 1511 I get the same as you. I'm currently upgrading that one, so we'll
> see if anything different happens.
> > 
> > Hmm, that ColorizationColorBalance value seems out of range. It's supposed
to
> be
> > a percentage, so it really shouldn't be higher than 0x64, which might
explain
> > why Edge is drawing differently than Windows. Also I don't think 0xFF is a
> valid
> > high-bit value for colors in the registry, so that's confusing me too.

I'm on 1511. It'd be super obnoxious if they changed the formula in anniversary
edition! Lemme know what you find out.

[Peter Kasting, 2016-10-05 00:24:08]

I have anniversary here at home if you guys want me to cross-check something. 
Not following closely enough to see precisely what to test.

I'll refrain from further review passes until you're happy with the behavior.

[Bret, 2016-10-05 00:29:31]

On 2016/10/05 00:24:08, Peter Kasting wrote:
> I have anniversary here at home if you guys want me to cross-check something. 
> Not following closely enough to see precisely what to test.
> 
> I'll refrain from further review passes until you're happy with the behavior.

This would be useful:
1. Set your background to solid black or white.
2. Check "Automatically pick an accent color from my background" and "Show color
on Start, taskbar, action center, and titlebar"
3. Tell me what color your active titlebar and border color are.
4. Open up regedit, go to HKEY_CURRENT_USER/SOFTWARE/Microsoft/Windows/DWM and
tell me what values you have for AccentColor, ColorizationColor and
ColorizationColorBalance.

I get blue/white titlebar/border, but it sounds like anniversary edition might
be different, and might have a different formula for calculating the border
color.

[jbauman, 2016-10-05 01:29:54]

On 2016/10/05 00:29:31, Bret Sepulveda wrote:
> On 2016/10/05 00:24:08, Peter Kasting wrote:
> > I have anniversary here at home if you guys want me to cross-check
something. 
> > Not following closely enough to see precisely what to test.
> > 
> > I'll refrain from further review passes until you're happy with the
behavior.
> 
> This would be useful:
> 1. Set your background to solid black or white.
> 2. Check "Automatically pick an accent color from my background" and "Show
color
> on Start, taskbar, action center, and titlebar"
> 3. Tell me what color your active titlebar and border color are.
> 4. Open up regedit, go to HKEY_CURRENT_USER/SOFTWARE/Microsoft/Windows/DWM and
> tell me what values you have for AccentColor, ColorizationColor and
> ColorizationColorBalance.
> 
> I get blue/white titlebar/border, but it sounds like anniversary edition might
> be different, and might have a different formula for calculating the border
> color.

After upgrading to anniversary update, that computer now behaves identically
with the other, i.e. ColorizationColorBalance is 0xfffffff3. I've never seen it
as a different negative value, so I wonder what that could mean.

[jbauman, 2016-10-05 21:24:27]

On 2016/10/05 01:29:54, jbauman wrote:
> On 2016/10/05 00:29:31, Bret Sepulveda wrote:
> > On 2016/10/05 00:24:08, Peter Kasting wrote:
> > > I have anniversary here at home if you guys want me to cross-check
> something. 
> > > Not following closely enough to see precisely what to test.
> > > 
> > > I'll refrain from further review passes until you're happy with the
> behavior.
> > 
> > This would be useful:
> > 1. Set your background to solid black or white.
> > 2. Check "Automatically pick an accent color from my background" and "Show
> color
> > on Start, taskbar, action center, and titlebar"
> > 3. Tell me what color your active titlebar and border color are.
> > 4. Open up regedit, go to HKEY_CURRENT_USER/SOFTWARE/Microsoft/Windows/DWM
and
> > tell me what values you have for AccentColor, ColorizationColor and
> > ColorizationColorBalance.
> > 
> > I get blue/white titlebar/border, but it sounds like anniversary edition
might
> > be different, and might have a different formula for calculating the border
> > color.
> 
> After upgrading to anniversary update, that computer now behaves identically
> with the other, i.e. ColorizationColorBalance is 0xfffffff3. I've never seen
it
> as a different negative value, so I wonder what that could mean.

Also, restarting DWM causes the border to be set to #C4C4C4 (though edge doesn't
notice and keeps the top at #9B9B9B), while restarting explorer or toggling the
personalization setting causes it to be reset to #7D7D7D.

[Bret, 2016-10-05 21:31:06]

On 2016/10/05 21:24:27, jbauman wrote:
> On 2016/10/05 01:29:54, jbauman wrote:
> > On 2016/10/05 00:29:31, Bret Sepulveda wrote:
> > > On 2016/10/05 00:24:08, Peter Kasting wrote:
> > > > I have anniversary here at home if you guys want me to cross-check
> > something. 
> > > > Not following closely enough to see precisely what to test.
> > > > 
> > > > I'll refrain from further review passes until you're happy with the
> > behavior.
> > > 
> > > This would be useful:
> > > 1. Set your background to solid black or white.
> > > 2. Check "Automatically pick an accent color from my background" and "Show
> > color
> > > on Start, taskbar, action center, and titlebar"
> > > 3. Tell me what color your active titlebar and border color are.
> > > 4. Open up regedit, go to HKEY_CURRENT_USER/SOFTWARE/Microsoft/Windows/DWM
> and
> > > tell me what values you have for AccentColor, ColorizationColor and
> > > ColorizationColorBalance.
> > > 
> > > I get blue/white titlebar/border, but it sounds like anniversary edition
> might
> > > be different, and might have a different formula for calculating the
border
> > > color.
> > 
> > After upgrading to anniversary update, that computer now behaves identically
> > with the other, i.e. ColorizationColorBalance is 0xfffffff3. I've never seen
> it
> > as a different negative value, so I wonder what that could mean.
> 
> Also, restarting DWM causes the border to be set to #C4C4C4 (though edge
doesn't
> notice and keeps the top at #9B9B9B), while restarting explorer or toggling
the
> personalization setting causes it to be reset to #7D7D7D.

What are the other registry values when ColorizationColorBalance is 0xFFFFFFF3
and what is the resulting border color? I'm mostly interested in figuring out
the formula for that value and then as long as we're matching what Windows does
it can get as wild as it wants to.

[jbauman, 2016-10-05 21:45:34]

On 2016/10/05 21:31:06, Bret Sepulveda wrote:
> On 2016/10/05 21:24:27, jbauman wrote:
> > On 2016/10/05 01:29:54, jbauman wrote:
> > > On 2016/10/05 00:29:31, Bret Sepulveda wrote:
> > > > On 2016/10/05 00:24:08, Peter Kasting wrote:
> > > > > I have anniversary here at home if you guys want me to cross-check
> > > something. 
> > > > > Not following closely enough to see precisely what to test.
> > > > > 
> > > > > I'll refrain from further review passes until you're happy with the
> > > behavior.
> > > > 
> > > > This would be useful:
> > > > 1. Set your background to solid black or white.
> > > > 2. Check "Automatically pick an accent color from my background" and
"Show
> > > color
> > > > on Start, taskbar, action center, and titlebar"
> > > > 3. Tell me what color your active titlebar and border color are.
> > > > 4. Open up regedit, go to
HKEY_CURRENT_USER/SOFTWARE/Microsoft/Windows/DWM
> > and
> > > > tell me what values you have for AccentColor, ColorizationColor and
> > > > ColorizationColorBalance.
> > > > 
> > > > I get blue/white titlebar/border, but it sounds like anniversary edition
> > might
> > > > be different, and might have a different formula for calculating the
> border
> > > > color.
> > > 
> > > After upgrading to anniversary update, that computer now behaves
identically
> > > with the other, i.e. ColorizationColorBalance is 0xfffffff3. I've never
seen
> > it
> > > as a different negative value, so I wonder what that could mean.
> > 
> > Also, restarting DWM causes the border to be set to #C4C4C4 (though edge
> doesn't
> > notice and keeps the top at #9B9B9B), while restarting explorer or toggling
> the
> > personalization setting causes it to be reset to #7D7D7D.
> 
> What are the other registry values when ColorizationColorBalance is 0xFFFFFFF3
> and what is the resulting border color? I'm mostly interested in figuring out
> the formula for that value and then as long as we're matching what Windows
does
> it can get as wild as it wants to.

Border color: #7D7D7D
Titlebar color: #8C8C8C

AccentColor: 0xff8c8c8c
ColorizationAfterglow: 0x008c8c8c
ColorizationAfterglowBalance: 0x0000000a
ColorizationBlurBalance: 0x00000067
ColorizationColor: 0x008c8c8c
ColorizationColorBalance: 0xfffffff3
ColorizationGlassAttribute: 0x1
ColorPrevalence: 0x1
Composition: 0x1
EnableAeroPeek: 0x1
EnableWindowColorization: 0x1

After restarting DWM, positive values of ColorizationColorBalance behave as
expected, but negative values give a border color of #C4C4C4 instead, until
explorer is restarted.

[Bret, 2016-10-05 22:33:23]

On 2016/10/05 21:45:34, jbauman wrote:
> On 2016/10/05 21:31:06, Bret Sepulveda wrote:
> > On 2016/10/05 21:24:27, jbauman wrote:
> > > On 2016/10/05 01:29:54, jbauman wrote:
> > > > On 2016/10/05 00:29:31, Bret Sepulveda wrote:
> > > > > On 2016/10/05 00:24:08, Peter Kasting wrote:
> > > > > > I have anniversary here at home if you guys want me to cross-check
> > > > something. 
> > > > > > Not following closely enough to see precisely what to test.
> > > > > > 
> > > > > > I'll refrain from further review passes until you're happy with the
> > > > behavior.
> > > > > 
> > > > > This would be useful:
> > > > > 1. Set your background to solid black or white.
> > > > > 2. Check "Automatically pick an accent color from my background" and
> "Show
> > > > color
> > > > > on Start, taskbar, action center, and titlebar"
> > > > > 3. Tell me what color your active titlebar and border color are.
> > > > > 4. Open up regedit, go to
> HKEY_CURRENT_USER/SOFTWARE/Microsoft/Windows/DWM
> > > and
> > > > > tell me what values you have for AccentColor, ColorizationColor and
> > > > > ColorizationColorBalance.
> > > > > 
> > > > > I get blue/white titlebar/border, but it sounds like anniversary
edition
> > > might
> > > > > be different, and might have a different formula for calculating the
> > border
> > > > > color.
> > > > 
> > > > After upgrading to anniversary update, that computer now behaves
> identically
> > > > with the other, i.e. ColorizationColorBalance is 0xfffffff3. I've never
> seen
> > > it
> > > > as a different negative value, so I wonder what that could mean.
> > > 
> > > Also, restarting DWM causes the border to be set to #C4C4C4 (though edge
> > doesn't
> > > notice and keeps the top at #9B9B9B), while restarting explorer or
toggling
> > the
> > > personalization setting causes it to be reset to #7D7D7D.
> > 
> > What are the other registry values when ColorizationColorBalance is
0xFFFFFFF3
> > and what is the resulting border color? I'm mostly interested in figuring
out
> > the formula for that value and then as long as we're matching what Windows
> does
> > it can get as wild as it wants to.
> 
> Border color: #7D7D7D
> Titlebar color: #8C8C8C
> 
> AccentColor: 0xff8c8c8c
> ColorizationAfterglow: 0x008c8c8c
> ColorizationAfterglowBalance: 0x0000000a
> ColorizationBlurBalance: 0x00000067
> ColorizationColor: 0x008c8c8c
> ColorizationColorBalance: 0xfffffff3
> ColorizationGlassAttribute: 0x1
> ColorPrevalence: 0x1
> Composition: 0x1
> EnableAeroPeek: 0x1
> EnableWindowColorization: 0x1
> 
> After restarting DWM, positive values of ColorizationColorBalance behave as
> expected, but negative values give a border color of #C4C4C4 instead, until
> explorer is restarted.

Okay, I didn't understand what you were saying before. That's very bizarre,
seems like a bug. If Edge can't keep up with the color change on a DWM restart
then there's no hope for us either.

My best guess is that ColorizationColorBalance is unsigned and ceiled at 120,
which gives the right border color. If you add that to the formula and change
the registry value for ColorizationColor do you still match Windows? What if you
change ColorizationColorBalance to something else weird? My other guess is that
if anything goes wrong it just sets the border color to #7D7D7D.

[jbauman, 2016-10-06 00:55:54]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 00:56:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 01:24:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 01:24:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jbauman, 2016-10-06 01:29:43]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 01:30:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 02:26:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 02:26:58]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-10-06 02:30:22]

PTAL. This seems to work with getting the color from ColorizationColor.

[Peter Kasting, 2016-10-06 08:02:47]

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:160: COLOR_ACCENT_BORDER,
Nit: Briefly describe what this is.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:61: // This can accept alpha < 0 or >
1.
...But we never call it with < 0, right?

Graphically, I don't really understand what the equation below does when we call
with alpha > 1.  Sure, the math can work out, but what does it _mean_?

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:77: dwm_accent_color_ =
SK_ColorWHITE;
I didn't check: for native drawing, does the ColorPrevalence value above affect
whether the colorization color value is ignored?

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:86: // slideshow.
Is it true that the colorization color balance is always 100 or less, or else
120?  Or does it take on values in the range between 100 and 120?

This gets back to my question above; I don't really understand what these values
mean conceptually.

Underneath this question, I'm also asking if Windows' Anniversary Update
behavior is really something that makes any sense, that we should imitate
slavishly, or if it's just a bug, that produces weirder behavior in edge cases
than the pre-Anniversary algorithm and does nothing else better.  If there's a
conceptual way to define what's going on here so it has some plausible reason
for existence, even if it's an unusual one, then OK, but if this is just totally
broken in Windows natively, then I don't know whether I want to reproduce logic
that makes no sense, especially if they go and fix it in the next update.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:89: const uint8_t gray_level = 0xd9;
Nit: Comment where this comes from.  More accurately, comment above what all
these concepts mean, by describing roughly what the colorization color and color
balance mean.  Somewhere in such an explanation, a phrase like "the color
balance controls the blend between the colorization color and a neutral #D9D9D9"
would explain why this number is down here.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:91: // colorization_color's alpha can
be 0xc4 or 0.
Nit: As Bret noted, this isn't really an alpha value.  Explain that it's not an
alpha value, noting what these main, as a means to explaining why it's both safe
and necessary to overwrite this.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:204: //
Activation border may have changed color.
Grammar Nazi Nit: Add "The" in front

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:296: //
don't extend the glass in at all because it won't be visible.
Nit: Clearer (to me):

Don't extend the DWM frame in unless we're letting the DWM draw a non-maximized,
non-fullscreen window.  Outside that scenario, no DWM-drawn area will be
visible.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:507: gfx::Rect
tabstrip_bounds = GetBoundsForTabStrip(browser_view()->tabstrip());
Nit: Let's move these constants down to just above the code that uses them.  We
can then combine the comment added here with the one below.  See next comment
for proposed wording.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:525: // compositing.
Nit: This comment partially restates info from the comment above.  If we move
the constant down anyway and combine the comments, here's a suggestion I hope
gives much more background for future puzzled spelunkers:

Draw the top of the accent border.

We let the DWM do this for the other sides of the window, by insetting the
client area to leave nonclient area available (see
BrowserDesktopWindowTreeHostWin::GetClientAreaInsets()).  However, along the top
window edge, we have to have zero nonclient area, or the DWM will draw a full
set of caption buttons outside our client area.  [pk note: I think saying
"outside our client area" is more correct than "outside the window", but not
sure.  Also, some of this info probably belongs in
BrowserDesktopWindowTreeHostWin::GetClientAreaInsets() in addition to/instead of
here.]

We could ask the DWM to draw the top accent border in the client area (by
ensuring the top margin is nonzero in
BrowserDesktopWindowTreeHostWin::UpdateDWMFrame()), but this requires that we
leave part of the client surface transparent and let the DWM draw the contents
underneath and blend.  This costs power (~10% extra draw playing a maximized
1080p60 video).  If we draw this ourselves, we can make the client surface fully
opaque and avoid this cost.

However, this requires that the accent border be fully opaque.  Active window
accent borders are opaque, and we can get the correct color from the theme
provider.  Inactive window accent borders are natively ARGB 0x80565656.  There's
no single opaque color that will match that against all backgrounds, and we
don't know what's behind us anyway.  So we copy Edge (which also has an opaque
top border) and assume most users will have relatively bright backgrounds; they
use #A2A2A2 for the inactive top border, which matches what the native border
would look like atop #EFEFEF.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:530:
canvas->DrawRect(gfx::RectF(0, 0, width() * scale, y), paint);
Nit: Blank line below this, since the comment above doesn't apply to the
subsequent code.

[Bret, 2016-10-06 20:48:10]

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:61: // This can accept alpha < 0 or >
1.
On 2016/10/06 08:02:46, Peter Kasting (OOO Oct. 6-9) wrote:
> ...But we never call it with < 0, right?
> 
> Graphically, I don't really understand what the equation below does when we
call
> with alpha > 1.  Sure, the math can work out, but what does it _mean_?

Yeah don't mention it's allowed to be "out of bounds" if that's not useful in
practice. I would call it "color_balance" maybe? I was calling it "alpha" only
because this is the alpha blending formula. On the other hand if we end up doing
something different below we can go back to using utility functions (which would
be nice).

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:77: dwm_accent_color_ =
SK_ColorWHITE;
On 2016/10/06 08:02:46, Peter Kasting (OOO Oct. 6-9) wrote:
> I didn't check: for native drawing, does the ColorPrevalence value above
affect
> whether the colorization color value is ignored?

No, the border is always colored the same way even if ColorPrevalence is 0.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:86: // slideshow.
On 2016/10/06 08:02:46, Peter Kasting (OOO Oct. 6-9) wrote:
> Is it true that the colorization color balance is always 100 or less, or else
> 120?  Or does it take on values in the range between 100 and 120?
> 
> This gets back to my question above; I don't really understand what these
values
> mean conceptually.
> 
> Underneath this question, I'm also asking if Windows' Anniversary Update
> behavior is really something that makes any sense, that we should imitate
> slavishly, or if it's just a bug, that produces weirder behavior in edge cases
> than the pre-Anniversary algorithm and does nothing else better.  If there's a
> conceptual way to define what's going on here so it has some plausible reason
> for existence, even if it's an unusual one, then OK, but if this is just
totally
> broken in Windows natively, then I don't know whether I want to reproduce
logic
> that makes no sense, especially if they go and fix it in the next update.

I just tried thrashing my own registry values on 1511 and it's using
ColorizationColorBalance=27(???) when it's out of bounds (with
ColorizationColor=#FF00FF it gives me a border color of #E39EE3). Cf Edge's top
border, which seems to always be using ColorizationColorBalance=80, though that
means it never matches Windows. This seems to match anniversary's "restart dwm
but not explorer" case.

It also seems like 100 is the actual max. Past that point Edge and Windows start
doing different things (even below 120). Unless that too is different on
anniversary edition I'm tempted to just match what Edge does and not worry about
it too much, since ColorizationColorBalance getting set to such a flagrantly
out-of-bounds value does seem like a bug.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.h (right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.h:37: SkColor dwm_accent_color_;
Hmm this name is confusing because the AccentColor is actually the frame color,
even if it shows up in more limited circumstances than the border color. Maybe
just call it dwm_border_color_? Or maybe dwm_accent_border_color_ which is
consistent with what you're calling it everywhere else.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:296: //
don't extend the glass in at all because it won't be visible.
On 2016/10/06 08:02:47, Peter Kasting (OOO Oct. 6-9) wrote:
> Nit: Clearer (to me):
> 
> Don't extend the DWM frame in unless we're letting the DWM draw a
non-maximized,
> non-fullscreen window.  Outside that scenario, no DWM-drawn area will be
> visible.

Not quite accurate because DWM still draws non-client area, and it doesn't
explain why the custom titlebar part is important unless you're familiar with
how it works. I think the comment is okay as is.

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:525: // compositing.
On 2016/10/06 08:02:47, Peter Kasting (OOO Oct. 6-9) wrote:
> Nit: This comment partially restates info from the comment above.  If we move
> the constant down anyway and combine the comments, here's a suggestion I hope
> gives much more background for future puzzled spelunkers:
> 
> Draw the top of the accent border.
> 
> We let the DWM do this for the other sides of the window, by insetting the
> client area to leave nonclient area available (see
> BrowserDesktopWindowTreeHostWin::GetClientAreaInsets()).  However, along the
top
> window edge, we have to have zero nonclient area, or the DWM will draw a full
> set of caption buttons outside our client area.  [pk note: I think saying
> "outside our client area" is more correct than "outside the window", but not
> sure.  Also, some of this info probably belongs in
> BrowserDesktopWindowTreeHostWin::GetClientAreaInsets() in addition to/instead
of
> here.]
> 
> We could ask the DWM to draw the top accent border in the client area (by
> ensuring the top margin is nonzero in
> BrowserDesktopWindowTreeHostWin::UpdateDWMFrame()), but this requires that we
> leave part of the client surface transparent and let the DWM draw the contents
> underneath and blend.  This costs power (~10% extra draw playing a maximized
> 1080p60 video).  If we draw this ourselves, we can make the client surface
fully
> opaque and avoid this cost.
> 
> However, this requires that the accent border be fully opaque.  Active window
> accent borders are opaque, and we can get the correct color from the theme
> provider.  Inactive window accent borders are natively ARGB 0x80565656. 
There's
> no single opaque color that will match that against all backgrounds, and we
> don't know what's behind us anyway.  So we copy Edge (which also has an opaque
> top border) and assume most users will have relatively bright backgrounds;
they
> use #A2A2A2 for the inactive top border, which matches what the native border
> would look like atop #EFEFEF.

I like Peter's suggestion in general. I made some edits:

"
Draw the top of the accent border.

We let the DWM do this for the other sides of the window by insetting the
client area to leave nonclient area available. However, along the top
window edge, we have to have zero nonclient area or the DWM will draw a full
native titlebar outside our client area [add a similar sentence to
BrowserDesktopWindowTreeHostWin::GetClientAreaInsets too]. See
BrowserDesktopWindowTreeHostWin::GetClientAreaInsets().

We could ask the DWM to draw the top accent border in the client area (by
calling DwmExtendFrameIntoClientArea in
BrowserDesktopWindowTreeHostWin::UpdateDWMFrame()), but this requires that we
leave part of the client surface transparent. Forcing DWM to blend with us costs
power even when the window isn't visible [I would avoid specific data here]. If
we draw this ourselves, we can make the client surface fully opaque and avoid
this cost.

So the accent border also has to be opaque, but native inactive borders are
#565656 with 80% alpha. We copy Edge (which also custom-draws its top border)
and use #A2A2A2 instead.
"

In editing that comment I realized that Edge uses #090909 when it's dark-themed.
Add a TODO somewhere since we don't have dark theme support yet and this is easy
to miss when we add it.

[jbauman, 2016-10-06 21:55:06]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 21:55:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 22:44:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 22:44:25]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-10-06 23:12:06]

PTAL. Modified the comments to your suggestions and switched the behavior to
match Edge's.

[Bret, 2016-10-07 18:21:56]

lgtm w nits

https://codereview.chromium.org/2381283003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:105: //
else DWM will draw a full native titlebar outside the client area.
nit: Windows, not DWM

https://codereview.chromium.org/2381283003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:177: auto*
non_client_view = GetWidget()->non_client_view();
nit: use NonClientView

https://codereview.chromium.org/2381283003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:204: auto*
non_client_view = GetWidget()->non_client_view();
nit: here too

https://codereview.chromium.org/2381283003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:525: // blend with us
costs power even for the parts of the window that are
nit: "Forcing DWM to blend with us costs power even when it has no effect."?

[jbauman, 2016-10-07 19:25:26]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 19:25:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 20:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 20:13:04]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-10-11 20:01:39]

LGTM

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:296: //
don't extend the glass in at all because it won't be visible.
On 2016/10/06 20:48:10, Bret Sepulveda wrote:
> On 2016/10/06 08:02:47, Peter Kasting (OOO Oct. 6-9) wrote:
> > Nit: Clearer (to me):
> > 
> > Don't extend the DWM frame in unless we're letting the DWM draw a
> non-maximized,
> > non-fullscreen window.  Outside that scenario, no DWM-drawn area will be
> > visible.
> 
> Not quite accurate because DWM still draws non-client area, and it doesn't
> explain why the custom titlebar part is important unless you're familiar with
> how it works. I think the comment is okay as is.

What about:

Don't extend the DWM frame in unless we're letting the DWM draw a non-maximized,
non-fullscreen window.  Outside that scenario, the DWM will not be drawing in
the window client area.

This corrects the inaccuracy you mention.  The reason I'm suggesting a change is
because I'd like to spend less time restating the code and more time on "why". 
It's subtle, though.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
opaque color.
Nit: I would be clearer about the meaning of this byte, e.g.:

|colorization_color|'s high byte is not an alpha value but a channel order code,
so replace it with 0xff to make this an opaque ARGB color.

(Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:94: 255 * colorization_color_balance
/ 100.f);
This implicitly truncates float to int.  Do one of these (they should be
equivalent):

gfx::ToRoundedInt(255 * colorization_color_balance / 100.f)
Tween::LinearIntValueBetween(colorization_color_balance / 100., 0, 255);

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:205: //
The activation border may have changed color.
Tiny nit: I'd put this comment above the line above, just because that temp is
really part of this logical code block the comment describes.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:522: // calling
DwmExtendFrameIntoClientArea in
Nit: Add () after function name

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:525: // blend with us
costs power even when it has no effect. If we draw this
Nit: What does "even when it has no effect" mean here?  Are you describing the
maximized window case?

I would probably just leave this clause out as it's not really clear in context
what it means.  If you want to leave it, maybe make it clearer: "costs power
(even if the accent border area winds up offscreen, e.g. for a maximized
window)."

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:532: // TODO(jbauman):
Match Edge's #090909 when using the dark theme.
Which dark theme are you referring to?  Chrome incognito?  Something in Windows?

[Bret, 2016-10-11 20:48:20]

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:296: //
don't extend the glass in at all because it won't be visible.
On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> On 2016/10/06 20:48:10, Bret Sepulveda wrote:
> > On 2016/10/06 08:02:47, Peter Kasting (OOO Oct. 6-9) wrote:
> > > Nit: Clearer (to me):
> > > 
> > > Don't extend the DWM frame in unless we're letting the DWM draw a
> > non-maximized,
> > > non-fullscreen window.  Outside that scenario, no DWM-drawn area will be
> > > visible.
> > 
> > Not quite accurate because DWM still draws non-client area, and it doesn't
> > explain why the custom titlebar part is important unless you're familiar
with
> > how it works. I think the comment is okay as is.
> 
> What about:
> 
> Don't extend the DWM frame in unless we're letting the DWM draw a
non-maximized,
> non-fullscreen window.  Outside that scenario, the DWM will not be drawing in
> the window client area.
> 
> This corrects the inaccuracy you mention.  The reason I'm suggesting a change
is
> because I'd like to spend less time restating the code and more time on "why".

> It's subtle, though.

Okay, I agree about not restating the code. Maybe this doesn't need a comment at
all? Or if you think it does, maybe just "Don't extend the DWM frame in when it
won't be visible." I still think "won't be visible" is more accurate because DWM
still draws things when even when we draw over it (you can tell if Chrome dies
somehow and only the Windows-drawn area is visible).

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
opaque color.
On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> Nit: I would be clearer about the meaning of this byte, e.g.:
> 
> |colorization_color|'s high byte is not an alpha value but a channel order
code,
> so replace it with 0xff to make this an opaque ARGB color.
> 
> (Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)

Oh oops! You're right, this could be wrong if the color is in BGR order.
ColorizationColor is always in RGB format in practice unless someone mucks with
the registry, but it's more correct to use the helper that accent_color uses
above, skia::COLORREFToSkColor, which uses Windows macros. Then this whole
comment can go away.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:532: // TODO(jbauman):
Match Edge's #090909 when using the dark theme.
On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> Which dark theme are you referring to?  Chrome incognito?  Something in
Windows?

In Edge it's a user setting, but that's just because it doesn't respect the
frame color at all. I imagine we'd do it in the same cases where we'd want to
draw dark-themed caption buttons (but we haven't decided on those cases yet).

[jbauman, 2016-10-11 20:55:24]

On 2016/10/11 20:48:20, Bret Sepulveda wrote:
>
https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
> (right):
> 
>
https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:296: //
> don't extend the glass in at all because it won't be visible.
> On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > On 2016/10/06 20:48:10, Bret Sepulveda wrote:
> > > On 2016/10/06 08:02:47, Peter Kasting (OOO Oct. 6-9) wrote:
> > > > Nit: Clearer (to me):
> > > > 
> > > > Don't extend the DWM frame in unless we're letting the DWM draw a
> > > non-maximized,
> > > > non-fullscreen window.  Outside that scenario, no DWM-drawn area will be
> > > > visible.
> > > 
> > > Not quite accurate because DWM still draws non-client area, and it doesn't
> > > explain why the custom titlebar part is important unless you're familiar
> with
> > > how it works. I think the comment is okay as is.
> > 
> > What about:
> > 
> > Don't extend the DWM frame in unless we're letting the DWM draw a
> non-maximized,
> > non-fullscreen window.  Outside that scenario, the DWM will not be drawing
in
> > the window client area.
> > 
> > This corrects the inaccuracy you mention.  The reason I'm suggesting a
change
> is
> > because I'd like to spend less time restating the code and more time on
"why".
> 
> > It's subtle, though.
> 
> Okay, I agree about not restating the code. Maybe this doesn't need a comment
at
> all? Or if you think it does, maybe just "Don't extend the DWM frame in when
it
> won't be visible." I still think "won't be visible" is more accurate because
DWM
> still draws things when even when we draw over it (you can tell if Chrome dies
> somehow and only the Windows-drawn area is visible).
> 
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
> File chrome/browser/themes/theme_service_win.cc (right):
> 
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
> chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
> opaque color.
> On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > Nit: I would be clearer about the meaning of this byte, e.g.:
> > 
> > |colorization_color|'s high byte is not an alpha value but a channel order
> code,
> > so replace it with 0xff to make this an opaque ARGB color.
> > 
> > (Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)
> 
> Oh oops! You're right, this could be wrong if the color is in BGR order.
> ColorizationColor is always in RGB format in practice unless someone mucks
with
> the registry, but it's more correct to use the helper that accent_color uses
> above, skia::COLORREFToSkColor, which uses Windows macros. Then this whole
> comment can go away.
> 
I don't see any indication that the high byte means anything useful to us. On my
system it's always 0xc4 except when in the weird ColorizationColorBalance mode,
where it changes to be 0, and on my Windows 7 machine it seems to be 0x6b. In
all cases the color value is actually 0xXXRRGGBB.

skia::COLORREFToSkColor always interprets it with the wrong byte order.

>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):
> 
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:532: //
TODO(jbauman):
> Match Edge's #090909 when using the dark theme.
> On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > Which dark theme are you referring to?  Chrome incognito?  Something in
> Windows?
> 
> In Edge it's a user setting, but that's just because it doesn't respect the
> frame color at all. I imagine we'd do it in the same cases where we'd want to
> draw dark-themed caption buttons (but we haven't decided on those cases yet).

[Peter Kasting, 2016-10-11 20:56:52]

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/2381283003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:296: //
don't extend the glass in at all because it won't be visible.
On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > On 2016/10/06 20:48:10, Bret Sepulveda wrote:
> > > On 2016/10/06 08:02:47, Peter Kasting (OOO Oct. 6-9) wrote:
> > > > Nit: Clearer (to me):
> > > > 
> > > > Don't extend the DWM frame in unless we're letting the DWM draw a
> > > non-maximized,
> > > > non-fullscreen window.  Outside that scenario, no DWM-drawn area will be
> > > > visible.
> > > 
> > > Not quite accurate because DWM still draws non-client area, and it doesn't
> > > explain why the custom titlebar part is important unless you're familiar
> with
> > > how it works. I think the comment is okay as is.
> > 
> > What about:
> > 
> > Don't extend the DWM frame in unless we're letting the DWM draw a
> non-maximized,
> > non-fullscreen window.  Outside that scenario, the DWM will not be drawing
in
> > the window client area.
> > 
> > This corrects the inaccuracy you mention.  The reason I'm suggesting a
change
> is
> > because I'd like to spend less time restating the code and more time on
"why".
> 
> > It's subtle, though.
> 
> Okay, I agree about not restating the code. Maybe this doesn't need a comment
at
> all? Or if you think it does, maybe just "Don't extend the DWM frame in when
it
> won't be visible." I still think "won't be visible" is more accurate because
DWM
> still draws things when even when we draw over it (you can tell if Chrome dies
> somehow and only the Windows-drawn area is visible).

I like your simple comment suggestion.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
opaque color.
On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > Nit: I would be clearer about the meaning of this byte, e.g.:
> > 
> > |colorization_color|'s high byte is not an alpha value but a channel order
> code,
> > so replace it with 0xff to make this an opaque ARGB color.
> > 
> > (Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)
> 
> Oh oops! You're right, this could be wrong if the color is in BGR order.
> ColorizationColor is always in RGB format in practice unless someone mucks
with
> the registry, but it's more correct to use the helper that accent_color uses
> above, skia::COLORREFToSkColor, which uses Windows macros. Then this whole
> comment can go away.

Hmm, that function looks like it assumes 0BGR instead of looking at the high
byte.

Maybe we need to use that function and add a high byte check?  I see about 4
callsites of this currently, so hopefully auditing these to be sure that was the
right thing to do wouldn't be onerous.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:532: // TODO(jbauman):
Match Edge's #090909 when using the dark theme.
On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > Which dark theme are you referring to?  Chrome incognito?  Something in
> Windows?
> 
> In Edge it's a user setting, but that's just because it doesn't respect the
> frame color at all. I imagine we'd do it in the same cases where we'd want to
> draw dark-themed caption buttons (but we haven't decided on those cases yet).

I see.

I use a dark frame color on my own system and the 0xA2 inactive accent border
still seems reasonable, so my suspicion is that we may not ever want this,
though I'm not sure.

I might omit this comment for now if we don't know if/when we'll be doing this,
especially if we didn't also change the other three accent borders to be opaque
at the same time, since it'd look pretty weird.

[Peter Kasting, 2016-10-11 21:09:34]

On 2016/10/11 20:55:24, jbauman wrote:
> I don't see any indication that the high byte means anything useful to us. On
my
> system it's always 0xc4 except when in the weird ColorizationColorBalance
mode,
> where it changes to be 0, and on my Windows 7 machine it seems to be 0x6b. In
> all cases the color value is actually 0xXXRRGGBB.

Random: 0x6b = ~0xc4

> skia::COLORREFToSkColor always interprets it with the wrong byte order.

Yeah, looking up more docs online it seems like COLORREF is always BGR, with the
top byte some sort of set of flags for palettes.  And the various Microsoft docs
say these colors are ARGB.  I don't know what the top byte means.  Maybe just
have the comment say the top byte's meaning is unclear.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:72: if
((dwm_key_->ReadValueDW(L"ColorizationColor", &colorization_color) ==
Does calling DwmGetColorizationColor() get this value?

You could probably get both these values with DwmGetColorizationParameters()
(see http://www.codeproject.com/Articles/610909/Changing-Windows-Aero-Color on
how to use this undocumented API), but that may be more trouble than it's worth.

[Bret, 2016-10-11 21:10:15]

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
opaque color.
On 2016/10/11 20:56:51, Peter Kasting (very slow) wrote:
> On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> > On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > > Nit: I would be clearer about the meaning of this byte, e.g.:
> > > 
> > > |colorization_color|'s high byte is not an alpha value but a channel order
> > code,
> > > so replace it with 0xff to make this an opaque ARGB color.
> > > 
> > > (Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)
> > 
> > Oh oops! You're right, this could be wrong if the color is in BGR order.
> > ColorizationColor is always in RGB format in practice unless someone mucks
> with
> > the registry, but it's more correct to use the helper that accent_color uses
> > above, skia::COLORREFToSkColor, which uses Windows macros. Then this whole
> > comment can go away.
> 
> Hmm, that function looks like it assumes 0BGR instead of looking at the high
> byte.
> 
> Maybe we need to use that function and add a high byte check?  I see about 4
> callsites of this currently, so hopefully auditing these to be sure that was
the
> right thing to do wouldn't be onerous.

Oh I misread the definition of COLORREFToSkColor. The first branch in the
preprocessor switch looks correct to me, it's using the Get[RGB]Value macros,
which are provided by Windows and should always do the right thing. So maybe
we're actually getting the second branch? It's not a huge deal in either case,
it'd just be nice to be correct if someone manually edits the registry to be
0BGR order lol.

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:532: // TODO(jbauman):
Match Edge's #090909 when using the dark theme.
On 2016/10/11 20:56:51, Peter Kasting (very slow) wrote:
> On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> > On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > > Which dark theme are you referring to?  Chrome incognito?  Something in
> > Windows?
> > 
> > In Edge it's a user setting, but that's just because it doesn't respect the
> > frame color at all. I imagine we'd do it in the same cases where we'd want
to
> > draw dark-themed caption buttons (but we haven't decided on those cases
yet).
> 
> I see.
> 
> I use a dark frame color on my own system and the 0xA2 inactive accent border
> still seems reasonable, so my suspicion is that we may not ever want this,
> though I'm not sure.
> 
> I might omit this comment for now if we don't know if/when we'll be doing
this,
> especially if we didn't also change the other three accent borders to be
opaque
> at the same time, since it'd look pretty weird.

I think it's somewhere between extremely difficult and impossible to custom-draw
the other borders so they're opaque. I'm okay having it be A2 even in the dark
theme case; it does look a little strange when Edge does it.

[jbauman, 2016-10-11 21:24:53]

On 2016/10/11 21:10:15, Bret Sepulveda wrote:
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
> File chrome/browser/themes/theme_service_win.cc (right):
> 
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
> chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
> opaque color.
> On 2016/10/11 20:56:51, Peter Kasting (very slow) wrote:
> > On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> > > On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > > > Nit: I would be clearer about the meaning of this byte, e.g.:
> > > > 
> > > > |colorization_color|'s high byte is not an alpha value but a channel
order
> > > code,
> > > > so replace it with 0xff to make this an opaque ARGB color.
> > > > 
> > > > (Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)
> > > 
> > > Oh oops! You're right, this could be wrong if the color is in BGR order.
> > > ColorizationColor is always in RGB format in practice unless someone mucks
> > with
> > > the registry, but it's more correct to use the helper that accent_color
uses
> > > above, skia::COLORREFToSkColor, which uses Windows macros. Then this whole
> > > comment can go away.
> > 
> > Hmm, that function looks like it assumes 0BGR instead of looking at the high
> > byte.
> > 
> > Maybe we need to use that function and add a high byte check?  I see about 4
> > callsites of this currently, so hopefully auditing these to be sure that was
> the
> > right thing to do wouldn't be onerous.
> 
> Oh I misread the definition of COLORREFToSkColor. The first branch in the
> preprocessor switch looks correct to me, it's using the Get[RGB]Value macros,
> which are provided by Windows and should always do the right thing. So maybe
> we're actually getting the second branch? It's not a huge deal in either case,
> it'd just be nice to be correct if someone manually edits the registry to be
> 0BGR order lol.

Looks like GetRValue, etc., are just dumb bit shifts, so they're no help either
unfortunately.
> 
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):
> 
>
https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:532: //
TODO(jbauman):
> Match Edge's #090909 when using the dark theme.
> On 2016/10/11 20:56:51, Peter Kasting (very slow) wrote:
> > On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> > > On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > > > Which dark theme are you referring to?  Chrome incognito?  Something in
> > > Windows?
> > > 
> > > In Edge it's a user setting, but that's just because it doesn't respect
the
> > > frame color at all. I imagine we'd do it in the same cases where we'd want
> to
> > > draw dark-themed caption buttons (but we haven't decided on those cases
> yet).
> > 
> > I see.
> > 
> > I use a dark frame color on my own system and the 0xA2 inactive accent
border
> > still seems reasonable, so my suspicion is that we may not ever want this,
> > though I'm not sure.
> > 
> > I might omit this comment for now if we don't know if/when we'll be doing
> this,
> > especially if we didn't also change the other three accent borders to be
> opaque
> > at the same time, since it'd look pretty weird.
> 
> I think it's somewhere between extremely difficult and impossible to
custom-draw
> the other borders so they're opaque. I'm okay having it be A2 even in the dark
> theme case; it does look a little strange when Edge does it.

[Peter Kasting, 2016-10-11 21:26:26]

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service_win.cc (right):

https://codereview.chromium.org/2381283003/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:89: // an 0xff alpha to make an
opaque color.
On 2016/10/11 21:10:15, Bret Sepulveda wrote:
> On 2016/10/11 20:56:51, Peter Kasting (very slow) wrote:
> > On 2016/10/11 20:48:19, Bret Sepulveda wrote:
> > > On 2016/10/11 20:01:38, Peter Kasting (very slow) wrote:
> > > > Nit: I would be clearer about the meaning of this byte, e.g.:
> > > > 
> > > > |colorization_color|'s high byte is not an alpha value but a channel
order
> > > code,
> > > > so replace it with 0xff to make this an opaque ARGB color.
> > > > 
> > > > (Bret, we don't need to check for 0x00 and byteswap BGR -> RGB, do we?)
> > > 
> > > Oh oops! You're right, this could be wrong if the color is in BGR order.
> > > ColorizationColor is always in RGB format in practice unless someone mucks
> > with
> > > the registry, but it's more correct to use the helper that accent_color
uses
> > > above, skia::COLORREFToSkColor, which uses Windows macros. Then this whole
> > > comment can go away.
> > 
> > Hmm, that function looks like it assumes 0BGR instead of looking at the high
> > byte.
> > 
> > Maybe we need to use that function and add a high byte check?  I see about 4
> > callsites of this currently, so hopefully auditing these to be sure that was
> the
> > right thing to do wouldn't be onerous.
> 
> Oh I misread the definition of COLORREFToSkColor. The first branch in the
> preprocessor switch looks correct to me, it's using the Get[RGB]Value macros,
> which are provided by Windows and should always do the right thing. So maybe
> we're actually getting the second branch? It's not a huge deal in either case,
> it'd just be nice to be correct if someone manually edits the registry to be
> 0BGR order lol.

As noted in my reply to jbauman, I think this is always 0x??RRGGBB, and the top
byte is not a channel order indicator.  It's not a COLORREF stuffed into a reg
key.  So I think the existing code here is good.

[Bret, 2016-10-11 21:28:51]

> As noted in my reply to jbauman, I think this is always 0x??RRGGBB, and the
top
> byte is not a channel order indicator.  It's not a COLORREF stuffed into a reg
> key.  So I think the existing code here is good.

Okay, SGTM too.

[jbauman, 2016-10-11 22:55:07]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 22:55:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-10-11 22:56:34]

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

[jbauman, 2016-10-11 22:56:35]

estade@: content/browser/themes OWNERS review.

[commit-bot: I haz the power, 2016-10-11 23:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 23:54:10]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-11 23:56:40]

I would have expected you to need to update ThemeProperties::GetDefaultColor.
Can you get rid of the default case in the switch statement in that function and
add your new color there (it's ok to return a placeholder color with a
NOTREACHED; I just think it's better to be explicit about that).

I'll be ooo for a week or so, so you may want to find a different owner for
c/b/themes.

[jbauman, 2016-10-12 02:12:30]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 02:12:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 02:58:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 02:58:08]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-10-12 21:04:38]

jbauman@chromium.org changed reviewers:
+ pkotwicz@chromium.org
- estade@chromium.org

[jbauman, 2016-10-12 21:04:39]

pkotwicz@: chrome/browser/themes/ OWNERS review

[pkotwicz, 2016-10-12 21:45:48]

chrome/browser/themes/ LGTM

[jbauman, 2016-10-13 01:30:47]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-10-13 01:30:47]

The patchset sent to the CQ was uploaded after l-g-t-m from bsep@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2381283003/#ps120001
(title: "remove default")

[commit-bot: I haz the power, 2016-10-13 01:31:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 01:37:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-13 01:41:01]

Description was changed from

==========
Have Chrome draw top window border when using custom titlebar.

This is the only transparent part of the window, so after this the
window can be made opaque. The appearance should be similar to other
native apps, and in particular to edge and other UWP apps that do the
same thing.

BUG=543904
TEST=--windows10-custom-titlebar with background slideshow and accent color
taken from background.
==========

to

==========
Have Chrome draw top window border when using custom titlebar.

This is the only transparent part of the window, so after this the
window can be made opaque. The appearance should be similar to other
native apps, and in particular to edge and other UWP apps that do the
same thing.

BUG=543904
TEST=--windows10-custom-titlebar with background slideshow and accent color
taken from background.

Committed: https://crrev.com/13552791a9ce4282df4ec658f07bf59a45ffb385
Cr-Commit-Position: refs/heads/master@{#424931}
==========

[commit-bot: I haz the power, 2016-10-13 01:41:02]

Patchset 7 (id:??) landed as
https://crrev.com/13552791a9ce4282df4ec658f07bf59a45ffb385
Cr-Commit-Position: refs/heads/master@{#424931}