Linux Aura: Added --use-system-title-bar flag.

Linux Aura: Added --use-system-title-bar flag.

This flag makes browser windows use the system window dressing and title
bar, instead of the custom frame. This is a Linux Aura equivalent of the
"Use System Title Bar and Border" preference in Linux GTK. (Eventually,
this part will be controlled by the same preference, rather than this
flag.)

With this flag, browser windows use the native system window decoration,
with our custom close/minimize/maximize buttons removed, and the tab
strip being condensed vertically with square corners (like the maximized
tab strip).  Popup browser windows (including V1 apps and the Developer
Tools window) drop the blue custom frame altogether in favour of the
native frame.

BUG=317859
TEST=With --use-system-title-bar, open a browser window. Both maximized
and restored windows should have a system native title bar, and the tab
strip should be compacted into a single row (as when maximized).
TEST=With --use-system-title-bar, open a hosted app window. It should
have a system native title bar, with no second blue title bar.

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

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

[Matt Giuca, 2013-12-19 08:47:11]

Hi reviewers.

This CL goes most of the way to bringing back to the "Use System Title Bar and
Borders" functionality from Linux GTK into Linux Aura. (The part it is missing
is the fact that it is controlled by a preference, and is therefore synced and
dynamically changeable without restarting.)

It should not change any behaviour when the flag is off, but there is a risk
that it might, because of the change to DesktopRootWindowHostX11, which adds a
native border if remove_standard_frame is false (which it is by default!). I
have tried to set it to true everywhere when --use-system-title-bar is off, and
only turn it to false in the cases described in the CL description. I have
manually tested it thoroughly, and I haven't found any unusual windows getting
native frames.

Reviewer files:
benwells: apps/ui/views/*, chrome/browser/ui/views/apps/*
erg: ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc
jamescook: chrome/browser/ui/views/frame/*
sky: ui/views/widget/widget.h

(Any opinions on other files welcome too.) Thanks!

[benwells, 2013-12-19 19:30:12]

On 2013/12/19 08:47:11, Matt Giuca wrote:
> Hi reviewers.
> 
> This CL goes most of the way to bringing back to the "Use System Title Bar and
> Borders" functionality from Linux GTK into Linux Aura. (The part it is missing
> is the fact that it is controlled by a preference, and is therefore synced and
> dynamically changeable without restarting.)
> 
> It should not change any behaviour when the flag is off, but there is a risk
> that it might, because of the change to DesktopRootWindowHostX11, which adds a
> native border if remove_standard_frame is false (which it is by default!). I
> have tried to set it to true everywhere when --use-system-title-bar is off,
and
> only turn it to false in the cases described in the CL description. I have
> manually tested it thoroughly, and I haven't found any unusual windows getting
> native frames.
> 
> Reviewer files:
> benwells: apps/ui/views/*, chrome/browser/ui/views/apps/*
> erg: ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc
> jamescook: chrome/browser/ui/views/frame/*
> sky: ui/views/widget/widget.h
> 
> (Any opinions on other files welcome too.) Thanks!

Is your thinking that the native title bar flag for packaged apps would be
applied here? That is, if that flag is on they get the native frame, otherwise
they get the aura frame?

If so I think the code in the apps stuff might look quite different and we
should just go straight there.

[James Cook, 2013-12-19 20:57:54]

c/b/ui/frame is looking good. One question: I notice that the  task manager
(Shift-Esc) has a double-frame at the top, both with and without your patch.
Would that be easy to fix in this patch?

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1970: ,
optional nit: Maybe this could go somewhere above, or inside another USE_AURA
block?  It would get rid of this awkward comma and avoid merge conflicts with
the next guy to add something at the bottom of the list.

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:221:
(!delegate_->ShouldLeaveOffsetNearTopBorder() ||
Though not your fault, this conditional is too crazy. Can you pull it into some
if() statements or its own function?

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:529: bool
is_maximized = IsTitleBarCondensed();
nit: Maybe call this title_bar_condensed instead of is_maximized, since it no
longer exclusively means maximized.

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right):

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:98: // the
thick frame border and rounded corners.
Nice comment, very helpful to readers.

[Matt Giuca, 2013-12-19 23:01:25]

On 2013/12/19 19:30:12, benwells wrote:
> Is your thinking that the native title bar flag for packaged apps would be
> applied here? That is, if that flag is on they get the native frame, otherwise
> they get the aura frame?

That is the way things currently stand with this CL, yes.

> If so I think the code in the apps stuff might look quite different and we
> should just go straight there.

I don't really understand. I realise that having the true native frame may be
controversial which is one reason I put it behind a flag.

Originally, I had intended to separate out the browser window and apps windows
into separate CLs (and possibly separate flags), but I didn't for a couple of
reasons:
1. It turned out that the implementation was simpler to do them all at once. If
I was to separate this out, it would be a matter of adding explicit suppression
of native frames for apps with a comment that says // TODO: Turn this on later.
2. Part of the rationale for changing V2 apps to the blue style was that it was
consistent with the hosted app style. It therefore felt strange changing the
hosted app style without changing the V2 app style.

I decided that while we are experimenting with this flag, we may as well have
one flag to turn on all the native frames. Eventually, I would expect that we
make separate policy decisions for the browser window, V1 apps, and V2 apps --
for the browser we would make it controlled by the pref, and for the V1 and V2
apps we would decide to turn it on always, or remove it.

[Matt Giuca, 2013-12-20 03:53:21]

Thanks for the review. I actually did cause the double title issue for Task
Manager (it isn't present on master). Thanks for catching.

It's a bit hard to fix (requires CustomFrameView to not draw a frame). I'm
sending out another CL that also needs this functionality so I'll put that
through first, then return to this.

(I'll be OOO until Jan 3rd so this will have to wait until then.)

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1970: ,
Hmm, that sounds good but the order here actually matters (it's the order of the
flags on the about:flags page). I'm not really sure what the rules are (I'd kind
of prefer it to be in alphabetical order) but it seems like new flags are
usually added to the bottom, so I'd prefer to just do that than awkwardly adding
this flag underneath tab-capture-downscale-quality or something.

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:221:
(!delegate_->ShouldLeaveOffsetNearTopBorder() ||
On 2013/12/19 20:57:55, James Cook wrote:
> Though not your fault, this conditional is too crazy. Can you pull it into
some
> if() statements or its own function?

Done.

https://codereview.chromium.org/111723012/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:529: bool
is_maximized = IsTitleBarCondensed();
On 2013/12/19 20:57:55, James Cook wrote:
> nit: Maybe call this title_bar_condensed instead of is_maximized, since it no
> longer exclusively means maximized.

Done.

[benwells, 2013-12-20 04:26:43]

On 2013/12/19 23:01:25, Matt Giuca wrote:
> On 2013/12/19 19:30:12, benwells wrote:
> > Is your thinking that the native title bar flag for packaged apps would be
> > applied here? That is, if that flag is on they get the native frame,
otherwise
> > they get the aura frame?
> 
> That is the way things currently stand with this CL, yes.

I meant the other native frame flag
 The one we already have for other platforms. We should use that here too.

Sorry I'm about to get on a plane so am being brief. Cam explain more on Monday
/ when we're all back if it isn't clear.
> 
> > If so I think the code in the apps stuff might look quite different and we
> > should just go straight there.
> 
> I don't really understand. I realise that having the true native frame may be
> controversial which is one reason I put it behind a flag.
> 
> Originally, I had intended to separate out the browser window and apps windows
> into separate CLs (and possibly separate flags), but I didn't for a couple of
> reasons:
> 1. It turned out that the implementation was simpler to do them all at once.
If
> I was to separate this out, it would be a matter of adding explicit
suppression
> of native frames for apps with a comment that says // TODO: Turn this on
later.
> 2. Part of the rationale for changing V2 apps to the blue style was that it
was
> consistent with the hosted app style. It therefore felt strange changing the
> hosted app style without changing the V2 app style.
> 
> I decided that while we are experimenting with this flag, we may as well have
> one flag to turn on all the native frames. Eventually, I would expect that we
> make separate policy decisions for the browser window, V1 apps, and V2 apps --
> for the browser we would make it controlled by the pref, and for the V1 and V2
> apps we would decide to turn it on always, or remove it.

[Matt Giuca, 2013-12-20 05:21:39]

On 2013/12/20 04:26:43, benwells wrote:
> I meant the other native frame flag

Oh, --apps-use-native-frame. Yeah, we could. It would mean hosted apps are
blue-framed but V2 apps are native. But I don't see any good reason not to have
that for now.

We can discuss in two weeks. (I'm not here on Monday.)

[James Cook, 2013-12-20 22:23:51]

c/b/ui/views/frame LGTM

https://codereview.chromium.org/111723012/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/111723012/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:63: // When
the title bar is in its normal two row mode, the New Tab button isn't at
nit: You might want to put the word "restored" in this comment somewhere to give
the reader a clue that this constant is usually used with restored windows.

https://codereview.chromium.org/111723012/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:222: if
(!restored && delegate_->IsTabStripVisible() &&
better, thanks

[Matt Giuca, 2014-01-06 04:28:20]

https://codereview.chromium.org/111723012/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/111723012/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:63: // When
the title bar is in its normal two row mode, the New Tab button isn't at
On 2013/12/20 22:23:51, James Cook wrote:
> nit: You might want to put the word "restored" in this comment somewhere to
give
> the reader a clue that this constant is usually used with restored windows.

Done.

[Elliot Glaysher, 2014-01-06 21:24:13]

Patched it in locally. LGTM.

[Elliot Glaysher, 2014-01-14 19:16:02]

ping? The tentative plan is to release next week's dev channel to be aura. I'd
like to get at least something in by then.

[sky, 2014-01-14 20:31:28]

widget.h LGTM

[Matt Giuca, 2014-01-14 23:09:51]

Sorry, I had a chat with benwells last week, and he wasn't happy with using
ShellWindowFrameView for the native app window frame (since ShellWindowFrameView
is supposed to explicitly have the semantics "a non-native window frame"). So I
was planning to refactor this somewhat before submitting it.

Perhaps it would be good to remove the app window stuff from this patch so at
least we get the browser window changes in, and then look at the app window
stuff. Also, I am planning to have this behind a flag for the first release; is
that acceptable or do you actually want this to be controlled by the pref?

[Elliot Glaysher, 2014-01-14 23:41:28]

On 2014/01/14 23:09:51, Matt Giuca wrote:
> Perhaps it would be good to remove the app window stuff from this patch so at
> least we get the browser window changes in, and then look at the app window
> stuff. Also, I am planning to have this behind a flag for the first release;
is
> that acceptable or do you actually want this to be controlled by the pref?

The problem with having things behind a flag is that its not discoverable; I
don't know where people affected would find out about it. Actually controlling
this by a pref Shouldn't Be That Much Work[1], since that preference already
exists: prefs::UseCustomFramePref (found in chrome/common/pref_names.cc). The
logic that currently sets that prefernce is in c/b/u/gtk/browser_titlebar.cc, as
part of the menu model (also relevant: IDS_SHOW_WINDOW_DECORATIONS_MENU). This
then needs to be placed into the menu model; which I'm not entirely sure where
the code in views that handles this is, but I'd start looking at
c/b/u/v/frame/system_menu_model_builder.cc, particularly
SMMB::BuildSystemMenuForBrowserWindow(). 

(I don't actually care about anything but the main window, at least for this
first dev release. Obviously all windows should obey this preference eventually,
but the point is to credibly show that we're working on it.)

[1]: Famous Last Words. Offer may not be valid in all countries. Void where
prohibited.

[Matt Giuca, 2014-01-14 23:45:21]

Okay, I'll try to get the preference stuff for M34. Does it need to be done for
the first dev release? (Or rather, second dev release.)

To be honest, this is the first I've heard that this feature was a priority for
you. Up until now, I've been treating it as a side project with the vague idea
of having it done for M34.

[Elliot Glaysher, 2014-01-14 23:58:23]

It's entirely fine to dump this mostly as is to the tree today with the
command line flag; I think I can fix up the parts that are related to the
preferences fairly quickly if you'd rather work on something else.

(Sorry about the messaging here; I think the misunderstand here was because
I said that this feature was unimportant, not that it wasn't a priority. I
don't really care about this feature either way.)


On Tue, Jan 14, 2014 at 3:45 PM, <mgiuca@chromium.org> wrote:

> Okay, I'll try to get the preference stuff for M34. Does it need to be
> done for
> the first dev release? (Or rather, second dev release.)
>
> To be honest, this is the first I've heard that this feature was a
> priority for
> you. Up until now, I've been treating it as a side project with the vague
> idea
> of having it done for M34.
>
> https://codereview.chromium.org/111723012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[benwells, 2014-01-15 02:56:45]

On 2014/01/14 23:58:23, Elliot Glaysher wrote:
> It's entirely fine to dump this mostly as is to the tree today with the
> command line flag; I think I can fix up the parts that are related to the
> preferences fairly quickly if you'd rather work on something else.
> 
> (Sorry about the messaging here; I think the misunderstand here was because
> I said that this feature was unimportant, not that it wasn't a priority. I
> don't really care about this feature either way.)
> 
> 
> On Tue, Jan 14, 2014 at 3:45 PM, <mailto:mgiuca@chromium.org> wrote:
> 
> > Okay, I'll try to get the preference stuff for M34. Does it need to be
> > done for
> > the first dev release? (Or rather, second dev release.)
> >
> > To be honest, this is the first I've heard that this feature was a
> > priority for
> > you. Up until now, I've been treating it as a side project with the vague
> > idea
> > of having it done for M34.
> >
> > https://codereview.chromium.org/111723012/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I'd be happiest not having this affect shell windows for now. The code in there
to do with frames is pretty bad and this change imho makes it worse.

[Matt Giuca, 2014-01-16 08:18:36]

I have removed app windows from this CL, so now it just affects browser windows
(including V1 apps, popups and dev tools).

A problem I have been studying for awhile today is the Task manager dialog (and
perhaps other dialogs, though I have never found any other stand-alone
dialogs...). See Patch Set 14, in which I added a special case for dialogs on
Linux to have remove_standard_frame set to true. I don't like having to add a
special #if case like this, but it was the only easy way to make it work.
Unfortunately, remove_standard_frame isn't being set correctly on Windows (yet
it works anyway), but that isn't an option on Linux.

Basically, on Windows, the Task Manager dialog always has remove_standard_frame
set to false. This means the window ALWAYS has a native frame (see
http://crbug.com/334979 which I just filed) -- I believe the only reason it
doesn't appear to have a native frame on Windows is that the custom frame is
being drawn over the native one (but I could be wrong). In any case, we can't do
this on Linux. I could fix 334979 properly but it seems like a big refactor, so
for now, I opted to just fix it on Linux. (Note: I can't take that statement out
of the #if because on Windows it needs to be false in Glass mode.) Is this an
okay plan in the interim, or do I really have to address 334979 before
submitting this?

[Elliot Glaysher, 2014-01-16 19:15:59]

On 2014/01/16 08:18:36, Matt Giuca wrote:
> I have removed app windows from this CL, so now it just affects browser
windows
> (including V1 apps, popups and dev tools).
> 
> A problem I have been studying for awhile today is the Task manager dialog
(and
> perhaps other dialogs, though I have never found any other stand-alone
> dialogs...). See Patch Set 14, in which I added a special case for dialogs on
> Linux to have remove_standard_frame set to true. I don't like having to add a
> special #if case like this, but it was the only easy way to make it work.
> Unfortunately, remove_standard_frame isn't being set correctly on Windows (yet
> it works anyway), but that isn't an option on Linux.
> 
> Basically, on Windows, the Task Manager dialog always has
remove_standard_frame
> set to false. This means the window ALWAYS has a native frame (see
> http://crbug.com/334979 which I just filed) -- I believe the only reason it
> doesn't appear to have a native frame on Windows is that the custom frame is
> being drawn over the native one (but I could be wrong). In any case, we can't
do
> this on Linux. I could fix 334979 properly but it seems like a big refactor,
so
> for now, I opted to just fix it on Linux. (Note: I can't take that statement
out
> of the #if because on Windows it needs to be false in Glass mode.) Is this an
> okay plan in the interim, or do I really have to address 334979 before
> submitting this?

I'm fine with that plan. I don't actual expect this to be fixed outside the main
browser frame in the short term. I'm more worried about just having something to
show other people to soothe concerns.

[Matt Giuca, 2014-01-16 23:05:42]

Cool :)

Ben, are you okay with this now? (Technically I don't need your LGTM any more
since I've reverted the ShellWindowFrame changes, but I'll check to see if
you're happy since you've commented on this CL.)

[benwells, 2014-01-17 02:44:50]

native_app_window_views lgtm

[commit-bot: I haz the power, 2014-01-17 02:55:02]

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

[commit-bot: I haz the power, 2014-01-17 06:17:37]

Change committed as 245433

[Matt Giuca, 2014-01-20 00:57:58]

This had to be reverted (https://codereview.chromium.org/139303014/) because it
broke DesktopScreenX11Test (which unfortunately didn't run on the try server).

erg: Can you please review the (one-line) change to
ui/views/widget/desktop_aura/desktop_screen_x11_unittest.cc which fixes the
broken test. Thanks.

[Elliot Glaysher, 2014-01-21 18:26:06]

lgtm

[commit-bot: I haz the power, 2014-01-22 03:25:22]

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

[commit-bot: I haz the power, 2014-01-22 08:57:14]

Change committed as 246244