Remove title and icon from chrome apps native style title bars.

Remove title and icon from chrome apps native style title bars.

These windows aren't meant to have titles or icons, but still need to
get a proper title as far as the OS is concerned. This means using a
custom but glass frame, similarly to how unthemed browser windows look.

BUG=339558
TEST=Try out frame:chrome app windows windows on Windows 7, 8 and XP.
Use the windows in various states (maximised, fullscreen, normal).
Make sure they work after switching in and out of aero mode.

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

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

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

[benwells, 2014-04-03 07:37:02]

tapted for apps specific stuff (and anything else you wanna look at)
sadrul for the crazy native widget / tree host stuff. Please reassign if there
is a better reviewer for the windows stuff.

[sadrul, 2014-04-03 17:10:55]

+sky@

On 2014/04/03 07:37:02, benwells wrote:
> tapted for apps specific stuff (and anything else you wanna look at)
> sadrul for the crazy native widget / tree host stuff. Please reassign if there
> is a better reviewer for the windows stuff.

I looked at the code, but the Windows code includes things I am not familiar
with. So redirecting to sky@

[tapted, 2014-04-03 19:40:24]

sorry I am not awesomely familiar with the Windows APIs either, so maybe some
silly questions here.

https://codereview.chromium.org/213743017/diff/20001/apps/ui/views/app_window...
File apps/ui/views/app_window_frame_view.h (right):

https://codereview.chromium.org/213743017/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.h:51: AppWindowFrameView(views::Widget*
widget,
I think the separate Init(..) is the prevailing style in views. For example,
calling AddChildView in the constructor triggers some virtual methods (e.g.
VeiewHierarchyChanged) which could miss calling an override if someone inherits
from AppWindowFrameView.

https://codereview.chromium.org/213743017/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.h:60: int resize_inside_bounds_size() {
nit: const method?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.cc
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.cc:27:
app_window_desktop_window_tree_host_;
could the data member be dropped, and just assign `new AppW...(..)` here?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc:9:
#include "base/debug/stack_trace.h"
stray debugging header

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h:16: //
AppWindowDesktopWindowTreeHostWin handles update the glass of Windows
nit: `handles updates to the glass`?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:183: if
(ui::win::IsAeroGlassEnabled()) {
Does this need an IsNativeWindowInAsh check as well?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/glass_app_window_frame_view.cc (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:21:
"browser/ui/views/extensions/GlassAppWindowFrameView";
should this be ui/views/apps/..?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:30: // If 1 is not
subtraced, they are too big. There is possibly some reason
nit: subtraced -> subtracted

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:34: int frame_size =
GetSystemMetrics(SM_CXSIZEFRAME) - 1;
Can GetSystemMetrics ever fail/ return 0?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:61: return HTCLIENT;
This API isn't too familiar to me, so this might be a stupid question: Does this
work for a dual-screen setup?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:112:
min_size.Enlarge(0, client_bounds.y());
why just the y? (should x always be 0?)

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/glass_app_window_frame_view.h (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.h:8: #include
"ui/gfx/rect.h"
nit: I think ui/gfx/geometry/insets.h is the only header you technically need
(and NativeAppWindow / views::Widget the only forward decs you need), since iwyu
doesn't mandate including things to implement an interface from a parent class.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.h:30: class
GlassAppWindowFrameView : public views::NonClientFrameView {
close off namespace apps? (things in chrome/browser don't typically get put into
namespaces)

[sky, 2014-04-03 19:43:55]

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.h
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.h:39:
AppWindowDesktopWindowTreeHostWin* app_window_desktop_window_tree_host_;
Is there a reason you need as a member? AFAICT it's only used within
initnativewidget.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc:58: }
I'm surprised you don't need a default block here.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h:16: //
AppWindowDesktopWindowTreeHostWin handles update the glass of Windows
nit: fix 'handles update'. Maybe you meant 'handles updating'?

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h:35: void
UpdateDWMFrame();
Description?

[benwells, 2014-04-04 06:50:43]

https://codereview.chromium.org/213743017/diff/20001/apps/ui/views/app_window...
File apps/ui/views/app_window_frame_view.h (right):

https://codereview.chromium.org/213743017/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.h:51: AppWindowFrameView(views::Widget*
widget,
On 2014/04/03 19:40:24, tapted wrote:
> I think the separate Init(..) is the prevailing style in views. For example,
> calling AddChildView in the constructor triggers some virtual methods (e.g.
> VeiewHierarchyChanged) which could miss calling an override if someone
inherits
> from AppWindowFrameView.

Done.

https://codereview.chromium.org/213743017/diff/20001/apps/ui/views/app_window...
apps/ui/views/app_window_frame_view.h:60: int resize_inside_bounds_size() {
On 2014/04/03 19:40:24, tapted wrote:
> nit: const method?

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.cc
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.cc:27:
app_window_desktop_window_tree_host_;
On 2014/04/03 19:40:24, tapted wrote:
> could the data member be dropped, and just assign `new AppW...(..)` here?

Done. (great minds think alike...)

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.h
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.h:39:
AppWindowDesktopWindowTreeHostWin* app_window_desktop_window_tree_host_;
On 2014/04/03 19:43:56, sky wrote:
> Is there a reason you need as a member? AFAICT it's only used within
> initnativewidget.

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc:9:
#include "base/debug/stack_trace.h"
On 2014/04/03 19:40:24, tapted wrote:
> stray debugging header

So there is .. gone

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.cc:58: }
On 2014/04/03 19:43:56, sky wrote:
> I'm surprised you don't need a default block here.

I think it is good. The function in DesktopWindowTreeHostWin this overrides is
empty so normally this class does nothing. Also see
BrowserDesktopWindowTreeHostWin's implementation which doesn't have a default.

BTW I considered making this an if but a switch seemed slightly better for this
type of function.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h
(right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h:16: //
AppWindowDesktopWindowTreeHostWin handles update the glass of Windows
On 2014/04/03 19:43:56, sky wrote:
> nit: fix 'handles update'. Maybe you meant 'handles updating'?

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h:16: //
AppWindowDesktopWindowTreeHostWin handles update the glass of Windows
On 2014/04/03 19:40:24, tapted wrote:
> nit: `handles updates to the glass`?

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_window_desktop_window_tree_host_win.h:35: void
UpdateDWMFrame();
On 2014/04/03 19:43:56, sky wrote:
> Description?

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:183: if
(ui::win::IsAeroGlassEnabled()) {
On 2014/04/03 19:40:24, tapted wrote:
> Does this need an IsNativeWindowInAsh check as well?

This doesn't get called in that case. See
ChromeNativeAppWindowViews::CreateNonClientFrameView, which will end up creating
a ash::CustomFrameViewAsh instead of a standard app window frame view.

I've renamed this function to CreateStandardDesktopAppFrame so it is clearer.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/glass_app_window_frame_view.cc (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:21:
"browser/ui/views/extensions/GlassAppWindowFrameView";
On 2014/04/03 19:40:24, tapted wrote:
> should this be ui/views/apps/..?

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:30: // If 1 is not
subtraced, they are too big. There is possibly some reason
On 2014/04/03 19:40:24, tapted wrote:
> nit: subtraced -> subtracted

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:34: int frame_size =
GetSystemMetrics(SM_CXSIZEFRAME) - 1;
On 2014/04/03 19:40:24, tapted wrote:
> Can GetSystemMetrics ever fail/ return 0?

According to MSDN if it fails it will return 0. It does not mention why it would
fail, the only thing I can think of is if you give it a bad metric, but maybe
there are other cases (e.g. your system is totally screwed).

I now make sure the insets are min 1. I tested what happens in these cases and
it isn't great but there are no explosions. Not that the normal chrome frame
will also be pretty broken in these cases. (From testing, if the insets are 0
there are explosions as the view size at widget initialization is 0,0)

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:61: return HTCLIENT;
On 2014/04/03 19:40:24, tapted wrote:
> This API isn't too familiar to me, so this might be a stupid question: Does
this
> work for a dual-screen setup?

It should do. This function is only called if the mouse is over the window.

This is also what the glass browser frame does.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/glass_app_window_frame_view.h (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.h:8: #include
"ui/gfx/rect.h"
On 2014/04/03 19:40:24, tapted wrote:
> nit: I think ui/gfx/geometry/insets.h is the only header you technically need
> (and NativeAppWindow / views::Widget the only forward decs you need), since
iwyu
> doesn't mandate including things to implement an interface from a parent
class.

Done.

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.h:30: class
GlassAppWindowFrameView : public views::NonClientFrameView {
On 2014/04/03 19:40:24, tapted wrote:
> close off namespace apps? (things in chrome/browser don't typically get put
into
> namespaces)

Done.

https://codereview.chromium.org/213743017/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/213743017/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:24: #include
"chrome/browser/ui/web_applications/web_app_ui.h"
This was a rebase error...

[tapted, 2014-04-04 16:15:18]

lgtm

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/glass_app_window_frame_view.cc (right):

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:34: int frame_size =
GetSystemMetrics(SM_CXSIZEFRAME) - 1;
On 2014/04/04 06:50:43, benwells wrote:
> On 2014/04/03 19:40:24, tapted wrote:
> > Can GetSystemMetrics ever fail/ return 0?
> 
> According to MSDN if it fails it will return 0. It does not mention why it
would
> fail, the only thing I can think of is if you give it a bad metric, but maybe
> there are other cases (e.g. your system is totally screwed).
> 
> I now make sure the insets are min 1. I tested what happens in these cases and
> it isn't great but there are no explosions. Not that the normal chrome frame
> will also be pretty broken in these cases. (From testing, if the insets are 0
> there are explosions as the view size at widget initialization is 0,0)

cool - wild conjecture: maybe this is the explanation for having to subtract 1
(otherwise the function could never return 0 without the possibility of someone
thinking it an error..)

https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:112:
min_size.Enlarge(0, client_bounds.y());
On 2014/04/03 19:40:24, tapted wrote:
> why just the y? (should x always be 0?)

there was one last question :). But I think I grok it.

[sky, 2014-04-04 17:09:00]

LGTM

[benwells, 2014-04-08 04:38:36]

On 2014/04/04 16:15:18, tapted wrote:
> lgtm
> 
>
https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/apps/glass_app_window_frame_view.cc (right):
> 
>
https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:34: int frame_size
=
> GetSystemMetrics(SM_CXSIZEFRAME) - 1;
> On 2014/04/04 06:50:43, benwells wrote:
> > On 2014/04/03 19:40:24, tapted wrote:
> > > Can GetSystemMetrics ever fail/ return 0?
> > 
> > According to MSDN if it fails it will return 0. It does not mention why it
> would
> > fail, the only thing I can think of is if you give it a bad metric, but
maybe
> > there are other cases (e.g. your system is totally screwed).
> > 
> > I now make sure the insets are min 1. I tested what happens in these cases
and
> > it isn't great but there are no explosions. Not that the normal chrome frame
> > will also be pretty broken in these cases. (From testing, if the insets are
0
> > there are explosions as the view size at widget initialization is 0,0)
> 
> cool - wild conjecture: maybe this is the explanation for having to subtract 1
> (otherwise the function could never return 0 without the possibility of
someone
> thinking it an error..)
> 
>
https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:112:
> min_size.Enlarge(0, client_bounds.y());
> On 2014/04/03 19:40:24, tapted wrote:
> > why just the y? (should x always be 0?)
> 
> there was one last question :). But I think I grok it.

I missed that. It actually looks like a bug to me...

[benwells, 2014-04-08 19:21:50]

On 2014/04/08 04:38:36, benwells wrote:
> On 2014/04/04 16:15:18, tapted wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/apps/glass_app_window_frame_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:34: int
frame_size
> =
> > GetSystemMetrics(SM_CXSIZEFRAME) - 1;
> > On 2014/04/04 06:50:43, benwells wrote:
> > > On 2014/04/03 19:40:24, tapted wrote:
> > > > Can GetSystemMetrics ever fail/ return 0?
> > > 
> > > According to MSDN if it fails it will return 0. It does not mention why it
> > would
> > > fail, the only thing I can think of is if you give it a bad metric, but
> maybe
> > > there are other cases (e.g. your system is totally screwed).
> > > 
> > > I now make sure the insets are min 1. I tested what happens in these cases
> and
> > > it isn't great but there are no explosions. Not that the normal chrome
frame
> > > will also be pretty broken in these cases. (From testing, if the insets
are
> 0
> > > there are explosions as the view size at widget initialization is 0,0)
> > 
> > cool - wild conjecture: maybe this is the explanation for having to subtract
1
> > (otherwise the function could never return 0 without the possibility of
> someone
> > thinking it an error..)
> > 
> >
>
https://codereview.chromium.org/213743017/diff/20001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/apps/glass_app_window_frame_view.cc:112:
> > min_size.Enlarge(0, client_bounds.y());
> > On 2014/04/03 19:40:24, tapted wrote:
> > > why just the y? (should x always be 0?)
> > 
> > there was one last question :). But I think I grok it.
> 
> I missed that. It actually looks like a bug to me...

Hmm, can't really test this at the moment as I'm only on RDP. I'll land it as is
and check / fix it when back home.

[benwells, 2014-04-08 19:22:07]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-04-08 19:22:48]

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

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

Change committed as 262516

[Matt Giuca, 2014-04-10 05:53:49]

This change was reverted (https://codereview.chromium.org/229343004) because it
conflicted with another change I was planning to merge into M35. We will be
re-landing it after committing the other CL
(https://codereview.chromium.org/224903022) and resolving the conflicts.

[benwells, 2014-04-22 06:48:11]

+mgiuca for the linux resizing stuff that is now incorporated.

[Matt Giuca, 2014-04-23 05:02:10]

Merge with my Linux changes lgtm

[benwells, 2014-04-23 06:32:03]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-04-23 06:32:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/213743017/140001

[commit-bot: I haz the power, 2014-04-23 09:22:43]

Change committed as 265598

[benwells, 2014-04-24 01:11:05]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-04-24 01:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/213743017/160001

[commit-bot: I haz the power, 2014-04-24 03:08:18]

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

[commit-bot: I haz the power, 2014-04-24 03:08:19]

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

[benwells, 2014-04-24 03:13:45]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-04-24 03:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/213743017/160001

[commit-bot: I haz the power, 2014-04-24 05:18:20]

Change committed as 265860