[Mac] Include MacViews native app windows in regular builds.

[Mac] Include MacViews native app windows in regular builds.

The MacViews native app window implementation was only available behind
a gyp flag (mac_views_browser). This includes it into a regular Mac
build and allows it to be enabled from chrome:flags.

BUG=459877
Committed: https://crrev.com/4bcdf7e076321d092cea4b5224bfad1f8fff3e7a
Cr-Commit-Position: refs/heads/master@{#333636}

[jackhou1, 2015-06-04 03:47:25]

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

[jackhou1, 2015-06-04 03:47:26]

tapted, PTAL

[tapted, 2015-06-04 04:48:01]

Main concern is extension_keybinding_registry...

https://codereview.chromium.org/1167883002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6909: +        <message
name="IDS_FLAGS_MAC_VIEWS_NATIVE_APP_WINDOWS_NAME" desc="Name of the flag to
enable or disable toolkit-views Chrome App windows on Mac.">
nit: drop the _NATIVE_? Same below. [update:] Actually "native" is pretty much
spread throughout. I guess it makes sense to keep it here (or change it
everywhere..). I guess it's not that confusing.

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/DEPS (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/DEPS:2: "+chrome/browser/ui/views",
Hm - I'm looking for a generic solution to this.

WDYT of https://codereview.chromium.org/1162023008 ?

it has other implications (see below)

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm:9: #include
"chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h"
e.g. this would be disallowed, because this is not a "views_mac" file.

You could instead do

#include "chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.h"

Then in chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.cc
define a function like

extensions::NativeAppWindow* CreateViewsNativeAppWindow(
    extensions::AppWindow* app_window,
    const extensions::AppWindow::CreateParams& params) {
...
}



An alternative: There's actually no restriction on c/b/ui/views calling into
c/b/ui/cocoa. We could move this file to c/b/ui/views and call it
chrome_app_window_client_mac.mm

(it would sit next to chrome_app_window_client_views.cc which should probably be
renamed to chrome_app_window_client_aura.cc)


Or.. we could just say this is under /apps/ and we know what we're up to.


But see https://codereview.chromium.org/1162393003/ for an example of the stuff
I'm trying to avoid with that DEPS CL linked above. Maybe you have some other
ideas on how to solve that.

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm:12: bool
UseMacViewsNativeAppWindows() {
anonymous namespace

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1961:
'browser/ui/views/extensions/extension_keybinding_registry_views.cc',
I don't think this will work. Cocoa's BrowserWindowController only initializes a
ExtensionKeybindingRegistryCocoa. I think NativeAppWindowViews will need to be
refactored to target their abstract base class instead (i.e.
ExtensionsKeybindingRegistry). 

I think extension_keybinding_registry_views.cc will only funciton correctly with
a toolkit-views browser (i.e. it should stay in the mac_views_browser build
only)

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1964:
'browser/ui/views/frame/native_widget_mac_frameless_nswindow.mm',
sort into the ui/views stuff below

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1966: 
nit: no blank lines

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2430:
'browser/ui/views/apps/chrome_app_window_client_views_mac.mm',
ah! it already exists. So I think we just move this file into the build, have it
support Cocoa windows, delete this entire gyp section, and finally delete
chrome_app_window_client_cocoa.mm

[jackhou1, 2015-06-04 06:06:41]

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/DEPS (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/DEPS:2: "+chrome/browser/ui/views",
On 2015/06/04 04:48:00, tapted wrote:
> Hm - I'm looking for a generic solution to this.
> 
> WDYT of https://codereview.chromium.org/1162023008 ?
> 
> it has other implications (see below)

I agree it makes sense to be explict about which files:
- don't need views,
- use views but are part of the cocoa browser,
- are part of the views browser.

I expect there'll be a lot of files in the middle category before we can switch
to mac_views_browser=1.

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm:9: #include
"chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h"
On 2015/06/04 04:48:00, tapted wrote:
> e.g. this would be disallowed, because this is not a "views_mac" file.
> 
> You could instead do
> 
> #include "chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.h"
> 
> Then in chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.cc
> define a function like
> 
> extensions::NativeAppWindow* CreateViewsNativeAppWindow(
>     extensions::AppWindow* app_window,
>     const extensions::AppWindow::CreateParams& params) {
> ...
> }
> 
> 
> 
> An alternative: There's actually no restriction on c/b/ui/views calling into
> c/b/ui/cocoa. We could move this file to c/b/ui/views and call it
> chrome_app_window_client_mac.mm
> 
> (it would sit next to chrome_app_window_client_views.cc which should probably
be
> renamed to chrome_app_window_client_aura.cc)
> 
> 
> Or.. we could just say this is under /apps/ and we know what we're up to.
> 
> 
> But see https://codereview.chromium.org/1162393003/ for an example of the
stuff
> I'm trying to avoid with that DEPS CL linked above. Maybe you have some other
> ideas on how to solve that.

What about renaming this file to chrome_app_window_client_views_mac.mm, and
leave it in c/b/ui/cocoa/apps/?

I considered modifying c/b/ui/views/apps/chrome_app_window_client_views_mac.mm
instead of this. Leaving the other file as-is means that mac_views_browser
always uses mac-views app windows and does not need to pull in any of the cocoa
stuff, but maybe that's not a big deal.

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm:12: bool
UseMacViewsNativeAppWindows() {
On 2015/06/04 04:48:00, tapted wrote:
> anonymous namespace

Done.

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1961:
'browser/ui/views/extensions/extension_keybinding_registry_views.cc',
On 2015/06/04 04:48:00, tapted wrote:
> I don't think this will work. Cocoa's BrowserWindowController only initializes
a
> ExtensionKeybindingRegistryCocoa. I think NativeAppWindowViews will need to be
> refactored to target their abstract base class instead (i.e.
> ExtensionsKeybindingRegistry). 
> 
> I think extension_keybinding_registry_views.cc will only funciton correctly
with
> a toolkit-views browser (i.e. it should stay in the mac_views_browser build
> only)

extension_keybinding_registry_views.cc doesn't seem to depend on much from the
toolkit-views browser. I figured it would be better to use that and make it work
since we'd get to exercise some code that'll be in the mac-views browser. I
don't know much about how the views keybinding registry works, but it seems
possible to have both implementations.

I'm not sure if refactoring ChromeNativeAppWindowViews to use
ExtensionKeybindingRegistryCocoa is straightforward either. With
NativeAppWindowCocoa, events come in via ChromeEventProcessingWindow which calls
NativeAppWindowCocoa::HandledByExtensionCommand, which directly calls
ExtensionKeybindingRegistryCocoa::ProcessKeyEvent.

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1964:
'browser/ui/views/frame/native_widget_mac_frameless_nswindow.mm',
On 2015/06/04 04:48:00, tapted wrote:
> sort into the ui/views stuff below

Done.

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1966: 
On 2015/06/04 04:48:00, tapted wrote:
> nit: no blank lines

Done.

[tapted, 2015-06-05 02:09:08]

lgtm if you like my rename idea

it'll need a follow-up to update the browser_tests. i.e. the files that say
"this tests whatever is linked in" will need a test parameter that sets what the
value of this flag should be.

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm:9: #include
"chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h"
On 2015/06/04 06:06:40, jackhou1 wrote:
> On 2015/06/04 04:48:00, tapted wrote:
> > e.g. this would be disallowed, because this is not a "views_mac" file.
> > 
> > You could instead do
> > 
> > #include "chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.h"
> > 
> > Then in chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.cc
> > define a function like
> > 
> > extensions::NativeAppWindow* CreateViewsNativeAppWindow(
> >     extensions::AppWindow* app_window,
> >     const extensions::AppWindow::CreateParams& params) {
> > ...
> > }
> > 
> > 
> > 
> > An alternative: There's actually no restriction on c/b/ui/views calling into
> > c/b/ui/cocoa. We could move this file to c/b/ui/views and call it
> > chrome_app_window_client_mac.mm
> > 
> > (it would sit next to chrome_app_window_client_views.cc which should
probably
> be
> > renamed to chrome_app_window_client_aura.cc)
> > 
> > 
> > Or.. we could just say this is under /apps/ and we know what we're up to.
> > 
> > 
> > But see https://codereview.chromium.org/1162393003/ for an example of the
> stuff
> > I'm trying to avoid with that DEPS CL linked above. Maybe you have some
other
> > ideas on how to solve that.
> 
> What about renaming this file to chrome_app_window_client_views_mac.mm, and
> leave it in c/b/ui/cocoa/apps/?
> 
> I considered modifying c/b/ui/views/apps/chrome_app_window_client_views_mac.mm
> instead of this. Leaving the other file as-is means that mac_views_browser
> always uses mac-views app windows and does not need to pull in any of the
cocoa
> stuff, but maybe that's not a big deal.

Yep - I agree it would be best to leave
c/b/ui/views/apps/chrome_app_window_client_views_mac.mm as-is, without a Cocoa
dep. I was thinking of changing it to not implement
ChromeAppWindowClient::CreateNativeAppWindowImpl() directly, but provide access
to ChromeNativeAppWindowViewsMac (i.e. so there's only one place that does `new
ChromeNativeAppWindowViewsMac`). But I just toyed with that, and I don't think
there's a way to do it without #ifdefs which we don't want.

Then my only objection is that having two files called
chrome_app_window_client_views_mac.mm would be confusing :).

How about renaming to chrome_app_window_client_views_cocoa.mm?, then I can just
alter the deps rule to match on "_views_" so it gets views_cocoa and views_mac.
views_cocoa is a good substring for files that can swap in both cocoa and views
implementations at runtime (although it feels a little bit like a contradiction
to have both).

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1961:
'browser/ui/views/extensions/extension_keybinding_registry_views.cc',
On 2015/06/04 06:06:40, jackhou1 wrote:
> On 2015/06/04 04:48:00, tapted wrote:
> > I don't think this will work. Cocoa's BrowserWindowController only
initializes
> a
> > ExtensionKeybindingRegistryCocoa. I think NativeAppWindowViews will need to
be
> > refactored to target their abstract base class instead (i.e.
> > ExtensionsKeybindingRegistry). 
> > 
> > I think extension_keybinding_registry_views.cc will only funciton correctly
> with
> > a toolkit-views browser (i.e. it should stay in the mac_views_browser build
> > only)
> 
> extension_keybinding_registry_views.cc doesn't seem to depend on much from the
> toolkit-views browser. I figured it would be better to use that and make it
work
> since we'd get to exercise some code that'll be in the mac-views browser. I
> don't know much about how the views keybinding registry works, but it seems
> possible to have both implementations.
> 
> I'm not sure if refactoring ChromeNativeAppWindowViews to use
> ExtensionKeybindingRegistryCocoa is straightforward either. With
> NativeAppWindowCocoa, events come in via ChromeEventProcessingWindow which
calls
> NativeAppWindowCocoa::HandledByExtensionCommand, which directly calls
> ExtensionKeybindingRegistryCocoa::ProcessKeyEvent.

Hm - yeah I think it will be OK. A "registry" sounds a bit like a singleton, but
that doesn't seem to be the case.

Another concern might be that ExtensionKeybindingRegistryCocoa::ProcessKeyEvent
emits, e.g.,  extensions::NOTIFICATION_EXTENSION_COMMAND_PAGE_ACTION_MAC but the
views one does not. However, I'm pretty sure that code path doesn't matter for
packaged apps either way. i.e. only keybindings on the browser window should be
invoking browser actions.

So this lg - I don't think anything will break, but there might be some test
cases to try out.

[jackhou1, 2015-06-05 02:48:42]

Removed the deps change, so I'll wait for your deps CL to land.

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm (right):

https://codereview.chromium.org/1167883002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/chrome_app_window_client_cocoa.mm:9: #include
"chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h"
On 2015/06/05 02:09:07, tapted wrote:
> On 2015/06/04 06:06:40, jackhou1 wrote:
> > On 2015/06/04 04:48:00, tapted wrote:
> > > e.g. this would be disallowed, because this is not a "views_mac" file.
> > > 
> > > You could instead do
> > > 
> > > #include
"chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.h"
> > > 
> > > Then in chrome/browser/ui/cocoa/apps/chrome_app_window_client_views_mac.cc
> > > define a function like
> > > 
> > > extensions::NativeAppWindow* CreateViewsNativeAppWindow(
> > >     extensions::AppWindow* app_window,
> > >     const extensions::AppWindow::CreateParams& params) {
> > > ...
> > > }
> > > 
> > > 
> > > 
> > > An alternative: There's actually no restriction on c/b/ui/views calling
into
> > > c/b/ui/cocoa. We could move this file to c/b/ui/views and call it
> > > chrome_app_window_client_mac.mm
> > > 
> > > (it would sit next to chrome_app_window_client_views.cc which should
> probably
> > be
> > > renamed to chrome_app_window_client_aura.cc)
> > > 
> > > 
> > > Or.. we could just say this is under /apps/ and we know what we're up to.
> > > 
> > > 
> > > But see https://codereview.chromium.org/1162393003/ for an example of the
> > stuff
> > > I'm trying to avoid with that DEPS CL linked above. Maybe you have some
> other
> > > ideas on how to solve that.
> > 
> > What about renaming this file to chrome_app_window_client_views_mac.mm, and
> > leave it in c/b/ui/cocoa/apps/?
> > 
> > I considered modifying
c/b/ui/views/apps/chrome_app_window_client_views_mac.mm
> > instead of this. Leaving the other file as-is means that mac_views_browser
> > always uses mac-views app windows and does not need to pull in any of the
> cocoa
> > stuff, but maybe that's not a big deal.
> 
> Yep - I agree it would be best to leave
> c/b/ui/views/apps/chrome_app_window_client_views_mac.mm as-is, without a Cocoa
> dep. I was thinking of changing it to not implement
> ChromeAppWindowClient::CreateNativeAppWindowImpl() directly, but provide
access
> to ChromeNativeAppWindowViewsMac (i.e. so there's only one place that does
`new
> ChromeNativeAppWindowViewsMac`). But I just toyed with that, and I don't think
> there's a way to do it without #ifdefs which we don't want.
> 
> Then my only objection is that having two files called
> chrome_app_window_client_views_mac.mm would be confusing :).
> 
> How about renaming to chrome_app_window_client_views_cocoa.mm?, then I can
just
> alter the deps rule to match on "_views_" so it gets views_cocoa and
views_mac.
> views_cocoa is a good substring for files that can swap in both cocoa and
views
> implementations at runtime (although it feels a little bit like a
contradiction
> to have both).

Done.

[jackhou1, 2015-06-09 03:43:20]

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

[jackhou1, 2015-06-09 03:43:20]

sky, please review for OWNERS:
chrome/browser/ui/BUILD.gn

isherman, please review for OWNERS:
tools/metrics/histograms/histograms.xml

[sky, 2015-06-09 13:12:30]

LGTM

[Ilya Sherman, 2015-06-09 20:23:35]

histograms.xml lgtm

[jackhou1, 2015-06-10 00:33:45]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-06-10 00:33:46]

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/1167883002/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-06-10 00:39:39]

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

[commit-bot: I haz the power, 2015-06-10 01:26:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-10 01:27:18]

Patchset 5 (id:??) landed as
https://crrev.com/4bcdf7e076321d092cea4b5224bfad1f8fff3e7a
Cr-Commit-Position: refs/heads/master@{#333636}