[Mac] Lay out the browser window when adding the download shelf.

chrome/browser/ui/cocoa/browser_window_controller_unittest.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 
 #include <memory>
 
 #include "base/mac/mac_util.h"
 #import "base/mac/scoped_nsobject.h"
@@ skipping 11 matching lines @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/bookmarks/common/bookmark_pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #import "testing/gtest_mac.h"
 #import "third_party/ocmock/OCMock/OCMock.h"
 #import "ui/base/test/scoped_fake_nswindow_fullscreen.h"
 
+namespace {
+const gfx::Size kMinTabbedWindowSize(

[tapted, 2017/03/13 00:25:36]

I'm pretty sure this will add a static initializer, which will get your patch
reverted once it hits the waterfall. (try declaring `constexpr` to know
beforehand if it's a compile-time constant).

You could do some tricks to make MinWindowSizeForBrowserType `constexpr` instead
of `inline` -- you wouldn't be able to call NSMakeFoo, bug gfx::Size has
constexpr constructors or you could use C-style initializers for `struct
NSSize`.

There's also a downside to the browser.h #include..

[Sidney San Martín, 2017/03/14 23:28:39]

Hmm, I came very close to making MinWindowSizeForBrowserType() constexpr
originally. I went with your suggestion to avoid the browser.h include.

+    MinWindowSizeForBrowserType(Browser::TYPE_TABBED));
+}  // namespace
+
 using ::testing::Return;
 
 @interface BrowserWindowController (JustForTesting)
 // Already defined in BWC.
 - (void)saveWindowPositionIfNeeded;
 - (void)layoutSubviews;
 @end
 
 @interface BrowserWindowController (ExposedForTesting)
 // Implementations are below.
@@ skipping 102 matching lines @@
   Browser::CreateParams params(Browser::TYPE_TABBED, profile(), true);
   params.initial_bounds = gfx::Rect(0, 0, 50, 50);
   Browser* browser = new Browser(params);
   NSWindow* cocoaWindow = browser->window()->GetNativeWindow();
   BrowserWindowController* controller =
     static_cast<BrowserWindowController*>([cocoaWindow windowController]);
 
   ASSERT_TRUE([controller isTabbedWindow]);
   BrowserWindow* browser_window = [controller browserWindow];
   EXPECT_EQ(browser_window, browser->window());
-  gfx::Rect bounds = browser_window->GetBounds();
-  EXPECT_EQ(400, bounds.width());
-  EXPECT_EQ(272, bounds.height());
+  EXPECT_EQ(browser_window->GetBounds().size(), kMinTabbedWindowSize);
 
   // Try to set the bounds smaller than the minimum.
   browser_window->SetBounds(gfx::Rect(0, 0, 50, 50));
-  bounds = browser_window->GetBounds();
-  EXPECT_EQ(400, bounds.width());
-  EXPECT_EQ(272, bounds.height());
+  EXPECT_EQ(browser_window->GetBounds().size(), kMinTabbedWindowSize);
 
   [controller close];
 }
 
+// https://crbug.com/667698 - When Auto Layout is in use, adding the download
+// shelf without ever showing it shouldn't prevent the window from being
+// resized to its minimum width.
+TEST_F(BrowserWindowControllerTest, TestSetBoundsWithDownloadShelf) {
+  Browser::CreateParams params(Browser::TYPE_TABBED, profile(), true);
+  params.initial_bounds = gfx::Rect(0, 0, 1000, 50);
+  Browser* browser = new Browser(params);
+  BrowserWindow* browser_window = browser->window();
+  browser_window->ShowInactive();
+
+  EXPECT_TRUE(browser_window->GetDownloadShelf());

[tapted, 2017/03/13 00:25:36]

nit: comment here. Something like

// Requesting the download shelf lazily creates and adds the views to the view
hierarchy, but doesn't make it visible.


But.. It would be nice to have some guarantee that the statement is "verified"
e.g. is there some call we can make to ensure the DownloadShelfView is non-nil,
and has a superview?

[Sidney San Martín, 2017/03/14 23:28:39]

Done.
It depends on what this test is testing. When I get back to working on the
download shelf, I very much want to not put it in the view hierarchy at all
unless it's visible. At that point, this test will fail. Should the test be
deleted? If the answer is yes, OK. If the answer is no, then IMHO this is a
regression test for a specific crbug and testing for those details just makes it
brittle. Thoughts?

I think those checks would look something like this:

    NSWindow* nativeWindow = browser_window->GetNativeWindow();
    NSView* downloadShelfView = [[[BrowserWindowController
        browserWindowControllerForWindow:nativeWindow] downloadShelf] view];
    EXPECT_TRUE([downloadShelfView window] == nativeWindow);
    EXPECT_FALSE(browser_window->IsDownloadShelfVisible());

[tapted, 2017/03/15 00:49:51]

My main thought is that testing layout regressions is super-hard and you are
already a champion for going this far :).

I think what you have is good.
Of these, I'd perhaps add just EXPECT_NE(nil,  downloadShelfView) or something
to that effect. I don't think that will break once the shelf is not in the view
hierarchy. That's effectively implied by GetDownloadShelf() returning non-null
(just a little more Cocoa-specific), so consider this optional.

[Sidney San Martín, 2017/03/15 03:13:39]

Aw, thanks tapted@.
I was looking for the best way to add this, and discovered that
BrowserWindowControllerTest already comes with a BrowserWindowController! I just
rewrote the test a little to use it, and to add this check. I confirmed that it
still passes with and without Auto Layout, and fails with Auto Layout and
without the fix. Mind taking another quick look?

+  EXPECT_FALSE(browser_window->IsDownloadShelfVisible());
+
+  browser_window->SetBounds(gfx::Rect(0, 0, 50, 50));
+
+  // When linking against an SDK >= 10.11, AppKit may lay out the window
+  // asynchronously (CFExecutableLinkedOnOrAfter check in -[NSThemeFrame
+  // handleSetFrameCommonRedisplay]). Do layout now instead.
+  [browser_window->GetNativeWindow() layoutIfNeeded];
+
+  EXPECT_EQ(browser_window->GetBounds().size(), kMinTabbedWindowSize);
+
+  browser_window->Close();
+}
+
 TEST_F(BrowserWindowControllerTest, TestSetBoundsPopup) {
   // Create a popup with bounds smaller than the minimum.
   Browser::CreateParams params(Browser::TYPE_POPUP, profile(), true);
   params.initial_bounds = gfx::Rect(0, 0, 50, 50);
   Browser* browser = new Browser(params);
   NSWindow* cocoaWindow = browser->window()->GetNativeWindow();
   BrowserWindowController* controller =
     static_cast<BrowserWindowController*>([cocoaWindow windowController]);
 
   ASSERT_FALSE([controller isTabbedWindow]);
@@ skipping 639 matching lines @@
       [[NSWindow alloc] initWithContentRect:NSMakeRect(0,0,400,400)
                                   styleMask:NSBorderlessWindowMask
                                     backing:NSBackingStoreBuffered
                                       defer:NO]);
   [[testFullscreenWindow_ contentView] setWantsLayer:YES];
   return testFullscreenWindow_.get();
 }
 @end
 
 /* TODO(???): test other methods of BrowserWindowController */