Mac: Optimize HistoryOverlayView using layer.

chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm

Index: chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm
diff --git a/chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm b/chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm
index d2224cf517a0ce430847453adf16d8667c3828f3..91db9b011a49ad6e4facc855cb27ac5a44788026 100644
--- a/chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm
@@ -4,13 +4,7 @@
 
 #import "chrome/browser/ui/cocoa/history_overlay_controller.h"
 
-#import <QuartzCore/QuartzCore.h>
-
-#include "base/memory/scoped_ptr.h"
-#include "base/message_loop/message_pump_mac.h"
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
-#import "third_party/ocmock/gtest_support.h"
-#import "third_party/ocmock/OCMock/OCMock.h"
 
 class HistoryOverlayControllerTest : public CocoaTest {
  public:
@@ -32,9 +26,7 @@ class HistoryOverlayControllerTest : public CocoaTest {
   base::scoped_nsobject<NSView> test_view_;
 };
 
-// Tests that when the controller is |-dismiss|ed, the animation runs and then
-// is removed when the animation completes. The view should be added and
-// removed at the appropriate times.
+// Tests that the overlay view gets added and removed at the appropriate times.
 TEST_F(HistoryOverlayControllerTest, DismissClearsAnimationsAndRemovesView) {
   EXPECT_EQ(0u, [[test_view() subviews] count]);
 
@@ -43,40 +35,8 @@ TEST_F(HistoryOverlayControllerTest, DismissClearsAnimationsAndRemovesView) {
   [controller showPanelForView:test_view()];
   EXPECT_EQ(1u, [[test_view() subviews] count]);
 
-  scoped_ptr<base::MessagePumpNSRunLoop> message_pump(
-      new base::MessagePumpNSRunLoop);
-
-  id mock = [OCMockObject partialMockForObject:controller];
-  [mock setExpectationOrderMatters:YES];
-  [[[mock expect] andForwardToRealObject] dismiss];
-
-  // Called after |-animationDidStop:finished:|.
-  base::MessagePumpNSRunLoop* weak_message_pump = message_pump.get();
-  void (^quit_loop)(NSInvocation* invocation) = ^(NSInvocation* invocation) {
-      weak_message_pump->Quit();
-  };
-  // Set up the mock to first forward to the real implementation and then call
-  // the above block to quit the run loop.
-  [[[[mock expect] andForwardToRealObject] andDo:quit_loop]
-      animationDidStop:[OCMArg isNotNil] finished:YES];
-
-  // CAAnimations must be committed within a run loop. It is not sufficient
-  // to commit them and activate the loop after the fact. Schedule a block to
-  // dismiss the controller from within the run loop, which begins the
-  // animation.
-  CFRunLoopPerformBlock(CFRunLoopGetCurrent(),
-                        kCFRunLoopDefaultMode,
-                        ^(void) {
-      [mock dismiss];
-  });
-
-  // Run the loop, which will dismiss the overlay.
-  message_pump->Run(NULL);
-
-  EXPECT_OCMOCK_VERIFY(mock);
-
-  // After the animation runs, there should be no more animations.
-  EXPECT_FALSE([[controller view] animations]);
-
+  // We expect the view to be removed from its superview immediately
+  // after dismiss, even though it may still be animating a fade out.
+  [controller dismiss];
   EXPECT_EQ(0u, [[test_view() subviews] count]);

[Robert Sesek, 2015/02/04 20:18:22]

How does this ensure that the animation is no longer running when the test ends
and test_view_ is deleted?

[Andre, 2015/02/04 21:01:46]

It does not.
Is there a benefit from waiting for the animation?
Or are you concerned that something may go wrong if we don't?
I think we have many tests that exits without flushing the run loop, after
closing a
window for example. Isn't this similar?
Do you have suggestions on what we should do instead?

[Robert Sesek, 2015/02/04 21:21:25]

Part of CocoaTest is responsible for flushing the run loop and closing open
windows, but it is a bug that many tests don't clean up after themselves. I'm
concerned that we have pending work in the runloop that could UAF an object and
cause the test to crash. Can you wait for the animation to complete by listening
for the notification?

[Andre, 2015/02/04 21:32:29]

Which notification do you mean?
NSAnimationContext's completionHandler is only available on 10.7 and later.
I don't think this animation actually involves the runloop much or at all, it's
not
done using a timer on the main thread.
I'm thinking we should do it the simple way and resort to something more
complicated only if the test turns out to be flaky. I'm hoping that it won't.

[Robert Sesek, 2015/02/05 21:48:03]

Sorry, I was referring to -animationDidStop:finished:, but you're right. Looking
at the animation code, since it doesn't set a delegate, this may not be an
issue. I'm still concerned that the animation may still be running when the test
ends, though. Hopefully the Valgrind/ASan bots will report any issues.

 }