Mac: Optimize HistoryOverlayView using layer.

chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm

 // Copyright (c) 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/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:
   void SetUp() override {
     CocoaTest::SetUp();
 
     // The overlay controller shows the panel as a subview of the given view.
     test_view_.reset([[NSView alloc] initWithFrame:NSMakeRect(10, 10, 10, 10)]);
 
     // We add it to the test_window for authenticity.
     [[test_window() contentView] addSubview:test_view_];
   }
 
   NSView* test_view() {
     return test_view_;
   }
 
  private:
   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]);
 
   base::scoped_nsobject<HistoryOverlayController> controller(
       [[HistoryOverlayController alloc] initForMode:kHistoryOverlayModeBack]);
   [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.

 }