[Mac] Use chrome_native_app_window_views when mac_views_browser is enabled.

[Mac] Use chrome_native_app_window_views when mac_views_browser is enabled.

BUG=459877
Committed: https://crrev.com/3b0ed8bb4f9d4e72a4f0816fd37ca68828da5135
Cr-Commit-Position: refs/heads/master@{#318793}

[jackhou1, 2015-02-20 07:22:56]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2015-02-20 07:50:30]

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

https://codereview.chromium.org/935193003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:270:
wm::SetShadowType(widget()->GetNativeWindow(), wm::SHADOW_TYPE_NONE);
I think we need a general fix for this.

Mac automatically removes the shadow when the window type is frameless, so I
guess this is already done. But there are cases we don't want that. E.g.
menus/combo boxes should have a shadow, which currently gets removed.

For consistency, we probably want [window_ setHasShadow:YES] for
NativeWidgetMac, and then either a way to set the window shadow through the
views::Widget interface, or some kind of utility class like

#include "ui/views/native_window_util.h"  (or maybe even ui/base..)

void RemoveShadow(gfx::NativeWindow window);

which has a _aura.cc file for aura and a _mac.cc file for mac.

https://codereview.chromium.org/935193003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:620: #if
defined(USE_AURA)
It's annoying... but I think we need to try to make a 

ChromeNativeAppWindowViewsAura

and a

ChromeNativeAppWindowViewsMac

We'll probably need a ChromeNativeAppWindowViewsMac at some point anyway

https://codereview.chromium.org/935193003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:703: #if
!defined(OS_CHROMEOS) && defined(USE_AURA)
If this moves to ChromeNativeAppWindowAura the #ifdef (and the USE_ASH one
above) can probably just be removed. (ChromeOS never has desktop_type != ASH,
and it should still compile..).

There's probably some more USE_ASH mess in this file, so moving that and all the
aura dependencies out to ChromeNativeAppWindowAura is probably the best first
step before trying to get this compiling on Mac.

https://codereview.chromium.org/935193003/diff/20001/chrome/chrome_browser_ui...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/935193003/diff/20001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2855:
'browser/ui/views/apps/chrome_apps_client_views.cc',
These lists in `conditions` need to be duplicated in the BUILD.gn files as well.

An alternative is to make a variable, but I'd say this is experimental enough
that it doesn't deserve one (it's already a bit tough deciding exactly which
variable above a new file should go into...)

[tapted, 2015-02-20 07:51:01]

oops +mac-views-reviews@chromium.org

[jackhou1, 2015-02-26 04:35:16]

PTAL

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

https://codereview.chromium.org/935193003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:270:
wm::SetShadowType(widget()->GetNativeWindow(), wm::SHADOW_TYPE_NONE);
On 2015/02/20 07:50:29, tapted wrote:
> I think we need a general fix for this.
> 
> Mac automatically removes the shadow when the window type is frameless, so I
> guess this is already done. But there are cases we don't want that. E.g.
> menus/combo boxes should have a shadow, which currently gets removed.
> 
> For consistency, we probably want [window_ setHasShadow:YES] for
> NativeWidgetMac, and then either a way to set the window shadow through the
> views::Widget interface, or some kind of utility class like
> 
> #include "ui/views/native_window_util.h"  (or maybe even ui/base..)
> 
> void RemoveShadow(gfx::NativeWindow window);
> 
> which has a _aura.cc file for aura and a _mac.cc file for mac.

Done.

https://codereview.chromium.org/935193003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:620: #if
defined(USE_AURA)
On 2015/02/20 07:50:30, tapted wrote:
> It's annoying... but I think we need to try to make a 
> 
> ChromeNativeAppWindowViewsAura
> 
> and a
> 
> ChromeNativeAppWindowViewsMac
> 
> We'll probably need a ChromeNativeAppWindowViewsMac at some point anyway

Done.

https://codereview.chromium.org/935193003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:703: #if
!defined(OS_CHROMEOS) && defined(USE_AURA)
On 2015/02/20 07:50:30, tapted wrote:
> If this moves to ChromeNativeAppWindowAura the #ifdef (and the USE_ASH one
> above) can probably just be removed. (ChromeOS never has desktop_type != ASH,
> and it should still compile..).
> 
> There's probably some more USE_ASH mess in this file, so moving that and all
the
> aura dependencies out to ChromeNativeAppWindowAura is probably the best first
> step before trying to get this compiling on Mac.

Done.

https://codereview.chromium.org/935193003/diff/20001/chrome/chrome_browser_ui...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/935193003/diff/20001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2855:
'browser/ui/views/apps/chrome_apps_client_views.cc',
On 2015/02/20 07:50:30, tapted wrote:
> These lists in `conditions` need to be duplicated in the BUILD.gn files as
well.
> 
> An alternative is to make a variable, but I'd say this is experimental enough
> that it doesn't deserve one (it's already a bit tough deciding exactly which
> variable above a new file should go into...)

Done.

[tapted, 2015-02-27 00:04:11]

https://codereview.chromium.org/935193003/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_apps_client_views_mac.cc (right):

https://codereview.chromium.org/935193003/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_apps_client_views_mac.cc:1: // Copyright
2015 The Chromium Authors. All rights reserved.
I'd make this a .mm file. The chances of it having to be Objective-C later is
nigh on certain. E.g. to do macviews' version of #import native_app_window_cocoa
. 

We'll also need a runtime flag while rolling this out -- this file would need to
#import both native_app_window_cocoa.h and native_app_window_views_mac.h and
pick which one to return below based on a chrome://flag

also.. why "chrome_apps_client_views_mac" should they all be
"chrome_app_window_client_foo" like chrome_app_window_client_cocoa?

[jackhou1, 2015-02-27 03:14:33]

https://codereview.chromium.org/935193003/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_apps_client_views_mac.cc (right):

https://codereview.chromium.org/935193003/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_apps_client_views_mac.cc:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/02/27 00:04:11, tapted wrote:
> I'd make this a .mm file. The chances of it having to be Objective-C later is
> nigh on certain. E.g. to do macviews' version of #import
native_app_window_cocoa
> . 

Done.

> We'll also need a runtime flag while rolling this out -- this file would need
to
> #import both native_app_window_cocoa.h and native_app_window_views_mac.h and
> pick which one to return below based on a chrome://flag
> 
> also.. why "chrome_apps_client_views_mac" should they all be
> "chrome_app_window_client_foo" like chrome_app_window_client_cocoa?

There's chrome_apps_client_views.cc and chrome_apps_client_views_win.cc in this
directory. Should I rename them?

[jackhou1, 2015-02-27 06:02:00]

Renamed them, as discussed.

[tapted, 2015-03-01 23:15:07]

lgtm

https://codereview.chromium.org/935193003/diff/100001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/935193003/diff/100001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:231: sources += [
"views/apps/chrome_apps_client_views_mac.mm" ]
views/apps/chrome_app_window_client_views_mac.mm

[jackhou1, 2015-03-01 23:43:01]

https://codereview.chromium.org/935193003/diff/100001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/935193003/diff/100001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:231: sources += [
"views/apps/chrome_apps_client_views_mac.mm" ]
On 2015/03/01 23:15:07, tapted wrote:
> views/apps/chrome_app_window_client_views_mac.mm

Done.

[jackhou1, 2015-03-01 23:43:52]

jackhou@chromium.org changed reviewers:
+ sky@chromium.org

[jackhou1, 2015-03-01 23:43:53]

sky, please review for OWNERS of:

chrome/browser/ui/BUILD.gn

[sky, 2015-03-02 17:58:14]

.gn/.gyp LGTM

[jackhou1, 2015-03-02 22:14:24]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-03-02 22:14:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/935193003/#ps120001
(title: "Address comments")

[commit-bot: I haz the power, 2015-03-02 22:15:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935193003/120001

[commit-bot: I haz the power, 2015-03-02 23:09:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-02 23:10:33]

Patchset 7 (id:??) landed as
https://crrev.com/3b0ed8bb4f9d4e72a4f0816fd37ca68828da5135
Cr-Commit-Position: refs/heads/master@{#318793}