Don't animate the download shelf when entering/exiting fullscreen.

chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm

 // Copyright 2013 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/download/download_shelf_controller.h"
 
 #import <Cocoa/Cocoa.h>
 
 #include <memory>
 #include <utility>
 
 #import "base/mac/scoped_block.h"
 #import "base/mac/scoped_nsobject.h"
-#include "base/run_loop.h"
 #include "chrome/browser/download/download_shelf.h"
 #import "chrome/browser/ui/cocoa/download/download_item_controller.h"
 #include "chrome/browser/ui/cocoa/test/cocoa_profile_test.h"
 #import "chrome/browser/ui/cocoa/view_resizer_pong.h"
 #include "content/public/test/mock_download_item.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 #import "third_party/ocmock/OCMock/OCMock.h"
 #import "third_party/ocmock/gtest_support.h"
 #import "ui/events/test/cocoa_test_event_utils.h"
@@ skipping 139 matching lines @@
      andReturn:wrappedMockDownload.get()] wrappedMockDownload];
   return [item_controller retain];
 }
 
 TEST_VIEW(DownloadShelfControllerTest, [shelf_ view]);
 
 // Removing the last download from the shelf should cause it to close
 // immediately.
 TEST_F(DownloadShelfControllerTest, AddAndRemoveDownload) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   EXPECT_TRUE([shelf_ bridge]->IsShowing());
   [shelf_ add:item];
   [shelf_ remove:item];
   EXPECT_FALSE([shelf_ isVisible]);
   EXPECT_FALSE([shelf_ bridge]->IsShowing());
   // The shelf should be closed without scheduling an autoClose.
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
 }
 
 // Test that the shelf doesn't close automatically after a removal if there are
 // active download items still on the shelf.
 TEST_F(DownloadShelfControllerTest, AddAndRemoveWithActiveItem) {
   base::scoped_nsobject<DownloadItemController> item1(CreateItemController());
   base::scoped_nsobject<DownloadItemController> item2(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item1.get()];
   [shelf_ add:item2.get()];
   [shelf_ remove:item1.get()];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ remove:item2.get()];
   EXPECT_FALSE([shelf_ isVisible]);
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
 }
 
 // DownloadShelf::Unhide() should cause the shelf to be displayed if there are
 // active downloads on it.
 TEST_F(DownloadShelfControllerTest, HideAndUnhide) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
   [shelf_ bridge]->Hide();
   EXPECT_FALSE([shelf_ isVisible]);
   [shelf_ bridge]->Unhide();
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ remove:item.get()];
   EXPECT_FALSE([shelf_ isVisible]);
 }
 
 // DownloadShelf::Unhide() shouldn't cause the shelf to be displayed if all
 // active downloads are removed from the shelf while the shelf was hidden.
 TEST_F(DownloadShelfControllerTest, HideAutocloseUnhide) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
   [shelf_ bridge]->Hide();
   EXPECT_FALSE([shelf_ isVisible]);
   [shelf_ remove:item.get()];
   EXPECT_FALSE([shelf_ isVisible]);
   [shelf_ bridge]->Unhide();
   EXPECT_FALSE([shelf_ isVisible]);
 }
 
 // Test of autoclosing behavior after opening a download item. The mouse is on
 // the download shelf at the time the autoclose is scheduled.
 TEST_F(DownloadShelfControllerTest, AutoCloseAfterOpenWithMouseInShelf) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
   // Expect 2 cancelAutoClose calls: From the showDownloadShelf: call and the
   // add: call.
   EXPECT_EQ(2, shelf_.get()->cancelAutoCloseCount_);
   shelf_.get()->cancelAutoCloseCount_ = 0;
 
   // The mouse enters the shelf.
   [shelf_ mouseEntered:cocoa_test_event_utils::EnterEvent()];
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
@@ skipping 15 matching lines @@
 
   // The mouse enters the shelf again. The autoClose should be cancelled.
   [shelf_ mouseEntered:cocoa_test_event_utils::EnterEvent()];
   EXPECT_EQ(1, shelf_.get()->scheduleAutoCloseCount_);
   EXPECT_EQ(1, shelf_.get()->cancelAutoCloseCount_);
 }
 
 // Test of autoclosing behavior after opening a download item.
 TEST_F(DownloadShelfControllerTest, AutoCloseAfterOpenWithMouseOffShelf) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
 
   // The download is opened.
   EXPECT_CALL(*[[item wrappedMockDownload] mockDownload], GetOpened())
       .WillRepeatedly(Return(true));
   [shelf_ downloadWasOpened:item.get()];
 
   // The shelf should be closed immediately since the mouse is not over the
   // shelf.
   EXPECT_FALSE([shelf_ isVisible]);
 }
 
 // Test that if the shelf is closed while an autoClose is pending, the pending
 // autoClose is cancelled.
 TEST_F(DownloadShelfControllerTest, CloseWithPendingAutoClose) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
   // Expect 2 cancelAutoClose calls: From the showDownloadShelf: call and the
   // add: call.
   EXPECT_EQ(2, shelf_.get()->cancelAutoCloseCount_);
   shelf_.get()->cancelAutoCloseCount_ = 0;
 
   // The mouse enters the shelf.
   [shelf_ mouseEntered:cocoa_test_event_utils::EnterEvent()];
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
@@ skipping 19 matching lines @@
   [shelf_ remove:item];
   EXPECT_EQ(1, shelf_.get()->scheduleAutoCloseCount_);
   EXPECT_EQ(1, shelf_.get()->cancelAutoCloseCount_);
   EXPECT_FALSE([shelf_ isVisible]);
 }
 
 // That that the shelf cancels a pending autoClose if a new download item is
 // added to it.
 TEST_F(DownloadShelfControllerTest, AddItemWithPendingAutoClose) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
   // Expect 2 cancelAutoClose calls: From the showDownloadShelf: call and the
   // add: call.
   EXPECT_EQ(2, shelf_.get()->cancelAutoCloseCount_);
   shelf_.get()->cancelAutoCloseCount_ = 0;
 
   // The mouse enters the shelf.
   [shelf_ mouseEntered:cocoa_test_event_utils::EnterEvent()];
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
@@ skipping 18 matching lines @@
   base::scoped_nsobject<DownloadItemController> item2(CreateItemController());
   [shelf_ add:item.get()];
   EXPECT_EQ(1, shelf_.get()->scheduleAutoCloseCount_);
   EXPECT_EQ(1, shelf_.get()->cancelAutoCloseCount_);
   EXPECT_TRUE([shelf_ isVisible]);
 }
 
 // Test that pending autoClose calls are cancelled when exiting.
 TEST_F(DownloadShelfControllerTest, CancelAutoCloseOnExit) {
   base::scoped_nsobject<DownloadItemController> item(CreateItemController());
-  [shelf_ showDownloadShelf:YES
-               isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_TRUE([shelf_ isVisible]);
   [shelf_ add:item.get()];
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
   EXPECT_EQ(2, shelf_.get()->cancelAutoCloseCount_);
 
   [shelf_ browserWillBeDestroyed];
   EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
   EXPECT_EQ(3, shelf_.get()->cancelAutoCloseCount_);
   shelf_.reset();
 }
 
-// The view should not be hidden when the shelf is shown.
-// The view should be hidden after the closing animation.
-// Failing flakily on Mac 10.9, see: crbug.com/687447.
-TEST_F(DownloadShelfControllerTest, DISABLED_ViewVisibility) {
-  [shelf_ showDownloadShelf:YES isUserAction:NO];
+// The view should not be hidden when the shelf is open.
+// The view should be hidden when the shelf is closed.
+TEST_F(DownloadShelfControllerTest, ViewVisibility) {
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_FALSE([[shelf_ view] isHidden]);
 
-  base::RunLoop run_loop;
-  base::RunLoop* const run_loop_ptr = &run_loop;
-
-  [shelf_ setCloseAnimationHandler:^{
-    run_loop_ptr->Quit();
-  }];
-  [shelf_ showDownloadShelf:NO isUserAction:NO];
-  run_loop.Run();
+  [shelf_ showDownloadShelf:NO isUserAction:NO animate:NO];
   EXPECT_TRUE([[shelf_ view] isHidden]);
 
-  [shelf_ showDownloadShelf:YES isUserAction:NO];
+  [shelf_ showDownloadShelf:YES isUserAction:NO animate:NO];
   EXPECT_FALSE([[shelf_ view] isHidden]);
 }

[Nico, 2017/02/17 15:53:46]

i think we have 0 coverage for animate:YES

[Sidney San Martín, 2017/02/17 16:07:08]

This is true. Unfortunately the only test which actually tested animation (this
one) was disabled due to flakiness.

Not sure what the right answer is: I could keep the old behavior in the other
tests with animate:YES (but they didn't really exercise it), and leave this test
disabled, or maybe there's a different kind of test I could add that I'm not
considering?

[Nico, 2017/02/17 16:13:09]

:-/
I think there's some value in running code even if the test doesn't really check
the result of that code, in that ASan and whatnot will at least instrument and
execute the code and check for invalid writes and leaks, even if we don't test
for correctness. But up to you.

 
 }  // namespace