[Mac] Turn on Auto Layout for browser windows.

chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm

Index: chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
diff --git a/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm b/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
index 6f13830e02876cd8302338c94a833f395c2e38dc..3a61a7714d2a8c224ee32e1d6c669bd5bf01df52 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
+++ b/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
@@ -20,10 +20,11 @@ class ChromeBrowserWindowTest : public CocoaTest {
     const NSUInteger mask = NSTitledWindowMask | NSClosableWindowMask |
         NSMiniaturizableWindowMask | NSResizableWindowMask;
     window_ = [[ChromeBrowserWindow alloc]
-               initWithContentRect:NSMakeRect(0, 0, 800, 600)
-               styleMask:mask
-               backing:NSBackingStoreBuffered
-               defer:NO];
+           initWithContentRect:NSMakeRect(0, 0, 800, 600)
+                     styleMask:mask
+                       backing:NSBackingStoreBuffered
+                         defer:NO
+        wantsViewsOverTitlebar:YES];

[tapted, 2017/03/21 02:16:51]

Is this needed? It suggests that passing `NO` will result in a window that does
not want auto layout. We pass `NO` when there is no tab strip, which can happen
for popups, devtools, packaged apps. I think for the reasons you point out, we
wouldn't want these windows to behave differently in aspects of layout versus
regular browser windows. E.g. packaged apps want to do similar things; drawing
over the titlebar sometimes (and don't use BorderlessWindowMask) -- see
NativeAppWindowCocoa::InstallView()  (I think we just mash a WebContentsView
into the contentView in that case, but it's the altered codepaths for resizing
the WebContentsView that required the fix in http://crbug.com/655112#c16 -- I
think we'd want the code touched there to be able to make the same assumptions
about the layout codepaths.

[Sidney San Martín, 2017/03/22 21:40:22]

Where we force Auto Layout is somewhat arbitrary. Not *every* window should be
opted in, since that includes bubbles, dialogs, and menus, but you're right that
every kind of browser window should be.

I moved +requiresConstraintBasedLayout to the window controller's
chromeContentView (which turns out to be created by TabWindowController, which
even non-tabbed browser windows use, which is still odd but outside the scope of
this CL…). Let me know how that sounds.

     if (base::debug::BeingDebugged()) {
       [window_ orderFront:nil];
     } else {
@@ -45,3 +46,16 @@ TEST_F(ChromeBrowserWindowTest, ShowAndClose) {
   [window_ display];
 }
 
+// Verify that browser windows use Auto Layout. Since frame-based layout and

[tapted, 2017/03/21 02:16:51]

This is super nit-picky, but I'm not sure if this test is verifying that browser
windows use AutoLayout so much as verifying that the ChromeBrowserWindowTest
test harness creates a window that uses autolayout. 

Perhaps just a re-wording is appropriate here. E.g. "Verify that passing
wantsViewsOverTitlebar:YES to the  ChromeBrowserWindow initializer results in a
window that uses Auto Layout."

[Sidney San Martín, 2017/03/22 21:40:22]

Heh, great point :). Re-wording the comment like that would have shown that the
test wasn't meaningful. I moved it to BrowserWindowController's unit test, even
though the change is in its superclass, to honor the original intent: browser
windows should use Auto Layout.

+// Auto Layout behave differently in subtle ways, we shouldn't start/stop using
+// it accidentally. If we don't want Auto Layout, this test should be changed
+// to expect constraints.count == 0, not disabled.
+TEST_F(ChromeBrowserWindowTest, UsesAutoLayout) {
+  base::scoped_nsobject<NSView> view(

[tapted, 2017/03/21 02:16:51]

Comment why the extra subview is necessary for the test?

[Sidney San Martín, 2017/03/22 21:40:22]

Gone.

+      [[NSView alloc] initWithFrame:NSMakeRect(0, 0, 10, 10)]);
+  [view setAutoresizingMask:NSViewWidthSizable];
+  [window_.contentView addSubview:view];

[tapted, 2017/03/21 02:16:51]

nit: [[window contentView] addSubview: view]

[Sidney San Martín, 2017/03/22 21:40:22]

Gone.

+  [window_ orderFront:nil];
+  [window_ layoutIfNeeded];
+  EXPECT_TRUE([[[window_ contentView] constraints] count] > 0);

[tapted, 2017/03/21 02:16:50]

nit:

EXPECT_LT(0u, [[[window_ contentView] constraints] count]);

[Sidney San Martín, 2017/03/22 21:40:21]

Oof, `EXPECT_LT(0u, count)` seems harder to understand. Since it's unsigned, how
about `EXPECT_NE(0u, count)`?

[tapted, 2017/03/22 22:40:00]

looks good!

+}