Widget::ShouldUseNativeFrame is now meaningful on Linux.

Widget::ShouldUseNativeFrame is now meaningful on Linux.

This change is a big refactor to make this work without breaking
Windows. Previously, a lot of views code assumed that
ShouldUseNativeFrame meant an Aero Glass window (having the tab strip
transparently overlap a native frame), which is not true on Linux. Now,
another property, ShouldWindowContentsBeTransparent, is used to in the
Aero Glass case. On Windows, these two properties are the same, but on
Linux, the former can be true without the latter.

Now, in Linux Aura, browser windows run with --use-system-title-bar will
have ShouldUseNativeFrame return true, without any visual anomalies.
This has two purposes (both of which I will need soon):
1. It allows DesktopWindowTreeHostX11 to behave differently depending on
   whether the window has a native frame.
2. It will allow the window frame to be added or removed dynamically
   through Widget::set_frame_type.

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

[Matt Giuca, 2014-01-31 08:45:52]

Hi guys, some more background info for this CL. (It's one of those CLs that
seems kind of bad, but was in my opinion the least of a bad set of options. It
was a close call.) :(

I'm trying to make the "Use System Title Bar and Borders" preference dynamically
changeable on Linux. To do that, I need a way to tell DesktopWindowTreeHostX11
to change its border style from BrowserFrame. To send a message all that way, I
need to go through Widget (via DesktopNativeWidgetAura, which BrowserFrame
doesn't directly have access to), which means either adding a new
"SetNativeBorder" method to Widget, or using the FrameType stuff
(set_frame_type).

The problem I ran into is that ShouldUseNativeFrame always returns false on
Linux. Calling set_frame_type(FRAME_TYPE_FORCE_NATIVE) makes it return true,
which breaks the tab strip because it assumes Aero Glass. Hence this precursor
CL, making Linux not break if ShouldUseNativeFrame returns true. Hope that
explains it a bit.

[Elliot Glaysher, 2014-01-31 22:11:26]

I don't see a problem with this, but I defer to sky@.

[Matt Giuca, 2014-02-01 05:19:08]

Btw, OWNERS list is:

chrome/browser/ui/views/*: sky
ui/views/widget/desktop_aura/*: erg

> I don't see a problem with this, but I defer to sky@.

SGTM.

[Elliot Glaysher, 2014-02-03 19:16:11]

oh. lgtm then.

[sky, 2014-02-03 21:53:03]

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:248: bool UsingWindowsGlass(const
views::Widget* widget) const;
How about naming this ShouldUseGlassImages()? Also, make it static and have
TabStrip use this too.

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:76: static const int
kNativeFrameInactiveTabAlphaMultiSelection = 150;
Rename this to better match other naming.

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:533: const int
kNativeFrameOverlayAlpha = 178;
rename these too.

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:1342: #if defined(OS_WIN)
I would rather move the ifdef to a function with a descriptive name. Maybe
something like ShouldFadeUnselectedTabs().

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1164: transparent =
ShouldUseNativeFrame();
Add a method to DesktopRootWindow that determines this rather than ifdefs.

[Matt Giuca, 2014-02-05 23:25:30]

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:248: bool UsingWindowsGlass(const
views::Widget* widget) const;
On 2014/02/03 21:53:03, sky wrote:
> How about naming this ShouldUseGlassImages()?

I think this is different to the other methods above, because they dictate
whether a single particular thing should be shown. This function is really a
description of the environment, which is used in various ways (showing different
images, using different layout numbers, etc). In this case, it's more helpful to
name the function after what it tells you, than how you use it.

> Also, make it static and have TabStrip use this too.

Done. (Note that if you follow this function all the way down, it calls a
virtual method whose override on Windows calls another virtual method whose
override on Windows calls a static function (IsAeroGlassEnabled); I am cutting
out all those middle-men and just directly calling IsAeroGlassEnabled.)

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:76: static const int
kNativeFrameInactiveTabAlphaMultiSelection = 150;
On 2014/02/03 21:53:03, sky wrote:
> Rename this to better match other naming.

Done.

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:533: const int
kNativeFrameOverlayAlpha = 178;
On 2014/02/03 21:53:03, sky wrote:
> rename these too.

Done.

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:1342: #if defined(OS_WIN)
On 2014/02/03 21:53:03, sky wrote:
> I would rather move the ifdef to a function with a descriptive name. Maybe
> something like ShouldFadeUnselectedTabs().

Done.

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1164: transparent =
ShouldUseNativeFrame();
On 2014/02/03 21:53:03, sky wrote:
> Add a method to DesktopRootWindow that determines this rather than ifdefs.

Done.

[sky, 2014-02-06 00:36:14]

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:248: bool UsingWindowsGlass(const
views::Widget* widget) const;
On 2014/02/05 23:25:31, Matt Giuca wrote:
> On 2014/02/03 21:53:03, sky wrote:
> > How about naming this ShouldUseGlassImages()?
> 
> I think this is different to the other methods above, because they dictate
> whether a single particular thing should be shown. This function is really a
> description of the environment, which is used in various ways (showing
different
> images, using different layout numbers, etc). In this case, it's more helpful
to
> name the function after what it tells you, than how you use it.

Consider if we wanted to use this code on some other platform. In that case
UsingWindowsGlass isn't a good name. While technically UsingWindowsGlass is
correct now, the intention of this function is to decide where we use the
alternate set of images and layout. That's what I was hoping to capture with the
name ShouldUseGlassImages. Maybe ShouldUseGlassImagesAndLayout?

> > Also, make it static and have TabStrip use this too.
> 
> Done. (Note that if you follow this function all the way down, it calls a
> virtual method whose override on Windows calls another virtual method whose
> override on Windows calls a static function (IsAeroGlassEnabled); I am cutting
> out all those middle-men and just directly calling IsAeroGlassEnabled.)

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1164: transparent =
ShouldUseNativeFrame();
On 2014/02/05 23:25:31, Matt Giuca wrote:
> On 2014/02/03 21:53:03, sky wrote:
> > Add a method to DesktopRootWindow that determines this rather than ifdefs.
> 
> Done.

This isn't quite what I was asking for. I'm saying add another method to
DesktopRootWindowHost named ShouldContentWindowBeTransparent. This way the
decision is left to the DesktopRootWindowHost and there are no ifdefs in DNWA.
Alternatively move UpdateWindowTransparency to DesktopRootWindowHost.

[Matt Giuca, 2014-02-07 09:13:10]

Sorry, I had to refactor again. :( PTAL.

Hopefully this addresses all of your previous concerns or renders them moot.

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/136093007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.h:248: bool UsingWindowsGlass(const
views::Widget* widget) const;
Well it's sort of moot now.

I've had to do a big refactor because I realised I was making a bad assumption:
that DesktopWindowTreeHostWin was a leaf class (and therefore I could inline
it's implementation of ShouldUseNativeFrame which just calls
IsAeroGlassEnabled). It actually has a subclass,
BrowserDesktopWindowTreeHostWin, which returns false if you have a themed window
open, even if Glass is enabled.

So I largely have to revert to the original design of calling through Widget to
ask whether it is a Glass window.

What I have done is added a new method called ShouldWindowContentsBeTransparent
to both Widget and DesktopWindowTreeHost. The intention is that it is true if
the window contents overhang the title bar, and then Tab, TabStrip and Toolbar
use that information to determine whether to use the Glass style of images and
layout.

I think this is the best solution -- it's almost identical to the original code
but it cleanly separates the transparency from the nativity. Another solution I
thought of was to have ShouldWindowContentsBeTransparent in
DesktopWindowTreeHost (but not Widget), and then from these classes, simply use
(ShouldUseNativeFrame() && IsAeroGlassEnabled()) to determine whether to use the
glass theming. That seemed messier to me.

Is this OK?

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/136093007/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1164: transparent =
ShouldUseNativeFrame();
On 2014/02/06 00:36:14, sky wrote:
> On 2014/02/05 23:25:31, Matt Giuca wrote:
> > On 2014/02/03 21:53:03, sky wrote:
> > > Add a method to DesktopRootWindow that determines this rather than ifdefs.
> > 
> > Done.
> 
> This isn't quite what I was asking for. I'm saying add another method to
> DesktopRootWindowHost named ShouldContentWindowBeTransparent. This way the
> decision is left to the DesktopRootWindowHost and there are no ifdefs in DNWA.
> Alternatively move UpdateWindowTransparency to DesktopRootWindowHost.

Done.

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

https://codereview.chromium.org/136093007/diff/440001/ui/views/widget/widget....
ui/views/widget/widget.cc:874: return
native_widget_->ShouldWindowContentsBeTransparent();
This is the only potential behavioural change on Windows. Note that the code
path from Tab, TabStrip and Toolbar no longer goes through
Widget::ShouldUseNativeFrame, which means it doesn't honour frame_type_ (if it
is non-default). But that is never set on browser windows, to my knowledge, so I
believe this does not change the behaviour.

[sky, 2014-02-10 15:04:07]

https://codereview.chromium.org/136093007/diff/440001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/136093007/diff/440001/ui/views/widget/widget....
ui/views/widget/widget.h:596: bool ShouldWindowContentsBeTransparent() const;
This name isn't the greatest at the widget level, but I can't think of anything
that is really that much better.

[sky, 2014-02-10 15:04:49]

LGTM

[Matt Giuca, 2014-02-11 02:55:02]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-11 02:58:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/136093007/620001

[commit-bot: I haz the power, 2014-02-11 14:08:18]

Change committed as 250391