[Mac] Fix handling of download shelf auto close.

If the mouse is not on the download shelf when an auto-close is
scheduled, the shelf would not close automatically. Fix this by
closing immediately if the mouse isn't on the shelf.

Also adds unit-tests and update the shelf to use CrTrackingArea
instead of NSTrackingArea.

BUG=none
TEST=(1) Start a download that opens automatically.
     (2) Move the mouse off the shelf and wait for completion.
     (3) The shelf should close automatically.

Review URL: https://chromiumcodereview.appspot.com/13318002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@195080 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/cocoa/download/download_shelf_controller.h

@@ -6,6 +6,7 @@
 
 #include "base/memory/scoped_nsobject.h"
 #include "base/memory/scoped_ptr.h"
+#include "ui/base/cocoa/tracking_area.h"
 #import "chrome/browser/ui/cocoa/view_resizer.h"
 
 @class AnimatableView;
@@ -43,10 +44,20 @@ class PageNavigator;
  @private
   IBOutlet HoverButton* hoverCloseButton_;
 
+  // YES if the download shelf is intended to be displayed. The shelf animates
+  // out when it is closing. During this time, barIsVisible_ is NO although the
+  // shelf is still visible on screen.
   BOOL barIsVisible_;
 
+  // YES if the containing browser window is fullscreen.
   BOOL isFullscreen_;
 
+  // YES if the shelf should be closed when the mouse leaves the shelf.
+  BOOL shouldCloseOnMouseExit_;
+
+  // YES if the mouse is currently over the download shelf.
+  BOOL isMouseInsideView_;
+
   scoped_ptr<DownloadShelf> bridge_;
 
   // Height of the shelf when it's fully visible.
@@ -56,10 +67,8 @@ class PageNavigator;
   // out.
   CGFloat currentShelfHeight_;
 
-  // Used to autoclose the shelf when the mouse is moved off it.  Is non-nil
-  // only when a subsequent mouseExited event can trigger autoclose or when a
-  // subsequent mouseEntered event will cancel autoclose.  Is nil otherwise.
-  scoped_nsobject<NSTrackingArea> trackingArea_;
+  // Used to autoclose the shelf when the mouse is moved off it.
+  ui::ScopedCrTrackingArea trackingArea_;
 
   // The download items we have added to our shelf.
   scoped_nsobject<NSMutableArray> downloadItemControllers_;
@@ -102,6 +111,9 @@ class PageNavigator;
 // item should not have been already added to this shelf.
 - (void)addDownloadItem:(content::DownloadItem*)downloadItem;
 
+// Similar to addDownloadItem above, but adds a DownloadItemController.
+- (void)add:(DownloadItemController*)download;
+
 // Remove a download, possibly via clearing browser data.
 - (void)remove:(DownloadItemController*)download;
 

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

@@ -31,7 +31,7 @@ using content::DownloadItem;
 // Download shelf autoclose behavior:
 //
 // The download shelf autocloses if all of this is true:
-// 1) An item on the shelf has just been opened.
+// 1) An item on the shelf has just been opened or removed.
 // 2) All remaining items on the shelf have been opened in the past.
 // 3) The mouse leaves the shelf and remains off the shelf for 5 seconds.
 //
@@ -71,13 +71,17 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
 }  // namespace
 
 @interface DownloadShelfController(Private)
+- (void)removeDownload:(DownloadItemController*)download
+        isShelfClosing:(BOOL)isShelfClosing;
 - (void)layoutItems:(BOOL)skipFirst;
 - (void)closed;
 - (void)maybeAutoCloseAfterDelay;
+- (void)scheduleAutoClose;
+- (void)cancelAutoClose;
 - (void)autoClose;
 - (void)viewFrameDidChange:(NSNotification*)notification;
 - (void)installTrackingArea;
-- (void)cancelAutoCloseAndRemoveTrackingArea;
+- (void)removeTrackingArea;
 - (void)willEnterFullscreen;
 - (void)willLeaveFullscreen;
 - (void)updateCloseButton;
@@ -136,11 +140,12 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
                     selector:@selector(willLeaveFullscreen)
                         name:kWillLeaveFullscreenNotification
                       object:nil];
+  [self installTrackingArea];
 }
 
 - (void)dealloc {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
-  [self cancelAutoCloseAndRemoveTrackingArea];
+  [self removeTrackingArea];
 
   // The controllers will unregister themselves as observers when they are
   // deallocated. No need to do that here.
@@ -177,6 +182,12 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
 }
 
 - (void)remove:(DownloadItemController*)download {
+  [self removeDownload:download
+        isShelfClosing:NO];
+}
+
+- (void)removeDownload:(DownloadItemController*)download
+        isShelfClosing:(BOOL)isShelfClosing {
   // Look for the download in our controller array and remove it. This will
   // explicity release it so that it removes itself as an Observer of the
   // DownloadItem. We don't want to wait for autorelease since the DownloadItem
@@ -187,11 +198,14 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
   [[download view] removeFromSuperview];
 
   [downloadItemControllers_ removeObject:download];
-  [self layoutItems];
 
-  // If there are no more downloads or if all the remaining downloads have been
-  // opened, we can close the shelf.
-  [self maybeAutoCloseAfterDelay];
+  if (!isShelfClosing) {
+    [self layoutItems];
+
+    // If there are no more downloads or if all the remaining downloads have
+    // been opened, we can close the shelf.
+    [self maybeAutoCloseAfterDelay];
+  }
 }
 
 - (void)downloadWasOpened:(DownloadItemController*)item_controller {
@@ -204,17 +218,22 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
 // to remove themselves as observers before the remaining shutdown happens.
 - (void)exiting {
   [[self animatableView] stopAnimation];
-  [self cancelAutoCloseAndRemoveTrackingArea];
+  [self removeTrackingArea];
+  while ([downloadItemControllers_ count] > 0) {
+    [self removeDownload:[downloadItemControllers_ lastObject]
+          isShelfClosing:YES];
+  }
   downloadItemControllers_.reset();
 }
 
 - (void)showDownloadShelf:(BOOL)show
              isUserAction:(BOOL)isUserAction {
+  shouldCloseOnMouseExit_ = NO;
+
   if ([self isVisible] == show)
     return;
 
   if (!show) {
-    [self cancelAutoCloseAndRemoveTrackingArea];
     int numInProgress = 0;
     for (NSUInteger i = 0; i < [downloadItemControllers_ count]; ++i) {
       if ([[downloadItemControllers_ objectAtIndex:i]download]->IsInProgress())
@@ -277,16 +296,20 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
 
 - (void)addDownloadItem:(DownloadItem*)downloadItem {
   DCHECK([NSThread isMainThread]);
-  [self cancelAutoCloseAndRemoveTrackingArea];
-
-  // Insert new item at the left.
   scoped_nsobject<DownloadItemController> controller(
       [[DownloadItemController alloc] initWithDownload:downloadItem
                                                  shelf:self
                                              navigator:navigator_]);
+  [self add:controller.get()];
+}
+
+- (void)add:(DownloadItemController*)controller {
+  DCHECK([NSThread isMainThread]);
+  shouldCloseOnMouseExit_ = NO;
 
+  // Insert new item at the left.
   // Adding at index 0 in NSMutableArrays is O(1).
-  [downloadItemControllers_ insertObject:controller.get() atIndex:0];
+  [downloadItemControllers_ insertObject:controller atIndex:0];
 
   [itemContainerView_ addSubview:[controller view]];
 
@@ -324,7 +347,8 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
     // Since no user will ever see the item being removed (needs a horizontal
     // screen resolution greater than 3200 at 16 items at 200 pixels each),
     // there's no point in animating the removal.
-    [self remove:[downloadItemControllers_ lastObject]];
+    [self removeDownload:[downloadItemControllers_ lastObject]
+          isShelfClosing:NO];
   }
 
   // Finally, move the remaining items to the right. Skip the first item when
@@ -347,7 +371,8 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
                           download->IsCancelled() ||
                           download->IsInterrupted();
     if (isTransferDone && !download->IsDangerous()) {
-      [self remove:itemController];
+      [self removeDownload:itemController
+            isShelfClosing:YES];
     } else {
       // Treat the item as opened when we close. This way if we get shown again
       // the user need not open this item for the shelf to auto-close.
@@ -358,16 +383,24 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
 }
 
 - (void)mouseEntered:(NSEvent*)event {
+  isMouseInsideView_ = YES;
   // If the mouse re-enters the download shelf, cancel the auto-close.  Further
-  // mouse exits should not trigger autoclose, so also remove the tracking area.
-  [self cancelAutoCloseAndRemoveTrackingArea];
+  // mouse exits should not trigger autoclose.
+  if (shouldCloseOnMouseExit_) {
+    [self cancelAutoClose];
+    shouldCloseOnMouseExit_ = NO;
+  }
 }
 
 - (void)mouseExited:(NSEvent*)event {
+  isMouseInsideView_ = NO;
+  if (shouldCloseOnMouseExit_)
+    [self scheduleAutoClose];
+}
+
+- (void)scheduleAutoClose {
   // Cancel any previous hide requests, just to be safe.
-  [NSObject cancelPreviousPerformRequestsWithTarget:self
-                                           selector:@selector(autoClose)
-                                             object:nil];
+  [self cancelAutoClose];
 
   // Schedule an autoclose after a delay.  If the mouse is moved back into the
   // view, or if an item is added to the shelf, the timer will be canceled.
@@ -376,6 +409,12 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
              afterDelay:kAutoCloseDelaySeconds];
 }
 
+- (void)cancelAutoClose {
+  [NSObject cancelPreviousPerformRequestsWithTarget:self
+                                           selector:@selector(autoClose)
+                                             object:nil];
+}
+
 - (void)maybeAutoCloseAfterDelay {
   // We can close the shelf automatically if all the downloads on the shelf have
   // been opened.
@@ -386,14 +425,12 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
       return;
   }
 
-  if ([self isVisible] && [downloadItemControllers_ count] > 0) {
-    // If the shelf is visible and has download items remaining on it, close the
-    // shelf after the user moves the mouse out of the download shelf. Note that
-    // the mouse might not be over the shelf. In this case, the shelf will not
-    // auto close.
-    // TODO(asanka): Don't install a tracking area if the mouse isn't over the
-    // shelf. Autoclose instead.
-    [self installTrackingArea];
+  if ([self isVisible] && [downloadItemControllers_ count] > 0 &&
+      isMouseInsideView_) {
+    // If there are download items on the shelf and the user is potentially stil
+    // interacting with them, schedule an auto close after the user moves the
+    // mouse off the shelf.
+    shouldCloseOnMouseExit_ = YES;
   } else {
     // We notify the DownloadShelf of our intention to close even if the shelf
     // is currently hidden. If the shelf was temporarily hidden (e.g. because
@@ -408,28 +445,23 @@ const NSSize kHoverCloseButtonDefaultSize = { 18, 18 };
 }
 
 - (void)installTrackingArea {
-  // Install the tracking area to listen for mouseExited messages and trigger
-  // the shelf autoclose.
-  if (trackingArea_.get())
-    return;
+  // Install the tracking area to listen for mouseEntered and mouseExited
+  // messages.
+  DCHECK(!trackingArea_.get());
 
-  trackingArea_.reset([[NSTrackingArea alloc]
+  trackingArea_.reset([[CrTrackingArea alloc]
                         initWithRect:[[self view] bounds]
                              options:NSTrackingMouseEnteredAndExited |
                                      NSTrackingActiveAlways |
                                      NSTrackingInVisibleRect
                                owner:self
                             userInfo:nil]);
-  [[self view] addTrackingArea:trackingArea_];
+  [[self view] addTrackingArea:trackingArea_.get()];
 }
 
-- (void)cancelAutoCloseAndRemoveTrackingArea {
-  [NSObject cancelPreviousPerformRequestsWithTarget:self
-                                           selector:@selector(autoClose)
-                                             object:nil];
-
+- (void)removeTrackingArea {
   if (trackingArea_.get()) {
-    [[self view] removeTrackingArea:trackingArea_];
+    [[self view] removeTrackingArea:trackingArea_.get()];
     trackingArea_.reset(nil);
   }
 }

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 <Cocoa/Cocoa.h>
+
+#import "base/memory/scoped_nsobject.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/download/download_shelf.h"
+#include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
+#import "chrome/browser/ui/cocoa/download/download_item_controller.h"
+#import "chrome/browser/ui/cocoa/download/download_shelf_controller.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"
+
+using ::testing::Return;
+using ::testing::AnyNumber;
+
+// Wraps a content::MockDownloadItem so it can be retained by the mock
+// DownloadItemController.
+@interface WrappedMockDownloadItem : NSObject {
+ @private
+  scoped_ptr<content::MockDownloadItem> download_;
+}
+- (id)initWithMockDownload:(scoped_ptr<content::MockDownloadItem>)download;
+- (content::DownloadItem*)download;
+- (content::MockDownloadItem*)mockDownload;
+@end
+
+@implementation WrappedMockDownloadItem
+- (id)initWithMockDownload:(scoped_ptr<content::MockDownloadItem>)download {
+  if ((self = [super init])) {
+    download_ = download.Pass();
+  }
+  return self;
+}
+
+- (content::DownloadItem*)download {
+  return download_.get();
+}
+
+- (content::MockDownloadItem*)mockDownload {
+  return download_.get();
+}
+@end
+
+// Test method for accessing the wrapped MockDownloadItem.
+@interface DownloadItemController (DownloadShelfControllerTest) {
+}
+- (WrappedMockDownloadItem*)wrappedMockDownload;
+@end
+
+@implementation DownloadItemController (DownloadShelfControllerTest)
+- (WrappedMockDownloadItem*)wrappedMockDownload {
+  return nil;
+}
+@end
+
+// Subclass of the DownloadShelfController to override scheduleAutoClose and
+// cancelAutoClose. During regular operation, a scheduled autoClose waits for 5
+// seconds before closing the shelf (unless it is cancelled during this
+// time). For testing purposes, we count the number of invocations of
+// {schedule,cancel}AutoClose instead of actually scheduling and cancelling.
+@interface CountingDownloadShelfController : DownloadShelfController {
+ @public
+  int scheduleAutoCloseCount_;
+  int cancelAutoCloseCount_;
+}
+@end
+
+@implementation CountingDownloadShelfController
+
+-(void)scheduleAutoClose {
+  ++scheduleAutoCloseCount_;
+}
+
+-(void)cancelAutoClose {
+  ++cancelAutoCloseCount_;
+}
+@end
+
+namespace {
+
+class DownloadShelfControllerTest : public CocoaProfileTest {
+ public:
+  virtual void SetUp() OVERRIDE {
+    CocoaProfileTest::SetUp();
+    ASSERT_TRUE(browser());
+
+    resize_delegate_.reset([[ViewResizerPong alloc] init]);
+    shelf_.reset([[CountingDownloadShelfController alloc]
+                   initWithBrowser:browser()
+                    resizeDelegate:resize_delegate_.get()]);
+    EXPECT_TRUE([shelf_ view]);
+    [[test_window() contentView] addSubview:[shelf_ view]];
+  }
+
+  virtual void TearDown() OVERRIDE {
+    [shelf_ exiting];
+    shelf_.reset();
+    CocoaProfileTest::TearDown();
+  }
+
+ protected:
+  id CreateItemController();
+
+  scoped_nsobject<CountingDownloadShelfController> shelf_;
+  scoped_nsobject<ViewResizerPong> resize_delegate_;
+};
+
+id DownloadShelfControllerTest::CreateItemController() {
+  scoped_ptr<content::MockDownloadItem> download(
+      new ::testing::NiceMock<content::MockDownloadItem>);
+  ON_CALL(*download.get(), GetOpened())
+      .WillByDefault(Return(false));
+  ON_CALL(*download.get(), IsInProgress())
+      .WillByDefault(Return(true));
+
+  scoped_nsobject<WrappedMockDownloadItem> wrappedMockDownload(
+      [[WrappedMockDownloadItem alloc] initWithMockDownload:download.Pass()]);
+
+  id item_controller =
+      [OCMockObject mockForClass:[DownloadItemController class]];
+  scoped_nsobject<NSView> view([[NSView alloc] initWithFrame:NSZeroRect]);
+  [[[item_controller stub] andCall:@selector(download)
+                          onObject:wrappedMockDownload.get()] download];
+  [[item_controller stub] updateVisibility:[OCMArg any]];
+  [[[item_controller stub]
+     andReturnValue:[NSValue valueWithSize:NSMakeSize(10,10)]] preferredSize];
+  [[[item_controller stub] andReturn:view.get()] view];
+  [[[item_controller stub]
+     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) {
+  scoped_nsobject<DownloadItemController> item(CreateItemController());
+  [shelf_ showDownloadShelf:YES
+               isUserAction: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) {
+  scoped_nsobject<DownloadItemController> item1(CreateItemController());
+  scoped_nsobject<DownloadItemController> item2(CreateItemController());
+  [shelf_ showDownloadShelf:YES
+               isUserAction: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) {
+  scoped_nsobject<DownloadItemController> item(CreateItemController());
+  [shelf_ showDownloadShelf:YES
+               isUserAction: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) {
+  scoped_nsobject<DownloadItemController> item(CreateItemController());
+  [shelf_ showDownloadShelf:YES
+               isUserAction: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) {
+  scoped_nsobject<DownloadItemController> item(CreateItemController());
+  [shelf_ showDownloadShelf:YES
+               isUserAction:NO];
+  EXPECT_TRUE([shelf_ isVisible]);
+  [shelf_ add:item.get()];
+
+  // The mouse enters the shelf.
+  [shelf_ mouseEntered:nil];
+  EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
+
+  // The download opens.
+  EXPECT_CALL(*[[item wrappedMockDownload] mockDownload], GetOpened())
+      .WillRepeatedly(Return(true));
+  [shelf_ downloadWasOpened:item.get()];
+
+  // The shelf should now be waiting for the mouse to exit. autoClose should not
+  // be scheduled yet.
+  EXPECT_TRUE([shelf_ isVisible]);
+  EXPECT_EQ(0, shelf_.get()->scheduleAutoCloseCount_);
+
+  // The mouse exits the shelf. autoClose should be scheduled now.
+  [shelf_ mouseExited:nil];
+  EXPECT_EQ(1, shelf_.get()->scheduleAutoCloseCount_);
+  EXPECT_EQ(0, shelf_.get()->cancelAutoCloseCount_);
+
+  // The mouse enters the shelf again. The autoClose should be cancelled.
+  [shelf_ mouseEntered:nil];
+  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) {
+  scoped_nsobject<DownloadItemController> item(CreateItemController());
+  [shelf_ showDownloadShelf:YES
+               isUserAction: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]);
+}
+
+}  // namespace

chrome/chrome_tests_unit.gypi

@@ -1292,6 +1292,7 @@
         'browser/ui/cocoa/custom_frame_view_unittest.mm',
         'browser/ui/cocoa/download/download_item_button_unittest.mm',
         'browser/ui/cocoa/download/download_item_cell_unittest.mm',
+        'browser/ui/cocoa/download/download_shelf_controller_unittest.mm',
         'browser/ui/cocoa/download/download_shelf_mac_unittest.mm',
         'browser/ui/cocoa/download/download_shelf_view_unittest.mm',
         'browser/ui/cocoa/download/download_util_mac_unittest.mm',