MacViews: Get chrome/browser/ui/views/frame to compile

MacViews: Get chrome/browser/ui/views/frame to compile

Some fixes to get this subdirectory to compile for MacViews.
An aura::Window dependency is moved from BrowserView to
BrowserCommandHandlerX11.
Hide Ash dependency from ImmersiveModeController.
Add Mac implementation of NativeBrowserFrame.

Now that we include both BrowserWindow::CreateBrowserWindow definitions
in the build, add a new build flag "mac_views_browser" to select between
Cocoa and Views browser.

BUG=425229
Committed: https://crrev.com/f6158b090dc352608141ba8a9d8f09bbdbf4af88
Cr-Commit-Position: refs/heads/master@{#303299}

[Andre, 2014-11-03 22:42:33]

Patchset #1 (id:1) has been deleted

[Andre, 2014-11-03 23:39:00]

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

[Andre, 2014-11-03 23:39:01]

Trent, PTAL.

[tapted, 2014-11-04 02:33:43]

We should add a BUG= to track these conversions.

The comments are mostly ways to avoid the #fidefs, since they don't usually lead
to maintainable code

https://codereview.chromium.org/697223003/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/697223003/diff/40001/build/common.gypi#newcode65
build/common.gypi:65: 'mac_views_browser%': 0,
nit: since other things don't depend on it, I think convention is to put this in
a later scope, so that it doesn't need to be promoted out each time. it can
probably go down after  'ozone_platform_test%': 0, on line ~1500

https://codereview.chromium.org/697223003/diff/40001/build/common.gypi#newcod...
build/common.gypi:2592: 'defines': ['MAC_VIEWS_BROWSER=1'],
You might not need this with the comments below

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_factory.mm (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_factory.mm:10: #if
!defined(MAC_VIEWS_BROWSER)
I think it's more conventional to exclude the entire file at the gyp/gn level

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:7: #include "ash/shell.h"
remove?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:14: #include
"chrome/browser/ui/ash/ash_util.h"
remove? or guard in #if ash

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:29: #include "ui/aura/window.h"
remove?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:30: #include
"ui/aura/window_event_dispatcher.h"
remove?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:98: if
(browser_view_->browser()->host_desktop_type() ==
This feels out of place here... the condition is essentially what's in
NativeBrowserFrameFactory::ShouldCreateForAshDesktop(), which is called from
NativeBrowserFrameFactory::Create() in the _auralinux and _aurawin versions.

I'm thinking maybe an interface like

class NativeBrowserFrame {
 public:
  /* .. */
  virtual views::Widget::InitParams WidgetParams() const = 0;


That might also allow you to get rid the NativeBrowserFrame::AsNativeWidget
interfaces (although codesearch might be hiding some other callers), as well as
some further #includes from this file

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:1443: #if defined(USE_AURA)
We'll likely need something here for Mac.. (and it might need objective C). This
would, e.g., ensure the renderer is using the correct display scale factor once
it knows what kind of NSScreen it's going to be drawn on.

One way to avoid the #ifdefs might be frame()->OnTabInserted(contents)

BrowserFrame::OnTabInserted(WebContents* contents) { 
  native_browser_frame_->OnTabInserted(contents);
}

BrowserFrameAura::OnTabInserted(WebContents* contents) {
  /* .. */
}

that's a big thread to pull though.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:2531: BrowserWindow*
BrowserWindow::CreateBrowserWindow(Browser* browser) {
move these to browser_window_factory.cc?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.h:354: void
EnterFullscreenWithChrome() override;
I don't think this is the right long-term approach. Can we remove the #ifdefs
from fullscreen_controller.cc and make an interface on BrowserWindow that works
in all build configurations?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/contents_web_view.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_web_view.cc:10: #include
"ui/aura/window.h"
guard this?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_web_view.cc:14: #include
"ui/wm/core/window_util.h"
guard this?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_web_view.cc:88: cloned_layer_tree_ =
wm::RecreateLayers(web_contents()->GetNativeView());
So, after poking some more, I think this won't be needed for MacViews. Closing
an NSWindow doesn't animate - it just disappears - so there's no "orphaned"
layer context that needs to be displayed during the close animation. I think you
can say something to that effect here, and remove the TODO

An alternative without #ifdefs might be something like `cloned_layer_tree_ =
super::holder_->native_wrapper_->RecreateLayers();`, but #ifdefs are maybe OK
here.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller.h (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller.h:30:
DISALLOW_COPY_AND_ASSIGN(ImmersiveRevealedLock);
I think you can turn this into a pure interface. Which basically means removing
this line and inlining the definitions of the constructor/destructor.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller.h:97: virtual
ImmersiveRevealedLock* GetRevealedLock(
This should really be called `CreateRevealedLock`, (and/or return a scoped_ptr,
but that's maybe unnecessary effort)

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:51: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:8: #if
defined(USE_ASH)
I'd go for a separate file: immersive_mode_controller_factory_mac.cc which
returns a `new ImmersiveModeControllerStub();` unconditionally, and just exclude
this file from a mac build (or give it an _ash suffix if that works for gn)

[Andre, 2014-11-05 01:23:13]

Patchset #3 (id:60001) has been deleted

[Andre, 2014-11-05 02:04:23]

Trent PTAL.
Added the bug# to description.

https://codereview.chromium.org/697223003/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/697223003/diff/40001/build/common.gypi#newcode65
build/common.gypi:65: 'mac_views_browser%': 0,
On 2014/11/04 02:33:42, tapted wrote:
> nit: since other things don't depend on it, I think convention is to put this
in
> a later scope, so that it doesn't need to be promoted out each time. it can
> probably go down after  'ozone_platform_test%': 0, on line ~1500

Done.

https://codereview.chromium.org/697223003/diff/40001/build/common.gypi#newcod...
build/common.gypi:2592: 'defines': ['MAC_VIEWS_BROWSER=1'],
On 2014/11/04 02:33:42, tapted wrote:
> You might not need this with the comments below

Deleted.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_factory.mm (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_factory.mm:10: #if
!defined(MAC_VIEWS_BROWSER)
On 2014/11/04 02:33:42, tapted wrote:
> I think it's more conventional to exclude the entire file at the gyp/gn level

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:7: #include "ash/shell.h"
On 2014/11/04 02:33:42, tapted wrote:
> remove?

Needed for ash::Shell:GetInstance().
Moved inside #if(OS_CHROMEOS) guard.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:14: #include
"chrome/browser/ui/ash/ash_util.h"
On 2014/11/04 02:33:42, tapted wrote:
> remove? or guard in #if ash

No longer needed, deleted.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:29: #include "ui/aura/window.h"
On 2014/11/04 02:33:42, tapted wrote:
> remove?

Removed.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:30: #include
"ui/aura/window_event_dispatcher.h"
On 2014/11/04 02:33:42, tapted wrote:
> remove?

Removed.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:98: if
(browser_view_->browser()->host_desktop_type() ==
On 2014/11/04 02:33:42, tapted wrote:
> This feels out of place here... the condition is essentially what's in
> NativeBrowserFrameFactory::ShouldCreateForAshDesktop(), which is called from
> NativeBrowserFrameFactory::Create() in the _auralinux and _aurawin versions.
> 
> I'm thinking maybe an interface like
> 
> class NativeBrowserFrame {
>  public:
>   /* .. */
>   virtual views::Widget::InitParams WidgetParams() const = 0;
> 
> 
> That might also allow you to get rid the NativeBrowserFrame::AsNativeWidget
> interfaces (although codesearch might be hiding some other callers), as well
as
> some further #includes from this file

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:1443: #if defined(USE_AURA)
On 2014/11/04 02:33:42, tapted wrote:
> We'll likely need something here for Mac.. (and it might need objective C).
This
> would, e.g., ensure the renderer is using the correct display scale factor
once
> it knows what kind of NSScreen it's going to be drawn on.
> 
> One way to avoid the #ifdefs might be frame()->OnTabInserted(contents)
> 
> BrowserFrame::OnTabInserted(WebContents* contents) { 
>   native_browser_frame_->OnTabInserted(contents);
> }
> 
> BrowserFrameAura::OnTabInserted(WebContents* contents) {
>   /* .. */
> }
> 
> that's a big thread to pull though.

The problem is that we will need this code in both BrowserFrameAsh and
DesktopBrowserFrameAura (right?).
I didn't see a nice way to avoid duplicating the code.
WebContentsViewAura may be a better place for this, since its CreateView has
related code.
I'm not sure we will need this for Mac, because we have other hooks such as
viewDidMoveToWindow.
WDYT?

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:2531: BrowserWindow*
BrowserWindow::CreateBrowserWindow(Browser* browser) {
On 2014/11/04 02:33:43, tapted wrote:
> move these to browser_window_factory.cc?

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.h:354: void
EnterFullscreenWithChrome() override;
On 2014/11/04 02:33:43, tapted wrote:
> I don't think this is the right long-term approach. Can we remove the #ifdefs
> from fullscreen_controller.cc and make an interface on BrowserWindow that
works
> in all build configurations?

Looks tricky, I think we will need to do it in a separate CL.
I have removed this change (and the gyp change) from the patch.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/contents_web_view.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_web_view.cc:10: #include
"ui/aura/window.h"
On 2014/11/04 02:33:43, tapted wrote:
> guard this?

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_web_view.cc:14: #include
"ui/wm/core/window_util.h"
On 2014/11/04 02:33:43, tapted wrote:
> guard this?

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_web_view.cc:88: cloned_layer_tree_ =
wm::RecreateLayers(web_contents()->GetNativeView());
On 2014/11/04 02:33:43, tapted wrote:
> So, after poking some more, I think this won't be needed for MacViews. Closing
> an NSWindow doesn't animate - it just disappears - so there's no "orphaned"
> layer context that needs to be displayed during the close animation. I think
you
> can say something to that effect here, and remove the TODO
> 
> An alternative without #ifdefs might be something like `cloned_layer_tree_ =
> super::holder_->native_wrapper_->RecreateLayers();`, but #ifdefs are maybe OK
> here.

Done.
Thanks for figuring it out.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller.h (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller.h:30:
DISALLOW_COPY_AND_ASSIGN(ImmersiveRevealedLock);
On 2014/11/04 02:33:43, tapted wrote:
> I think you can turn this into a pure interface. Which basically means
removing
> this line and inlining the definitions of the constructor/destructor.

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller.h:97: virtual
ImmersiveRevealedLock* GetRevealedLock(
On 2014/11/04 02:33:43, tapted wrote:
> This should really be called `CreateRevealedLock`, (and/or return a
scoped_ptr,
> but that's maybe unnecessary effort)

I agree, but probably does not belong in this CL.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:51: };
On 2014/11/04 02:33:43, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:8: #if
defined(USE_ASH)
On 2014/11/04 02:33:43, tapted wrote:
> I'd go for a separate file: immersive_mode_controller_factory_mac.cc which
> returns a `new ImmersiveModeControllerStub();` unconditionally, and just
exclude
> this file from a mac build (or give it an _ash suffix if that works for gn)

Done.

[tapted, 2014-11-05 05:00:47]

lgtm

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/697223003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:1443: #if defined(USE_AURA)
On 2014/11/05 02:04:23, Andre wrote:
> On 2014/11/04 02:33:42, tapted wrote:
> > We'll likely need something here for Mac.. (and it might need objective C).
> This
> > would, e.g., ensure the renderer is using the correct display scale factor
> once
> > it knows what kind of NSScreen it's going to be drawn on.
> > 
> > One way to avoid the #ifdefs might be frame()->OnTabInserted(contents)
> > 
> > BrowserFrame::OnTabInserted(WebContents* contents) { 
> >   native_browser_frame_->OnTabInserted(contents);
> > }
> > 
> > BrowserFrameAura::OnTabInserted(WebContents* contents) {
> >   /* .. */
> > }
> > 
> > that's a big thread to pull though.
> 
> The problem is that we will need this code in both BrowserFrameAsh and
> DesktopBrowserFrameAura (right?).
> I didn't see a nice way to avoid duplicating the code.
> WebContentsViewAura may be a better place for this, since its CreateView has
> related code.
> I'm not sure we will need this for Mac, because we have other hooks such as
> viewDidMoveToWindow.
> WDYT?

yeah - it looks tricky to get BrowserFrameAsh/Aura sharing base class without
crazy injection tricks. Creating a hierarchy off NativeBrowserFrame (i.e.
NativeBrowserFrameAura/NativeBrowserFrameMac) should work instead... but it
raises some multiple-inheritance red-flags.

But if Mac is functional with this #ifdef'd out, then it should be OK like this.

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:39: return 0;
NOTIMPLEMENTED()?

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

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.h:352: 
nit: can remove this line to avoid a delta in this file

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

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/native_browser_frame.h:23: virtual
views::Widget::InitParams GetWidgetParams() = 0;
needs a comment

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

https://codereview.chromium.org/697223003/diff/100001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2005:
'browser/ui/views/frame/browser_frame_mac.h',
nit: sort after frame_common

https://codereview.chromium.org/697223003/diff/100001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2809: }, {
you'll need to replicate this bit in gn - I think we're trying to keep them in
sync even though there's no gn bot for mac yet

[Andre, 2014-11-05 20:55:23]

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:39: return 0;
On 2014/11/05 05:00:47, tapted wrote:
> NOTIMPLEMENTED()?

Done.

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

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.h:352: 
On 2014/11/05 05:00:47, tapted wrote:
> nit: can remove this line to avoid a delta in this file

Done.

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

https://codereview.chromium.org/697223003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/native_browser_frame.h:23: virtual
views::Widget::InitParams GetWidgetParams() = 0;
On 2014/11/05 05:00:47, tapted wrote:
> needs a comment

Done.

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

https://codereview.chromium.org/697223003/diff/100001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2005:
'browser/ui/views/frame/browser_frame_mac.h',
On 2014/11/05 05:00:47, tapted wrote:
> nit: sort after frame_common

Done.

https://codereview.chromium.org/697223003/diff/100001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2809: }, {
On 2014/11/05 05:00:47, tapted wrote:
> you'll need to replicate this bit in gn - I think we're trying to keep them in
> sync even though there's no gn bot for mac yet

Done.

[Andre, 2014-11-05 20:59:52]

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

[Andre, 2014-11-05 20:59:52]

sky@ PTAL.

[sky, 2014-11-05 22:44:25]

https://codereview.chromium.org/697223003/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/697223003/diff/160001/build/common.gypi#newco...
build/common.gypi:1506: # Whether the browser is non-native (using Views
Toolkit) on Mac.
Do we really need the ability to change this independent of toolkit_views? I'm
wondering if you can instead use toolkit_views always?

https://codereview.chromium.org/697223003/diff/160001/build/common.gypi#newco...
build/common.gypi:1507: 'mac_views_browser%': 0,
Can you key off toolkit_views instead of another define?

[Andre, 2014-11-05 23:27:04]

https://codereview.chromium.org/697223003/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/697223003/diff/160001/build/common.gypi#newco...
build/common.gypi:1506: # Whether the browser is non-native (using Views
Toolkit) on Mac.
On 2014/11/05 22:44:25, sky wrote:
> Do we really need the ability to change this independent of toolkit_views? I'm
> wondering if you can instead use toolkit_views always?

We would like the standard Mac build to include toolkit_views=1 so that we can
avoid MacViews build regression. At link time, the views code get pruned because
they are not referenced and the Cocoa code runs as before. This means that we
need another flag to select linking the views browser instead of the Cocoa one
(for development only for now).

[sky, 2014-11-05 23:57:35]

Ugh, ok, LGTM

[Andre, 2014-11-05 23:58:41]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-11-06 00:02:43]

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

[commit-bot: I haz the power, 2014-11-06 05:21:53]

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

[commit-bot: I haz the power, 2014-11-06 05:21:54]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Andre, 2014-11-07 01:34:03]

Patchset #8 (id:180001) has been deleted

[Andre, 2014-11-07 06:46:40]

Trent, Scott, PTAL.
The failure was because window_ was already deleted by the time we get to
~BrowserCommandHandlerX11.

[tapted, 2014-11-07 08:35:59]

lgtm - (looks like this is just bringing back a check that used to be in
~BrowserFrame(), so makes sense)

https://codereview.chromium.org/697223003/diff/200001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_command_handler_x11.cc (right):

https://codereview.chromium.org/697223003/diff/200001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_command_handler_x11.cc:21: if (window)
Should this ever be null? - maybe DCHECK(window)?

[Andre, 2014-11-07 19:06:44]

https://codereview.chromium.org/697223003/diff/200001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_command_handler_x11.cc (right):

https://codereview.chromium.org/697223003/diff/200001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_command_handler_x11.cc:21: if (window)
On 2014/11/07 08:35:58, tapted wrote:
> Should this ever be null? - maybe DCHECK(window)?

Done.

[Andre, 2014-11-07 20:54:41]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-11-07 20:56:36]

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

[commit-bot: I haz the power, 2014-11-07 21:46:27]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2014-11-07 21:47:09]

Patchset 9 (id:??) landed as
https://crrev.com/f6158b090dc352608141ba8a9d8f09bbdbf4af88
Cr-Commit-Position: refs/heads/master@{#303299}