[MD] Implement incognito colors as a NativeTheme (for Aura).

[MD] Implement incognito colors as a NativeTheme

For ChromeOS (and at least for now, Linux), this is NativeThemeAuraDark. For Windows, this is NativeThemeWinDark, which paints controls like NativeThemeWin but uses NativeThemeAuraDark for colors.

This CL focuses on the location bar and omnibox. It adds a number of colors (most of those which have already been spec'd) but is missing lots more colors.

BUG=501377
Committed: https://crrev.com/563454636cfe3df6a57fff84560e8a89e4f6b175
Cr-Commit-Position: refs/heads/master@{#359956}

[Evan Stade, 2015-11-10 01:26:22]

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

[Evan Stade, 2015-11-10 01:26:23]

Before I go too far in this direction, what do you think of the general approach
of creating a different NativeTheme for incognito mode? Any better ideas come to
mind?

Notes about this CL:
- tons of colors are missing (not yet spec'd)
- it's Linux only at the moment because Linux is the only place where it's
already easy to provide a non-standard NativeTheme
- I think Windows would want to use NativeThemeAuraDark without modification. I
don't think there's any other sane way to derive these colors (unless Peter has
some idea?)
- There will probably need to be an incognito ThemeProvider as well for certain
things

[Peter Kasting, 2015-11-10 01:43:00]

It seems like an OK general idea, but I'd like to see what it would take to use
this on Windows.

[tdanderson, 2015-11-10 15:27:16]

On 2015/11/10 01:43:00, Peter Kasting wrote:
> It seems like an OK general idea, but I'd like to see what it would take to
use
> this on Windows.

This idea also seems good to me. How much work would be involved
in order to provide a non-standard NativeTheme on the other
platforms? IIUC would that be the only thing standing in our way
to start making use of this on CrOS?

[tdanderson, 2015-11-10 15:28:35]

tdanderson@chromium.org changed reviewers:
+ rogerta@chromium.org

[tdanderson, 2015-11-10 15:28:35]

+rogerta@ I believe you are doing work with system theming on Windows.

[Roger Tawa OOO till Jul 10th, 2015-11-10 16:03:04]

On 2015/11/10 15:28:35, tdanderson wrote:
> +rogerta@ I believe you are doing work with system theming on Windows.

I'll take a closer look this CL.  Here is a CL that I've start experimenting
with to draw native looking buttons on win10 with a theme.  Let me know if you
think my CL is in the right direction.

[Roger Tawa OOO till Jul 10th, 2015-11-10 16:03:30]

On 2015/11/10 16:03:04, Roger Tawa wrote:
> On 2015/11/10 15:28:35, tdanderson wrote:
> > +rogerta@ I believe you are doing work with system theming on Windows.
> 
> I'll take a closer look this CL.  Here is a CL that I've start experimenting
> with to draw native looking buttons on win10 with a theme.  Let me know if you
> think my CL is in the right direction.

And here is the CL link:
https://codereview.chromium.org/1431303002

[Evan Stade, 2015-11-10 17:00:55]

On 2015/11/10 15:27:16, tdanderson wrote:
> On 2015/11/10 01:43:00, Peter Kasting wrote:
> > It seems like an OK general idea, but I'd like to see what it would take to
> use
> > this on Windows.
> 
> This idea also seems good to me. How much work would be involved
> in order to provide a non-standard NativeTheme on the other
> platforms? IIUC would that be the only thing standing in our way
> to start making use of this on CrOS?

For both Windows and CrOS, I think we'd have to do something similar to what
Linux does today. See [1] --- ChromeOS just always uses the aura theme, whereas
Windows and Linux delegate to the DesktopWindowTreeHost. On Linux that in turn
passes through to the LinuxUi, which is an interface in ui/ but implemented in
chrome/ (the implementation uses the profile of the associated window to decide
what kind of theme to show). On Windows, the implementation is currently in ui/
[2]. So for Windows and CrOS both, we'd probably need to move the
GetNativeTheme[ForWindow] implementation to chrome/ via an interface just like
Linux.

Anyway, that's how you would get the right NativeTheme. As far as what the right
NativeTheme is, for Windows, I'd say NativeThemeWin for normal and
NativeThemeAuraDark for incognito.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...

[Evan Stade, 2015-11-10 18:10:25]

On 2015/11/10 17:00:55, Evan Stade wrote:
> On 2015/11/10 15:27:16, tdanderson wrote:
> > On 2015/11/10 01:43:00, Peter Kasting wrote:
> > > It seems like an OK general idea, but I'd like to see what it would take
to
> > use
> > > this on Windows.
> > 
> > This idea also seems good to me. How much work would be involved
> > in order to provide a non-standard NativeTheme on the other
> > platforms? IIUC would that be the only thing standing in our way
> > to start making use of this on CrOS?
> 
> For both Windows and CrOS, I think we'd have to do something similar to what
> Linux does today. See [1] --- ChromeOS just always uses the aura theme,
whereas
> Windows and Linux delegate to the DesktopWindowTreeHost. On Linux that in turn
> passes through to the LinuxUi, which is an interface in ui/ but implemented in
> chrome/ (the implementation uses the profile of the associated window to
decide
> what kind of theme to show). On Windows, the implementation is currently in
ui/
> [2]. So for Windows and CrOS both, we'd probably need to move the
> GetNativeTheme[ForWindow] implementation to chrome/ via an interface just like
> Linux.
> 
> Anyway, that's how you would get the right NativeTheme. As far as what the
right
> NativeTheme is, for Windows, I'd say NativeThemeWin for normal and
> NativeThemeAuraDark for incognito.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...

Perhaps ThemeProvider is this interface and the above plan could be enacted by
changing this line [3] to GetThemeProvider()->GetNativeTheme() (we might need to
pass a window type as well)

[3]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi...

[Evan Stade, 2015-11-11 00:24:49]

To answer Peter's question about how to use this on Windows (and CrOS): I tried
adding a function to ThemeProvider, but Widget::GetThemeProvider() only returns
a ThemeService for browser windows (not, for example, the omnibox results
popup). So I moved the new function to ViewsDelegate. This works on CrOS and I
assume it works on Windows but haven't tested. It relies on the fact the
BrowserView sets the profile as a keyed value on the native window and the
relationships tracked by TransientWindowManager. PTAL ps4 (particularly
widget.cc and chrome_views_delegate.cc). If this generally lgty I'll try to add
a few more colors and then ask for a full review.

[Peter Kasting, 2015-11-11 00:34:46]

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_aura_dark.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura_dark.h:15: class NATIVE_THEME_EXPORT
NativeThemeAuraDark : public NativeThemeAura {
I'm a bit concerned that on Windows we're proposing to use NativeThemeWin
normally and NativeThemeAuraDark for incognito, since the inheritance chains
are:

NativeTheme -> NativeThemeWin
NativeTheme -> NativeThemeBase -> NativeThemeAura -> NativeThemeAuraDark

I worry about what sorts of differences will result from this besides just a few
colors.  It would be more reassuring if the new "dark" theme was a subclass or
direct sibling to the "normal" theme class in all cases.

It's making me wonder whether we're barking up the wrong tree; perhaps instead
of a different NativeTheme class, we should be using some sort of mixin object
to get colors and icons that can differ between normal and incognito mode,
either in the ultimate consumer, or inside the NativeThemeXXX implementation
itself?  Or maybe we should actually put "return incognito ? x : y" sorts of
statements directly into the functions in NativeThemeWin or the like?

https://codereview.chromium.org/1438513003/diff/60001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/views/views_delegate...
ui/views/views_delegate.h:197: virtual ui::NativeTheme*
GetNonDefaultNativeTheme(const Widget* widget);
Can this return the platform default native theme in cases where there's no
override?  If so, we can just name this GetNativeTheme() and maybe always use it
from Widget?

If not, maybe GetNativeThemeOverride() would be slightly better, since
"non-default" implies it's returning some sort of secondary theme so that
callers can use both, or something strange.

[Evan Stade, 2015-11-11 01:05:32]

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_aura_dark.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura_dark.h:15: class NATIVE_THEME_EXPORT
NativeThemeAuraDark : public NativeThemeAura {
On 2015/11/11 00:34:46, Peter Kasting wrote:
> I'm a bit concerned that on Windows we're proposing to use NativeThemeWin
> normally and NativeThemeAuraDark for incognito, since the inheritance chains
> are:
> 
> NativeTheme -> NativeThemeWin
> NativeTheme -> NativeThemeBase -> NativeThemeAura -> NativeThemeAuraDark
> 
> I worry about what sorts of differences will result from this besides just a
few
> colors.  It would be more reassuring if the new "dark" theme was a subclass or
> direct sibling to the "normal" theme class in all cases.
> 
> It's making me wonder whether we're barking up the wrong tree; perhaps instead
> of a different NativeTheme class, we should be using some sort of mixin object
> to get colors and icons that can differ between normal and incognito mode,
> either in the ultimate consumer, or inside the NativeThemeXXX implementation
> itself?  Or maybe we should actually put "return incognito ? x : y" sorts of
> statements directly into the functions in NativeThemeWin or the like?

Well, incognito ? x : y doesn't sound that viable because all the native theme
instances are singletons, and I don't see how it helps us much anyway (just
mixes the two different themes together). The reason I went with a
NativeThemeAura subclass is that NativeThemeAura contains some shared logic
(e.g. SkColorSetA) for both incognito and normal profiles, but yea, this may
mess up widget drawing for Windows?

How about this:

class NativeThemeAuraDark : public NativeThemeAura {...}
class NativeThemeWinDark : public NativeThemeWin {
  SkColor GetColor(id) { return NativeThemeAuraDark::GetColor(id); }
}

Eventually if we find colors that shouldn't be the same on Windows incognito we
can add those before the NativeThemeAuraDark redirect.

https://codereview.chromium.org/1438513003/diff/60001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/views/views_delegate...
ui/views/views_delegate.h:197: virtual ui::NativeTheme*
GetNonDefaultNativeTheme(const Widget* widget);
On 2015/11/11 00:34:46, Peter Kasting wrote:
> Can this return the platform default native theme in cases where there's no
> override?  If so, we can just name this GetNativeTheme() and maybe always use
it
> from Widget?

I don't know how we'd do this because ViewsDelegate::GetInstance() can
apparently be null (also, adding implementation to the base ViewsDelegate seems
bad, and adding it to ChromeViewsDelegate is impossible because it needs access
to internal apis).

> 
> If not, maybe GetNativeThemeOverride() would be slightly better, since
> "non-default" implies it's returning some sort of secondary theme so that
> callers can use both, or something strange.

I'm fine with the Override name.

[Peter Kasting, 2015-11-11 01:25:48]

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_aura_dark.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura_dark.h:15: class NATIVE_THEME_EXPORT
NativeThemeAuraDark : public NativeThemeAura {
On 2015/11/11 01:05:32, Evan Stade wrote:
> On 2015/11/11 00:34:46, Peter Kasting wrote:
> > I'm a bit concerned that on Windows we're proposing to use NativeThemeWin
> > normally and NativeThemeAuraDark for incognito, since the inheritance chains
> > are:
> > 
> > NativeTheme -> NativeThemeWin
> > NativeTheme -> NativeThemeBase -> NativeThemeAura -> NativeThemeAuraDark
> > 
> > I worry about what sorts of differences will result from this besides just a
> few
> > colors.  It would be more reassuring if the new "dark" theme was a subclass
or
> > direct sibling to the "normal" theme class in all cases.
> > 
> > It's making me wonder whether we're barking up the wrong tree; perhaps
instead
> > of a different NativeTheme class, we should be using some sort of mixin
object
> > to get colors and icons that can differ between normal and incognito mode,
> > either in the ultimate consumer, or inside the NativeThemeXXX implementation
> > itself?  Or maybe we should actually put "return incognito ? x : y" sorts of
> > statements directly into the functions in NativeThemeWin or the like?
> 
> Well, incognito ? x : y doesn't sound that viable because all the native theme
> instances are singletons,

It'd mean we'd have to pass in relevant state.  That may be enough to sink the
idea.

> How about this:
> 
> class NativeThemeAuraDark : public NativeThemeAura {...}
> class NativeThemeWinDark : public NativeThemeWin {
>   SkColor GetColor(id) { return NativeThemeAuraDark::GetColor(id); }
> }

That sort of route seems preferable.  I don't know that we could be quite this
simple -- we might need to do something like:

  SkColor NativeThemeAuraDark::GetColor(int id, bool* specifies_color) {
    *specifies_color = true;
    switch (id) {
      ...
      default:
        *specifies_color = false;
        return;
    }
  }

  SkColor NativeThemeAuraDark::GetColor(int id) {
    bool specifies_color;
    SkColor color = GetColor(id, &specifies_color);
    return specifies_color ? color : NativeThemeAura::GetColor(id);
  }

  SkColor NativeThemeWinDark::GetColor(int id) {
    bool specifies_color;
    SkColor color = GetColor(id, &specifies_color);
    return specifies_color ? color : NativeThemeWin::GetColor(id);
  }

Whether we need this depends on whether the dark theme has to replace literally
every color or not.  My suspicion is that it doesn't, and some things (link
color?) might be left "as-is", in which case we want to make sure Windows uses
the NativeThemeWin behaviors instead of the NativeThemeAura ones.

> Eventually if we find colors that shouldn't be the same on Windows incognito
we
> can add those before the NativeThemeAuraDark redirect.

Yeah, that'd be easy.

https://codereview.chromium.org/1438513003/diff/60001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/views/views_delegate...
ui/views/views_delegate.h:197: virtual ui::NativeTheme*
GetNonDefaultNativeTheme(const Widget* widget);
On 2015/11/11 01:05:32, Evan Stade wrote:
> On 2015/11/11 00:34:46, Peter Kasting wrote:
> > Can this return the platform default native theme in cases where there's no
> > override?  If so, we can just name this GetNativeTheme() and maybe always
use
> it
> > from Widget?
> 
> I don't know how we'd do this because ViewsDelegate::GetInstance() can
> apparently be null

I meant for Widget to so something like:

  return views_delegate ? views_delegate->GetNativeTheme() : GetNativeTheme();

So we'd still need to null-check, it's just that we wouldn't need to also check
the return value from this.

> (also, adding implementation to the base ViewsDelegate seems
> bad, and adding it to ChromeViewsDelegate is impossible because it needs
access
> to internal apis).

If it's technically infeasible, OK.  I wasn't sure.

[Evan Stade, 2015-11-11 01:32:30]

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_aura_dark.h (right):

https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_aura_dark.h:15: class NATIVE_THEME_EXPORT
NativeThemeAuraDark : public NativeThemeAura {
> > Eventually if we find colors that shouldn't be the same on Windows incognito
> we
> > can add those before the NativeThemeAuraDark redirect.
> 
> Yeah, that'd be easy.

So are you happy starting off with this plan?

FWIW, link color is incognito specific. I think pretty much 100% of colors will
be incognito specific because once you start changing backgrounds, everything
else has to change too.

[Peter Kasting, 2015-11-11 01:36:41]

On 2015/11/11 01:32:30, Evan Stade wrote:
>
https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
> File ui/native_theme/native_theme_aura_dark.h (right):
> 
>
https://codereview.chromium.org/1438513003/diff/60001/ui/native_theme/native_...
> ui/native_theme/native_theme_aura_dark.h:15: class NATIVE_THEME_EXPORT
> NativeThemeAuraDark : public NativeThemeAura {
> > > Eventually if we find colors that shouldn't be the same on Windows
incognito
> > we
> > > can add those before the NativeThemeAuraDark redirect.
> > 
> > Yeah, that'd be easy.
> 
> So are you happy starting off with this plan?

Yeah, that seems OK.

I don't have much opinion on whether the views delegate is the right place to
hang the override theme.  Scott might have an opinion.

> FWIW, link color is incognito specific. I think pretty much 100% of colors
will
> be incognito specific because once you start changing backgrounds, everything
> else has to change too.

That may well be, in which case we won't need the sort of complexity I describe.

[Evan Stade, 2015-11-11 01:38:22]

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

[Evan Stade, 2015-11-11 01:38:23]

+sky, this CL is not ready for full review, but what do you think of changes to
widget.cc and [chrome_]views_delegate?

[sky, 2015-11-11 18:59:35]

The other option is to specify the NativeTheme in Widget::InitParams. That seems
to give quite a bit of flexibility. Would that work for you?

[Evan Stade, 2015-11-11 19:24:39]

It might work. Two issues:

1. It seems like it would be a lot of work to track down everywhere a
widget is inited and update them (and then expect everyone to do the same
in the future). Also I think it's basically always "copy the parent's
native theme" so it seems strange to repeat that everywhere.
2. We support live changes to native themes (for Linux) so we'd also need
some setter method on widget.

If I moved the transient window manager code into base I could probably
move the new function back to themeprovider (themeservice) if that's in
some way preferable to viewsdelegate.

On Wed, Nov 11, 2015, 10:59 AM  <sky@chromium.org> wrote:

> The other option is to specify the NativeTheme in Widget::InitParams. That
> seems
> to give quite a bit of flexibility. Would that work for you?
>
> https://codereview.chromium.org/1438513003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-11-11 20:35:06]

Fair enough. Add the override you have on the delegate.

  -Scott

On Wed, Nov 11, 2015 at 11:24 AM, Evan Stade <estade@chromium.org> wrote:
> It might work. Two issues:
>
> 1. It seems like it would be a lot of work to track down everywhere a widget
> is inited and update them (and then expect everyone to do the same in the
> future). Also I think it's basically always "copy the parent's native theme"
> so it seems strange to repeat that everywhere.
> 2. We support live changes to native themes (for Linux) so we'd also need
> some setter method on widget.
>
> If I moved the transient window manager code into base I could probably move
> the new function back to themeprovider (themeservice) if that's in some way
> preferable to viewsdelegate.
>
>
> On Wed, Nov 11, 2015, 10:59 AM  <sky@chromium.org> wrote:
>>
>> The other option is to specify the NativeTheme in Widget::InitParams. That
>> seems
>> to give quite a bit of flexibility. Would that work for you?
>>
>> https://codereview.chromium.org/1438513003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2015-11-12 17:44:19]

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

[commit-bot: I haz the power, 2015-11-12 17:44:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438513003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438513003/100001

[Evan Stade, 2015-11-12 17:46:36]

Description was changed from

==========
[MD] Implement incognito colors as a NativeTheme (for Aura).

For discussion only, not for commit.

BUG=501377
==========

to

==========
[MD] Implement incognito colors as a NativeTheme

For ChromeOS (and at least for now, Linux), this is NativeThemeAuraDark. For
Windows, this is NativeThemeWinDark, which paints controls like NativeThemeWin
but uses NativeThemeAuraDark for colors.

This CL focuses on the location bar and omnibox. It adds a number of colors
(most of those which have already been spec'd) but is missing lots more colors.

BUG=501377
==========

[Evan Stade, 2015-11-12 17:49:08]

This CL is now ready for review.

Peter, PTAL everything under chrome/.
Scott, PTAL everything under ui/.

I still haven't verified this doesn't crash and burn on Windows. Roger: assuming
the trybots come back green, can you make sure it works at least reasonably well
on Windows?

[Evan Stade, 2015-11-12 17:50:27]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2015-11-12 17:50:34]

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

[commit-bot: I haz the power, 2015-11-12 17:51:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438513003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438513003/120001

[commit-bot: I haz the power, 2015-11-12 18:05:08]

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

[commit-bot: I haz the power, 2015-11-12 18:05:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sky, 2015-11-12 21:58:29]

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD.gn
File ui/native_theme/BUILD.gn (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD....
ui/native_theme/BUILD.gn:45: "native_theme_win_dark.cc",
Generally platform specific files end with the platform, eg
native_theme_dark_win. Is there a reason not to do that here?

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/native...
File ui/native_theme/native_theme_aura_dark.h (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/native...
ui/native_theme/native_theme_aura_dark.h:5: #ifndef
UI_NATIVE_THEME_NATIVE_THEME_AURA_OFF_THE_RECORD_H_
Guards are off.

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/native...
ui/native_theme/native_theme_aura_dark.h:17: static NativeThemeAuraDark*
instance();
Should the renderer pick this up too if appropriate?

https://codereview.chromium.org/1438513003/diff/140001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/views/widget/widget...
ui/views/widget/widget.cc:771:
ViewsDelegate::GetInstance()->GetNativeThemeOverride(this);
Can we cache this rather than querying the delegate every time?

[Evan Stade, 2015-11-12 22:11:11]

(no new patch uploaded yet)

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD.gn
File ui/native_theme/BUILD.gn (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD....
ui/native_theme/BUILD.gn:45: "native_theme_win_dark.cc",
On 2015/11/12 21:58:28, sky wrote:
> Generally platform specific files end with the platform, eg
> native_theme_dark_win. Is there a reason not to do that here?

Here is the inheritance chain:

NativeTheme : NativeThemeWin : NativeThemeWinDark

I wanted to add additional specializations on to the end rather than putting
them in the middle. But otherwise no, there's no particular reason
NativeThemeDarkWin wouldn't work.

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/native...
File ui/native_theme/native_theme_aura_dark.h (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/native...
ui/native_theme/native_theme_aura_dark.h:5: #ifndef
UI_NATIVE_THEME_NATIVE_THEME_AURA_OFF_THE_RECORD_H_
On 2015/11/12 21:58:28, sky wrote:
> Guards are off.

Oops. I guess that presubmit check isn't turned on for this directory?

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/native...
ui/native_theme/native_theme_aura_dark.h:17: static NativeThemeAuraDark*
instance();
On 2015/11/12 21:58:29, sky wrote:
> Should the renderer pick this up too if appropriate?

I think probably not, but not entirely sure at the moment. I doubt we want the
content area to look different than normal.

https://codereview.chromium.org/1438513003/diff/140001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/views/widget/widget...
ui/views/widget/widget.cc:771:
ViewsDelegate::GetInstance()->GetNativeThemeOverride(this);
On 2015/11/12 21:58:29, sky wrote:
> Can we cache this rather than querying the delegate every time?

I suppose we could as long as we call NativeTheme::NotifyObservers whenever the
theme changes.

[sky, 2015-11-12 23:45:52]

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD.gn
File ui/native_theme/BUILD.gn (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD....
ui/native_theme/BUILD.gn:45: "native_theme_win_dark.cc",
On 2015/11/12 22:11:11, Evan Stade wrote:
> On 2015/11/12 21:58:28, sky wrote:
> > Generally platform specific files end with the platform, eg
> > native_theme_dark_win. Is there a reason not to do that here?
> 
> Here is the inheritance chain:
> 
> NativeTheme : NativeThemeWin : NativeThemeWinDark
> 
> I wanted to add additional specializations on to the end rather than putting
> them in the middle. But otherwise no, there's no particular reason
> NativeThemeDarkWin wouldn't work.

I'm not suggesting moving any code, just the name. And I get what you're saying,
but I think Win is the end is generally how we name things like this and works
better with build stuff.

[Evan Stade, 2015-11-13 18:11:40]

I talked to Sebastien some more this morning, he clarified that the different
colors should only apply to the top chrome (including omnibox popup), but not
bubbles, dialogs, menus, etc. Thus we can make this CL a bit simpler. No change
to ViewsDelegate; just make Widget::GetNativeTheme() virtual and override it for
the two relevant widgets.

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD.gn
File ui/native_theme/BUILD.gn (right):

https://codereview.chromium.org/1438513003/diff/140001/ui/native_theme/BUILD....
ui/native_theme/BUILD.gn:45: "native_theme_win_dark.cc",
On 2015/11/12 23:45:52, sky wrote:
> On 2015/11/12 22:11:11, Evan Stade wrote:
> > On 2015/11/12 21:58:28, sky wrote:
> > > Generally platform specific files end with the platform, eg
> > > native_theme_dark_win. Is there a reason not to do that here?
> > 
> > Here is the inheritance chain:
> > 
> > NativeTheme : NativeThemeWin : NativeThemeWinDark
> > 
> > I wanted to add additional specializations on to the end rather than putting
> > them in the middle. But otherwise no, there's no particular reason
> > NativeThemeDarkWin wouldn't work.
> 
> I'm not suggesting moving any code, just the name. And I get what you're
saying,
> but I think Win is the end is generally how we name things like this and works
> better with build stuff.

changed the name

[Peter Kasting, 2015-11-14 02:46:08]

LGTM

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:334: :
SkColorSetRGB(7, 149, 0);
I think you need to call GetReadableColor(..., GetColor(BACKGROUND)) on the
overall result here.

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:359: ? inverted ?
GetColor(TEXT) : gfx::kGoogleRed700
I like ?:s, but please avoid nesting them...

    case SecurityStateModel::SECURITY_ERROR: {
      const bool md = ui::MaterialDesignController::IsModeMaterial();
      if (md && inverted)
        return GetColor(TEXT);
      return color_utils::GetReadableColor(
          md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0),
          GetColor(BACKGROUND));
    }

If you write things this way, you can also kill the |color| variable, and nuke
the default state and change the function end to the contents of the current
default state (since then all switch arms will return directly).

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:44:
views::Widget* popup_parent_;
Nit: Just call this |parent_| since "popup" is already in the class name
"AutocompletePopupWidget".

[Evan Stade, 2015-11-16 16:09:34]

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

[Evan Stade, 2015-11-16 16:09:35]

Scott appears to be busy, so +sadrul could you take a look at //ui/

[sky, 2015-11-16 16:10:17]

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_frame.cc:170: const ui::NativeTheme*
BrowserFrame::GetNativeTheme() const {
You sure you don't want to specify the NativeTheme as an init param? Less
subclassing.

[sky, 2015-11-16 16:10:49]

Or just behind.

On Mon, Nov 16, 2015 at 8:09 AM,  <estade@chromium.org> wrote:
> Scott appears to be busy, so +sadrul could you take a look at //ui/
>
> https://codereview.chromium.org/1438513003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2015-11-16 17:52:25]

> Or just behind.

thanks for taking a look, I wasn't sure what the "busy" block on your calendar
meant.

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_frame.cc:170: const ui::NativeTheme*
BrowserFrame::GetNativeTheme() const {
On 2015/11/16 16:10:17, sky wrote:
> You sure you don't want to specify the NativeTheme as an init param? Less
> subclassing.

Isn't it the same amount of subclassing? These two widgets already are
subclasses. But if you prefer it as an init param I can change it (my previous
arguments against it are no longer relevant).

[sky, 2015-11-16 18:06:23]

I prefer the init param, seems more flexible for other cases that
don't necessarily subclass.

  -Scott

On Mon, Nov 16, 2015 at 9:52 AM,  <estade@chromium.org> wrote:
>> Or just behind.
>
>
> thanks for taking a look, I wasn't sure what the "busy" block on your
> calendar
> meant.
>
>
>
https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
> File chrome/browser/ui/views/frame/browser_frame.cc (right):
>
>
https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
> chrome/browser/ui/views/frame/browser_frame.cc:170: const
> ui::NativeTheme* BrowserFrame::GetNativeTheme() const {
> On 2015/11/16 16:10:17, sky wrote:
>>
>> You sure you don't want to specify the NativeTheme as an init param?
>
> Less
>>
>> subclassing.
>
>
> Isn't it the same amount of subclassing? These two widgets already are
> subclasses. But if you prefer it as an init param I can change it (my
> previous arguments against it are no longer relevant).
>
> https://codereview.chromium.org/1438513003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2015-11-16 19:36:33]

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:334: :
SkColorSetRGB(7, 149, 0);
On 2015/11/14 02:46:08, Peter Kasting wrote:
> I think you need to call GetReadableColor(..., GetColor(BACKGROUND)) on the
> overall result here.

This color is not used for text on top of the location bar BG, except in the
SECURE text case, so I updated that below.

- in pre-MD, the bubble background is hardcoded to a light green 9-box, so no
need to adjust this color
- in MD, this green is also hard-coded. When the location bar BG is dark, the
bubble appearance changes a bit but the chip will always have a light BG.

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:359: ? inverted ?
GetColor(TEXT) : gfx::kGoogleRed700
On 2015/11/14 02:46:08, Peter Kasting wrote:
> I like ?:s, but please avoid nesting them...
> 
>     case SecurityStateModel::SECURITY_ERROR: {
>       const bool md = ui::MaterialDesignController::IsModeMaterial();
>       if (md && inverted)
>         return GetColor(TEXT);
>       return color_utils::GetReadableColor(
>           md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0),
>           GetColor(BACKGROUND));
>     }
> 
> If you write things this way, you can also kill the |color| variable, and nuke
> the default state and change the function end to the contents of the current
> default state (since then all switch arms will return directly).

rewrote this, but there are still two cases which use the color_utils fn

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1438513003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:44:
views::Widget* popup_parent_;
On 2015/11/14 02:46:08, Peter Kasting wrote:
> Nit: Just call this |parent_| since "popup" is already in the class name
> "AutocompletePopupWidget".

this change reverted

[Evan Stade, 2015-11-16 19:37:27]

(also switched to init param)

[Peter Kasting, 2015-11-16 21:28:04]

LGTM

[sky, 2015-11-16 21:43:47]

LGTM

[Evan Stade, 2015-11-16 22:59:04]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-11-16 22:59:47]

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

[commit-bot: I haz the power, 2015-11-16 23:07:05]

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

[commit-bot: I haz the power, 2015-11-16 23:07:06]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2015-11-16 23:44:08]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-11-16 23:44:26]

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

[commit-bot: I haz the power, 2015-11-17 00:26:58]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-17 00:27:54]

Patchset 12 (id:??) landed as
https://crrev.com/563454636cfe3df6a57fff84560e8a89e4f6b175
Cr-Commit-Position: refs/heads/master@{#359956}

[Evan Stade, 2015-11-17 00:47:56]

On 2015/11/17 00:27:54, commit-bot: I haz the power wrote:
> Patchset 12 (id:??) landed as
> https://crrev.com/563454636cfe3df6a57fff84560e8a89e4f6b175
> Cr-Commit-Position: refs/heads/master@{#359956}

just noticed this patch broke menu theming, on MD and pre-MD. Working on a fix.

[Evan Stade, 2015-11-17 02:23:29]

Patchset #13 (id:240001) has been deleted