Monitor the system native frame color on Win 10.

Monitor the system native frame color on Win 10.

GetColor(COLOR_FRAME...) will now report the actual in-use color for Win 10
native frames.  This is not used yet, but will be useful for:
(a) Dynamically choosing the right incognito icon color
(b) Dynamically choosing the right COLOR_TOOLBAR_TOP_SEPARATOR
(c) Eventually, converting Win 10 to custom-drawing the frame but still
    respecting the user's color choice

BUG=505013, 585470
TEST=none

Committed: https://crrev.com/9fcf69e0bc440b16661c39c4b9f13182f0430725
Cr-Commit-Position: refs/heads/master@{#378989}

[Peter Kasting, 2016-02-26 01:33:06]

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

[Evan Stade, 2016-02-26 16:36:32]

Is there a way we can avoid adding all these ifdefs to shared code?
ThemeServiceWin perhaps?

[Peter Kasting, 2016-02-26 19:51:17]

On 2016/02/26 16:36:32, Evan Stade wrote:
> Is there a way we can avoid adding all these ifdefs to shared code?
> ThemeServiceWin perhaps?

We can, but I wasn't sure they were worth it.

The two obvious options:

(1) Composition -- have some kind of a NativeThemeMonitor (or whatever) object
that we create on Windows.  We could avoid ifdefs by having this object
available, but no-op, on other platforms.
(2) Inheritance -- subclass ThemeService on Windows.  We need to touch whatever
creates ThemeService objects to be an appropriate factory function that vends
the right platform-specific object.  We also have to muck with how GetColor()
works in annoying ways, because we basically want to insert functionality after
the custom theme has had a chance to supply a color but before the rest of the
base class function runs.

I suspect of the two (1) is easier.  Do you think it's worth doing?  It feels
like probably a wash to me but without actually writing it I can't be certain.

[Evan Stade, 2016-02-26 20:07:02]

On 2016/02/26 19:51:17, Peter Kasting wrote:
> On 2016/02/26 16:36:32, Evan Stade wrote:
> > Is there a way we can avoid adding all these ifdefs to shared code?
> > ThemeServiceWin perhaps?
> 
> We can, but I wasn't sure they were worth it.
> 
> The two obvious options:
> 
> (1) Composition -- have some kind of a NativeThemeMonitor (or whatever) object
> that we create on Windows.  We could avoid ifdefs by having this object
> available, but no-op, on other platforms.
> (2) Inheritance -- subclass ThemeService on Windows.  We need to touch
whatever
> creates ThemeService objects to be an appropriate factory function that vends
> the right platform-specific object.  We also have to muck with how GetColor()
> works in annoying ways, because we basically want to insert functionality
after
> the custom theme has had a chance to supply a color but before the rest of the
> base class function runs.
> 
> I suspect of the two (1) is easier.  Do you think it's worth doing?  It feels
> like probably a wash to me but without actually writing it I can't be certain.

I was thinking 2. Instantiation doesn't seem like much of an issue as we already
do something similar here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/the...

For GetColor, I'd imagine ThemeService having a virtual member called
GetDefaultColor such that GetColor becomes:
a) get custom color if it exists
b) call GetDefaultColor
ThemeService::GetDefaultColor just contains what's already in GetColor below the
custom theme stuff. ThemeServiceWin can exten

[Bret, 2016-02-29 18:29:02]

bsep@chromium.org changed reviewers:
+ bsep@chromium.org

[Bret, 2016-02-29 18:29:03]

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

https://codereview.chromium.org/1744483002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:181: HKEY_CURRENT_USER,
L"SOFTWARE\\Microsoft\\Windows\\DWM", KEY_READ));
Are you sure this is actually the frame color? When I messed around with this
earlier Windows would only report to me the base accent color (i.e. the color
you get when you pick the swatch from the settings page), not the actual color
it uses to draw an active frame. They're slightly different.

If you're not using this to actually draw the active frame this is probably
fine, but don't call it "frame color" because that's not quite correct.

[Peter Kasting, 2016-02-29 19:57:06]

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

https://codereview.chromium.org/1744483002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:181: HKEY_CURRENT_USER,
L"SOFTWARE\\Microsoft\\Windows\\DWM", KEY_READ));
On 2016/02/29 18:29:03, Bret Sepulveda wrote:
> Are you sure this is actually the frame color? When I messed around with this
> earlier Windows would only report to me the base accent color (i.e. the color
> you get when you pick the swatch from the settings page), not the actual color
> it uses to draw an active frame. They're slightly different.

On my system, the swatch color and the frame color are exactly the same for all
the swatches I tried (~9) and the color in the registry matched those for the
cases I looked at (~4).

If there's a particular problematic swatch on your system, let me know so I can
test it on mine.

[Peter Kasting, 2016-03-02 03:24:21]

PTAL; modified to use inheritance.

[Bret, 2016-03-02 22:54:04]

lgtm otherwise

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

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:37: case
ThemeProperties::COLOR_FRAME:
i still think "frame" is a confusing name for this color even after you
explained the context. we might not even use it for the frame

[Evan Stade, 2016-03-03 01:24:26]

lgtm

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

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:14:
ThemeServiceWin::ThemeServiceWin() {
init dwm_frame_color_?

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:38: case
ThemeProperties::COLOR_FRAME_INCOGNITO:
I don't think you can hit this (or the other incognito one below). You'd get
COLOR_FRAME with incognito == true instead, although you don't care about
incognito's value in this case.

[Peter Kasting, 2016-03-03 09:23:06]

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

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:14:
ThemeServiceWin::ThemeServiceWin() {
On 2016/03/03 01:24:25, Evan Stade wrote:
> init dwm_frame_color_?

There's no need, since the code in the conditional here is guaranteed to
initialize it in a superset of the cases where we read it.

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:37: case
ThemeProperties::COLOR_FRAME:
On 2016/03/02 22:54:03, Bret Sepulveda wrote:
> i still think "frame" is a confusing name for this color even after you
> explained the context. we might not even use it for the frame

I understand where you're coming from.

That said, the "frame" in Chrome UI code refers to e.g. the opaque frame, glass
frame, etc.  I think it's reasonable in principle for us to refer to the primary
color of the UI drawn by those classes as the "frame color".

Much more importantly, this is terminology that's used in a number of different
places, including e.g. themes where it's externally exposed by the same name
(theme authors set the "frame" color in their themes, and similar names for
related colors).  We can't modify these externally-facing strings (at least
without extraordinary measures).  Accordingly, changing this name would make the
connection to themes confusing, and that connection is arguably the most
important thing in how we name these constants.

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:38: case
ThemeProperties::COLOR_FRAME_INCOGNITO:
On 2016/03/03 01:24:25, Evan Stade wrote:
> I don't think you can hit this (or the other incognito one below). You'd get
> COLOR_FRAME with incognito == true instead, although you don't care about
> incognito's value in this case.

You're right.  I misread the original code before I factored this out. 
Simplified.

[Peter Kasting, 2016-03-03 09:23:14]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-03-03 09:23:15]

The patchset sent to the CQ was uploaded after l-g-t-m from bsep@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1744483002/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2016-03-03 09:23:28]

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

[commit-bot: I haz the power, 2016-03-03 10:08:56]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Monitor the system native frame color on Win 10.

GetColor(COLOR_FRAME...) will now report the actual in-use color for Win 10
native frames.  This is not used yet, but will be useful for:
(a) Dynamically choosing the right incognito icon color
(b) Dynamically choosing the right COLOR_TOOLBAR_TOP_SEPARATOR
(c) Eventually, converting Win 10 to custom-drawing the frame but still
    respecting the user's color choice

BUG=505013,585470
TEST=none
==========

to

==========
Monitor the system native frame color on Win 10.

GetColor(COLOR_FRAME...) will now report the actual in-use color for Win 10
native frames.  This is not used yet, but will be useful for:
(a) Dynamically choosing the right incognito icon color
(b) Dynamically choosing the right COLOR_TOOLBAR_TOP_SEPARATOR
(c) Eventually, converting Win 10 to custom-drawing the frame but still
    respecting the user's color choice

BUG=505013,585470
TEST=none

Committed: https://crrev.com/9fcf69e0bc440b16661c39c4b9f13182f0430725
Cr-Commit-Position: refs/heads/master@{#378989}
==========

[commit-bot: I haz the power, 2016-03-03 10:10:35]

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

[Evan Stade, 2016-03-03 18:17:19]

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

https://codereview.chromium.org/1744483002/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service_win.cc:14:
ThemeServiceWin::ThemeServiceWin() {
On 2016/03/03 09:23:06, Peter Kasting wrote:
> On 2016/03/03 01:24:25, Evan Stade wrote:
> > init dwm_frame_color_?
> 
> There's no need, since the code in the conditional here is guaranteed to
> initialize it in a superset of the cases where we read it.

yes, but we run static analysis tools that don't know that so I think the
general policy is to always init PODs

[Will Harris, 2016-03-07 18:44:21]

hello, this has broken the windows 10 FYI bot - see crbug.com/592676

I will revert this soon unless I hear otherwise. Thanks.

[Will Harris, 2016-03-07 18:46:28]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1771933002/ by wfh@chromium.org.

The reason for reverting is: This breaks the win10 bot. see bug 592676.