[Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility()

chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm

Index: chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm
diff --git a/chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm b/chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm
index 1eeffaed319c00f982ce2e01dc2f6f1a1b578789..6fe72afbda57d69431151f30f40522b2bc0566b6 100644
--- a/chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm
@@ -17,6 +17,7 @@
 #include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
 #import "chrome/browser/ui/cocoa/image_button_cell.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
+#import "chrome/browser/ui/cocoa/view_resizer_pong.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -78,6 +79,8 @@ class ToolbarControllerTest : public CocoaProfileTest {
     CocoaProfileTest::SetUp();
     ASSERT_TRUE(browser());
 
+    resizeDelegate_.reset([[ViewResizerPong alloc] init]);
+
     CommandUpdater* updater =
         browser()->command_controller()->command_updater();
     // The default state for the commands is true, set a couple to false to
@@ -87,7 +90,8 @@ class ToolbarControllerTest : public CocoaProfileTest {
     bar_.reset([[TestToolbarController alloc]
         initWithCommands:browser()->command_controller()->command_updater()
                  profile:profile()
-                 browser:browser()]);
+                 browser:browser()
+          resizeDelegate:resizeDelegate_.get()]);
     EXPECT_TRUE([bar_ view]);
     NSView* parent = [test_window() contentView];
     [parent addSubview:[bar_ view]];
@@ -111,6 +115,7 @@ class ToolbarControllerTest : public CocoaProfileTest {
               [[views objectAtIndex:kHomeIndex] isEnabled] ? true : false);
   }
 
+  base::scoped_nsobject<ViewResizerPong> resizeDelegate_;
   base::scoped_nsobject<TestToolbarController> bar_;
 };
 
@@ -127,7 +132,7 @@ TEST_F(ToolbarControllerTest, TitlebarOnly) {
   NSView* view = [bar_ view];
 
   [bar_ setHasToolbar:NO hasLocationBar:YES];
-  EXPECT_NE(view, [bar_ view]);
+  EXPECT_EQ(view, [bar_ view]);
 
   // Simulate a popup going fullscreen and back by performing the reparenting

[tapted, 2015/10/20 06:31:09]

Make sure you test this to ensure it still behaves

[dominickn, 2015/10/26 02:53:04]

This should now be unaffected by the showLocationBarOnly call; the only change
is that it's the toolbarView being used, not locationBarView.

[tapted, 2015/10/26 04:40:55]

So did you open site that does HTML5 fullscreen in a popup, and ask it to go
fullscreen?

The issue is that there used to be code around this to resolve ownership
problems that came about with the weird locationView hack. The hierarchy is now
changed, so ownership patters could break.

[dominickn, 2015/10/26 06:42:42]

I tested a site with HTML5 fullscreen in a popup and it seemed to work the same
as without the patch.

   // that happens during fullscreen transitions
@@ -142,12 +147,58 @@ TEST_F(ToolbarControllerTest, TitlebarOnly) {
   [bar_ setHasToolbar:NO hasLocationBar:YES];
 }
 
+// Test updateVisibility with location bar only; this method is used by bookmark
+// apps, and should never be called when the toolbar is enabled. Ensure that the
+// buttons remain in the correct state.
+TEST_F(ToolbarControllerTest, UpdateVisibility) {
+  NSView* view = [bar_ view];
+  [bar_ setHasToolbar:NO hasLocationBar:YES];

[tapted, 2015/10/20 06:31:09]

Before calling this, EXPECT_FOO the button visibility for the other states
(which *can* be returned from). I.e., do

setHasToolbar:YES hasLocationBar:YES
setHasToolbar:NO hasLocationBar:NO
setHasToolbar:NO hasLocationBar:YES

[dominickn, 2015/10/26 02:53:04]

Done.

[tapted, 2015/10/26 04:40:55]

Ping

[dominickn, 2015/10/26 06:42:42]

Oops. Done.

+  EXPECT_FALSE([view isHidden]);
+  EXPECT_FALSE([[[bar_ toolbarViews] objectAtIndex:kLocationIndex] isHidden]);

[tapted, 2015/10/20 06:31:09]

This is a lot of repetition. Since bar_ is a data member, I think nicest would
be a helper method on ToolbarControllerTest like

NSView* ToolbarControllerTest::GetSubviewAt(SubviewIndex index) {..}

(after giving the enum a name)

[dominickn, 2015/10/26 02:53:04]

Done.

+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kBackIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kForwardIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kReloadIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kWrenchIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]);
+
+  // Maintain visible state.
+  [bar_ updateVisibility:YES withAnimation:NO];
+  EXPECT_GT([[bar_ view] frame].size.height, 0);
+  EXPECT_FALSE([view isHidden]);
+  EXPECT_FALSE([[[bar_ toolbarViews] objectAtIndex:kLocationIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kBackIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kForwardIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kReloadIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kWrenchIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]);

[tapted, 2015/10/20 06:31:09]

what about the browser action container?

[dominickn, 2015/10/26 02:53:04]

I believe that for popup and bookmark app windows there is no browser action
controller.

[tapted, 2015/10/26 04:40:55]

Can we include it in the test?

[dominickn, 2015/10/26 06:42:42]

Done.

+
+  // Hide the toolbar and ensure it is not visible.
+  [bar_ updateVisibility:NO withAnimation:NO];
+  EXPECT_TRUE([view isHidden]);
+  EXPECT_EQ(0, [resizeDelegate_ height]);
+  EXPECT_EQ(0, [[bar_ view] frame].size.height);
+
+  // Try to show the home button.
+  [bar_ showOptionalHomeButton];
+
+  // Re-show the bar. Buttons should remain hidden, including the home button.
+  [bar_ updateVisibility:YES withAnimation:NO];
+  EXPECT_GT([resizeDelegate_ height], 0);
+  EXPECT_GT([[bar_ view] frame].size.height, 0);
+  EXPECT_FALSE([view isHidden]);
+  EXPECT_FALSE([[[bar_ toolbarViews] objectAtIndex:kLocationIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kBackIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kForwardIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kReloadIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kWrenchIndex] isHidden]);
+  EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]);
+}
+
 // Make sure it works in the completely undecorated case.
 TEST_F(ToolbarControllerTest, NoLocationBar) {
   NSView* view = [bar_ view];
 
   [bar_ setHasToolbar:NO hasLocationBar:NO];
-  EXPECT_NE(view, [bar_ view]);
   EXPECT_TRUE([[bar_ view] isHidden]);
 
   // Simulate a popup going fullscreen and back by performing the reparenting