use the right NativeTheme on bubbles + dialogs on Aura Linux

use the right NativeTheme on bubbles + dialogs on Aura Linux

TODOs:
1. update native theme when user changes setting
2. fix various bugs like tooltip text color, bubble button color
3. decide if we should ever use aura theme for menus and tooltips

BUG=347832
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266047

[Ben Goodger (Google), 2014-04-21 22:13:19]

I'm gonna let erg review this first.

[Elliot Glaysher, 2014-04-21 22:43:19]

lgtm. Thank you for doing all of this.

https://codereview.chromium.org/243633003/diff/20001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/243633003/diff/20001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:461:
window->GetNativeWindowProperty(Profile::kProfileKey));
I sort of worry about destruction order issues here, but IIRC, the Profile
outlives all browser windows, so this shouldn't be a problem, I guess?

https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_scroll_view_container.cc (right):

https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_scroll_view_container.cc:284: #if
!(defined(OS_LINUX) && !defined(OS_CHROMEOS))
If you're bracing these with #if checks, you probably also want to take care of
the one in menu_config_views.cc?

[Evan Stade, 2014-04-22 18:33:22]

https://codereview.chromium.org/243633003/diff/20001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/243633003/diff/20001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:461:
window->GetNativeWindowProperty(Profile::kProfileKey));
On 2014/04/21 22:43:20, Elliot Glaysher wrote:
> I sort of worry about destruction order issues here, but IIRC, the Profile
> outlives all browser windows, so this shouldn't be a problem, I guess?

I believe this to be correct.

https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_scroll_view_container.cc (right):

https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_scroll_view_container.cc:284: #if
!(defined(OS_LINUX) && !defined(OS_CHROMEOS))
On 2014/04/21 22:43:20, Elliot Glaysher wrote:
> If you're bracing these with #if checks, you probably also want to take care
of
> the one in menu_config_views.cc?

No, I think menu_config_views.cc is OK as-is. I just blocked this out because
there's actually no shadow on linux (at least on my setup), so not having a
border looks pretty weird.

[Elliot Glaysher, 2014-04-22 19:48:26]

https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_scroll_view_container.cc (right):

https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_scroll_view_container.cc:284: #if
!(defined(OS_LINUX) && !defined(OS_CHROMEOS))
On 2014/04/22 18:33:22, Evan Stade wrote:
> On 2014/04/21 22:43:20, Elliot Glaysher wrote:
> > If you're bracing these with #if checks, you probably also want to take care
> of
> > the one in menu_config_views.cc?
> 
> No, I think menu_config_views.cc is OK as-is. I just blocked this out because
> there's actually no shadow on linux (at least on my setup), so not having a
> border looks pretty weird.

I can see us wanting to use the aura colors. However, I don't think we want to
use the extremely tall chromeos menus. We make an exception on Windows, and I
think we should do that here.

[Evan Stade, 2014-04-22 19:55:15]

On 2014/04/22 19:48:26, Elliot Glaysher wrote:
>
https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
> File ui/views/controls/menu/menu_scroll_view_container.cc (right):
> 
>
https://codereview.chromium.org/243633003/diff/20001/ui/views/controls/menu/m...
> ui/views/controls/menu/menu_scroll_view_container.cc:284: #if
> !(defined(OS_LINUX) && !defined(OS_CHROMEOS))
> On 2014/04/22 18:33:22, Evan Stade wrote:
> > On 2014/04/21 22:43:20, Elliot Glaysher wrote:
> > > If you're bracing these with #if checks, you probably also want to take
care
> > of
> > > the one in menu_config_views.cc?
> > 
> > No, I think menu_config_views.cc is OK as-is. I just blocked this out
because
> > there's actually no shadow on linux (at least on my setup), so not having a
> > border looks pretty weird.
> 
> I can see us wanting to use the aura colors. However, I don't think we want to
> use the extremely tall chromeos menus. We make an exception on Windows, and I
> think we should do that here.

I asked a similar question of our designers in crbug.com/365418

Windows does actually use the aura values when using NativeThemeAura.

[Elliot Glaysher, 2014-04-22 20:02:49]

On 2014/04/22 19:55:15, Evan Stade wrote:
> > On 2014/04/21 22:43:20, Elliot Glaysher wrote:
> > I can see us wanting to use the aura colors. However, I don't think we want
to
> > use the extremely tall chromeos menus. We make an exception on Windows, and
I
> > think we should do that here.
> 
> I asked a similar question of our designers in crbug.com/365418
> 
> Windows does actually use the aura values when using NativeThemeAura.

When does Windows use NativeThemeAura on the desktop? It looks like
DWTHW::GetNativeTheme() always passes back a NativeThemeWin instance on the
desktop.

[Evan Stade, 2014-04-22 20:53:39]

On 2014/04/22 20:02:49, Elliot Glaysher wrote:
> On 2014/04/22 19:55:15, Evan Stade wrote:
> > > On 2014/04/21 22:43:20, Elliot Glaysher wrote:
> > > I can see us wanting to use the aura colors. However, I don't think we
want
> to
> > > use the extremely tall chromeos menus. We make an exception on Windows,
and
> I
> > > think we should do that here.
> > 
> > I asked a similar question of our designers in crbug.com/365418
> > 
> > Windows does actually use the aura values when using NativeThemeAura.
> 
> When does Windows use NativeThemeAura on the desktop? It looks like
> DWTHW::GetNativeTheme() always passes back a NativeThemeWin instance on the
> desktop.

dunno, maybe never? But menu_config_win.cc has this:

void MenuConfig::Init(const NativeTheme* theme) {
  if (theme == ui::NativeThemeAura::instance()) {
    InitAura(theme);
    return;
  }

  [...]
}

So it somewhat follows that Linux should do the same as long as it's using
NativeThemeAura. Whether it should ever use NativeThemeAura for menus is another
matter (see TODO in cl desc.).

[Evan Stade, 2014-04-23 22:38:29]

had to rejigger some things because libgtk2ui can't use Profile (can't depend on
browser or a circular dependency is created). PTAL

[Elliot Glaysher, 2014-04-23 22:49:25]

On 2014/04/23 22:38:29, Evan Stade wrote:
> had to rejigger some things because libgtk2ui can't use Profile (can't depend
on
> browser or a circular dependency is created). PTAL

lgtm

(I am of the opinion that NativeThemeAura is misnamed, should really be
NativeThemeAsh, and should never be applied to menus. I'm fine with using
CommonTheme colors in chrome theme mode, but don't want the aura sizing
decisions to be done since AFAIK, those are never done on the desktop.)

[Evan Stade, 2014-04-23 23:13:30]

Ben PTAL

[Ben Goodger (Google), 2014-04-24 15:48:49]

lgtm

[Evan Stade, 2014-04-24 16:24:21]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-04-24 16:25:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/243633003/80001

[commit-bot: I haz the power, 2014-04-24 17:17:21]

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

[commit-bot: I haz the power, 2014-04-24 17:17:21]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[Evan Stade, 2014-04-24 20:18:39]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2014-04-24 21:07:11]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2014-04-24 21:07:11]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:11:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/243633003/80001

[commit-bot: I haz the power, 2014-04-24 23:42:14]

Change committed as 266047