Make SysColorChangeListener Windows-only instead of having an empty shell.

Make SysColorChangeListener Windows-only instead of having an empty shell.

The class in only used on Windows and we end up building an empty class
on other platforms. It leads to "unused private fields" warnings that
could be avoided.

BUG=447445
Committed: https://crrev.com/20bb4c4aa6b26509c3040c562c5b051bd575e7eb
Cr-Commit-Position: refs/heads/master@{#337815}

[mlamouri (slow - plz ping), 2015-07-03 15:21:10]

mlamouri@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2015-07-04 06:22:22]

https://codereview.chromium.org/1219533009/diff/1/ui/gfx/sys_color_change_lis...
File ui/gfx/sys_color_change_listener.cc (right):

https://codereview.chromium.org/1219533009/diff/1/ui/gfx/sys_color_change_lis...
ui/gfx/sys_color_change_listener.cc:97: #endif
Don't you need to remove this too?

[mlamouri (slow - plz ping), 2015-07-06 08:57:33]

Done. I've also added a ifdef around a IsInvertedColorScheme(). It's not ideal.
Another solution would be to have a color_utils.{h,cc} or something similar.
WDYT?

PTAL.

[sadrul, 2015-07-06 18:26:32]

On 2015/07/06 08:57:33, Mounir Lamouri wrote:
> Done. I've also added a ifdef around a IsInvertedColorScheme(). It's not
ideal.
> Another solution would be to have a color_utils.{h,cc} or something similar.
> WDYT?
> 
> PTAL.

Looks like IsInvertedColorScheme() is called in a few non-windows places. Maybe
IsInvertedColorScheme() could move into color_utils.h, and the non-windows impl
could live in color_utils.cc?

[mlamouri (slow - plz ping), 2015-07-06 19:01:29]

I moved IsInvertedColorScheme() to color_utils. PTAL.

[sadrul, 2015-07-06 19:05:10]

On 2015/07/06 19:01:29, Mounir Lamouri wrote:
> I moved IsInvertedColorScheme() to color_utils. PTAL.

I think the windows build will fail because sys_color_change_listener calls the
removed UpdateInvertedColorScheme() function. (that's why I suggested having
just the non-windows version into color_utils.cc, with maybe a comment pointing
to where the windows version lives)

https://codereview.chromium.org/1219533009/diff/40001/ui/gfx/sys_color_change...
File ui/gfx/sys_color_change_listener.cc (right):

https://codereview.chromium.org/1219533009/diff/40001/ui/gfx/sys_color_change...
ui/gfx/sys_color_change_listener.cc:16: 
remove this blank like too

[mlamouri (slow - plz ping), 2015-07-06 19:19:45]

On 2015/07/06 at 19:05:10, sadrul wrote:
> On 2015/07/06 19:01:29, Mounir Lamouri wrote:
> > I moved IsInvertedColorScheme() to color_utils. PTAL.

Oups! Sorry :)

[mlamouri (slow - plz ping), 2015-07-06 19:24:34]

PTAL.

[sadrul, 2015-07-07 04:36:54]

Mostly nits. lgtm with these addressed.

Note: the try failure in win8_chromium_ng looks related.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.cc#...
ui/gfx/color_utils.cc:322: #if !defined(OS_WIN) // OS_WIN implementation lives
in sys_color_change_listener
Comment could be in a separate line.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.cc#...
ui/gfx/color_utils.cc:326: #endif // !defined(OS_WIN)
Two spaces before // for EOL comments

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.h#n...
ui/gfx/color_utils.h:109: // ScopedSysColorChangeListener, below.
Update the comment.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/sys_color_chang...
File ui/gfx/sys_color_change_listener.cc (right):

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/sys_color_chang...
ui/gfx/sys_color_change_listener.cc:17: #include
"ui/gfx/win/singleton_hwnd_observer.h"
Should be in the previous block of #includes

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/sys_color_chang...
ui/gfx/sys_color_change_listener.cc:38: }  // anonymous namespace
I hadn't seen us use 'anonymous namespace' like this before. But looks like some
code does use it. Weird.

From a quick search, looks like '// anonymous namespace' like this is used ~300
times, and just '// namespace' is used ~10K times in the chrome tree. So leave
it as '// namespace'?

[mlamouri (slow - plz ping), 2015-07-07 13:45:14]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2015-07-07 13:45:15]

jochen@, can you review the chrome/ bits?

Thanks :)

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.cc#...
ui/gfx/color_utils.cc:322: #if !defined(OS_WIN) // OS_WIN implementation lives
in sys_color_change_listener
On 2015/07/07 at 04:36:54, sadrul wrote:
> Comment could be in a separate line.

Done.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.cc#...
ui/gfx/color_utils.cc:326: #endif // !defined(OS_WIN)
On 2015/07/07 at 04:36:54, sadrul wrote:
> Two spaces before // for EOL comments

Done.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/color_utils.h#n...
ui/gfx/color_utils.h:109: // ScopedSysColorChangeListener, below.
On 2015/07/07 at 04:36:54, sadrul wrote:
> Update the comment.

Done.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/sys_color_chang...
File ui/gfx/sys_color_change_listener.cc (right):

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/sys_color_chang...
ui/gfx/sys_color_change_listener.cc:17: #include
"ui/gfx/win/singleton_hwnd_observer.h"
On 2015/07/07 at 04:36:54, sadrul wrote:
> Should be in the previous block of #includes

Done.

https://codereview.chromium.org/1219533009/diff/120001/ui/gfx/sys_color_chang...
ui/gfx/sys_color_change_listener.cc:38: }  // anonymous namespace
On 2015/07/07 at 04:36:54, sadrul wrote:
> I hadn't seen us use 'anonymous namespace' like this before. But looks like
some code does use it. Weird.
> 
> From a quick search, looks like '// anonymous namespace' like this is used
~300 times, and just '// namespace' is used ~10K times in the chrome tree. So
leave it as '// namespace'?

If you want.

[jochen (gone - plz use gerrit), 2015-07-08 13:07:57]

lgtm

[mlamouri (slow - plz ping), 2015-07-08 13:27:23]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-07-08 13:27:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1219533009/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-08 13:27:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219533009/160001

[commit-bot: I haz the power, 2015-07-08 14:30:29]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-08 14:31:33]

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