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

chrome/browser/ui/cocoa/browser_window_controller_unittest.mm

Index: chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
diff --git a/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm b/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
index b99bc4776b91e7cd8aa3e1728c937b3a302c5118..c54041603318e6cbf1b7c85f0f20ecd86ae7cf9e 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
@@ -164,6 +164,29 @@ TEST_F(BrowserWindowControllerTest, TestSetBounds) {
   [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, TestSetBoundsWithDownloadsShelf) {
+  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();
+  NSWindow* cocoaWindow = browser_window->GetNativeWindow();

[tapted, 2017/03/10 07:22:28]

nit: cocoa_window

(I realise other TEST_F use cocoaWindow but I use the "between @foo/@end" rule
to pick camelCase, and the style should at lease be consistent with
browser_window above)

alternatively (perhaps better) I guess

BrowserWindowController* controller =
    [BrowserWindowController
browserWindowControllerForWindow:browser_window->GetNativeWindow()];

will avoid the static_cast as well as the temporary :)

[Sidney San Martín, 2017/03/10 22:23:41]

See below.

+  BrowserWindowController* controller =
+      static_cast<BrowserWindowController*>([cocoaWindow windowController]);
+
+  [controller createAndAddDownloadShelf];

[tapted, 2017/03/10 07:22:28]

or perhaps a better alternative, you could just

EXPECT_TRUE(browser_window->GetDownloadShelf());

which guarantees a createAndAddDownloadShelf is called.

Also do we want something like

EXPECT_TRUE(browser_window=>IsDownloadShelfVisible()); ?

[Sidney San Martín, 2017/03/10 22:23:42]

That's much nicer, thanks!
Good idea. I added an EXPECT_FALSE — this bug only happens when the download
shelf is created without being shown.

+
+  browser_window->SetBounds(gfx::Rect(0, 0, 50, 50));

[tapted, 2017/03/10 07:22:28]

Is this subject to autolayout constraints?

If it is, you could comment before; something like

// BrowserWindowCocoa::SetBounds() uses -enforceMinWindowSize: to ensure the
bounds
// satisfy the window's autolayout constraints.

But -enforceMinWindowSize: just asks for the NSWindow's min/max size properties
-- I'm not sure (and think:not) whether those properties account for layout
calculations.

Then SetBounds just uses [NSWindow setFrame:] which I think doesn't apply
constraints the same way as a user-initiated resize.

E.g. does the test still pass with [self layoutSubviews]; removed?

Maybe there's a method you can call directly on
browser_window->GetNativeWindow() which sets the frame size subject to layout
constraints?

[Sidney San Martín, 2017/03/10 22:23:42]

[NSWindow setFrame:] marks the window as needing layout, and the next layout
pass will definitely adjust the size of the window to satisfy constraints.

Something interesting: SetBounds calls -[NSWindow setFrame:… display:YES]. When
linking against the 10.10 SDK, the display:YES causes layout to happen
immediately, and potentially change the window's frame. But, when linking
against a newer SDK, it waits until the current CATransaction commits. In a test
app, I can initially read back the frame as I set it, then it changes after the
commit. I added a preemptive -layoutIfNeeded and comment.
The test passes now, but it won't pass with Auto Layout in effect. I added a
demo patch set that just includes the test, with a dependency on another CL that
turns on Auto Layout, and ran it on a trybot so that you can see it fail.

+  gfx::Rect bounds = browser_window->GetBounds();
+  EXPECT_EQ(400, bounds.width());
+  EXPECT_EQ(272, bounds.height());

[tapted, 2017/03/10 07:22:28]

400 / 272 are magic numbers. At the very least they need to be drawn up into a
constant at the top of the test file and shared with other tests.

But a lot more appropriate would be to declare something in
browser_window_controller.h so that -[BrowserWindowController
initWithBrowser:..] can use them as well.

[Sidney San Martín, 2017/03/10 22:23:41]

I was following the surrounding tests here, but that makes sense. Thoughts on
the approach in the latest patch set?

+
+  [controller close];
+}
+
 TEST_F(BrowserWindowControllerTest, TestSetBoundsPopup) {
   // Create a popup with bounds smaller than the minimum.
   Browser::CreateParams params(Browser::TYPE_POPUP, profile(), true);