Linux: App windows with frame: chrome and no color are now native.

Linux: App windows with frame: chrome and no color are now native.

This is bringing Aura in line with the existing GTK behaviour. Windows
with a coloured frame still have the Windows 8 look.

App windows on Linux now always use AppWindowFrameView instead of the
blue CustomFrameView.

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

[Matt Giuca, 2014-03-25 08:13:35]

Hi Ben,

Doing a weird thing and sending you a CL I don't think is really acceptable --
so please don't spend too much time on this review. The reason it isn't
acceptable is that it means Linux still won't display native windows the way
Windows does. I want to discuss the best strategy so perhaps this CL is a
starting point for that discussion.

Matt

[Matt Giuca, 2014-03-27 06:39:38]

Sorry this took awhile. I got bogged down with draggable regions and a bit more
refactoring than expected to get rid of window_ (but I think it was worth it).

https://codereview.chromium.org/210363004/diff/100001/apps/ui/views/app_windo...
File apps/ui/views/app_window_frame_view.cc (right):

https://codereview.chromium.org/210363004/diff/100001/apps/ui/views/app_windo...
apps/ui/views/app_window_frame_view.cc:196: if (!draw_frame_) {
This part is a bit weird. On Linux, a native window will enter this code path
and create draggable regions. Is the intention that even if an app window
requests draggable regions, they are ignored if we have a window frame?

In that case, we will need another Boolean in here, frameless, which will add
extra confusion. (!draw_frame && !frameless means don't draw a frame, but there
is a frame, so don't do draggable regions.)

Since it's a minor concern, leaving it for now. I'm going to have to deal with
draggable regions on Linux anyway (I just discovered they are broken), so I'll
look into this in more detail in a follow-up CL.

[benwells, 2014-03-27 08:53:45]

lgtm if you remove the if (!has_frame) and make it if (draggable_region_

https://codereview.chromium.org/210363004/diff/100001/apps/ui/views/app_windo...
File apps/ui/views/app_window_frame_view.cc (right):

https://codereview.chromium.org/210363004/diff/100001/apps/ui/views/app_windo...
apps/ui/views/app_window_frame_view.cc:196: if (!draw_frame_) {
On 2014/03/27 06:39:38, Matt Giuca wrote:
> This part is a bit weird. On Linux, a native window will enter this code path
> and create draggable regions. Is the intention that even if an app window
> requests draggable regions, they are ignored if we have a window frame?
> 
> In that case, we will need another Boolean in here, frameless, which will add
> extra confusion. (!draw_frame && !frameless means don't draw a frame, but
there
> is a frame, so don't do draggable regions.)
> 
> Since it's a minor concern, leaving it for now. I'm going to have to deal with
> draggable regions on Linux anyway (I just discovered they are broken), so I'll
> look into this in more detail in a follow-up CL.

Yes, only frameless windows should have draggable regions. I think it would be
cleaner here to just have if (draggable_region_) and only set it if the window
is frameless.

Turns out this will always be the case anyway, see
https://code.google.com/p/chromium/codesearch#chromium/src/apps/ui/views/nati....

[Matt Giuca, 2014-03-27 09:16:27]

Thanks for your late night review!

https://codereview.chromium.org/210363004/diff/100001/apps/ui/views/app_windo...
File apps/ui/views/app_window_frame_view.cc (right):

https://codereview.chromium.org/210363004/diff/100001/apps/ui/views/app_windo...
apps/ui/views/app_window_frame_view.cc:196: if (!draw_frame_) {
Done.

Note: I did not "only set it if the window is frameless". I was going to change
the call to Init to say "IsFrameless() ? GetDraggableRegion() : NULL", but then
I realised since we've already got code that says "if not frameless prevent
setting draggable region", it would be bad to have a second check. (Keeping a
single point where that policy decision is made, in case we ever want to change
it.)

https://codereview.chromium.org/210363004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/210363004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:361:
!IsFrameless() && has_frame_color_,
Note: I simplified the CL a bit (after your review) -- this function no longer
takes a has_frame argument, it computes it directly in here.

[Matt Giuca, 2014-03-27 09:21:32]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-03-27 09:21:44]

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

[commit-bot: I haz the power, 2014-03-27 10:44:27]

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

[commit-bot: I haz the power, 2014-03-27 10:44:27]

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

[Matt Giuca, 2014-03-27 11:26:59]

Oh no, looks like this is regressing Thanh-Mai's bug (http://crbug.com/346115)
and I will need to make the window bounds refer to the non-client (frame)
bounds, not the client (content) bounds. I'll have to see how feasible this is
tomorrow.

:((((((((((((((((((

[benwells, 2014-03-28 10:36:31]

slgtm with one optional nit type thing.

https://codereview.chromium.org/210363004/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/210363004/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:361:
!IsFrameless() && has_frame_color_,
Can this just be has_frame_color_?

[Matt Giuca, 2014-03-28 10:39:51]

https://codereview.chromium.org/210363004/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/210363004/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:361:
!IsFrameless() && has_frame_color_,
On 2014/03/28 10:36:31, benwells wrote:
> Can this just be has_frame_color_?

Done.

[Matt Giuca, 2014-03-28 10:41:55]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-03-28 10:42:34]

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

[commit-bot: I haz the power, 2014-03-28 12:15:41]

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

[commit-bot: I haz the power, 2014-03-28 12:15:41]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Matt Giuca, 2014-03-28 12:48:04]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-03-28 12:49:33]

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

[commit-bot: I haz the power, 2014-03-28 17:06:49]

Change committed as 260176