diff options
4 files changed, 59 insertions, 4 deletions
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h index 29952b8..6b9e428 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller.h +++ b/chrome/browser/cocoa/bookmark_bar_controller.h @@ -334,7 +334,8 @@ willAnimateFromState:(bookmarks::VisualState)oldState - (BOOL)isEventAnExitEvent:(NSEvent*)event; - (id)folderTarget; - (int)displayedButtonCount; - +- (NSMenu*)buttonContextMenu; +- (void)setButtonContextMenu:(id)menu; @end // The (internal) |NSPasteboard| type string for bookmark button drags, used for diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm index ca8110c..bbb43d0 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller.mm +++ b/chrome/browser/cocoa/bookmark_bar_controller.mm @@ -549,6 +549,16 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12; return displayedButtonCount_; } +- (NSMenu*)buttonContextMenu { + return buttonContextMenu_; +} + +// Intentionally ignores ownership issues; used for testing and we try +// to minimize touching the object passed in (likely a mock). +- (void)setButtonContextMenu:(id)menu { + buttonContextMenu_ = menu; +} + // Keep the "no items" label centered in response to a frame size change. - (void)centerNoItemsLabel { // Note that this computation is done in the parent's coordinate system, @@ -1727,8 +1737,19 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) { [self configureOffTheSideButtonContentsAndVisibility]; } +// To avoid problems with sync, changes that may impact the current +// bookmark (e.g. deletion) make sure context menus are closed. This +// prevents deleting a node which no longer exists. +- (void)cancelMenuTracking { + [buttonContextMenu_ cancelTracking]; + [buttonFolderContextMenu_ cancelTracking]; +} + - (void)nodeAdded:(BookmarkModel*)model parent:(const BookmarkNode*)newParent index:(int)newIndex { + // If a context menu is open, close it. + [self cancelMenuTracking]; + const BookmarkNode* newNode = newParent->GetChild(newIndex); id<BookmarkButtonControllerProtocol> newController = [self controllerForNode:newParent]; @@ -1740,6 +1761,9 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) { - (void)nodeRemoved:(BookmarkModel*)model parent:(const BookmarkNode*)oldParent index:(int)index { + // If a context menu is open, close it. + [self cancelMenuTracking]; + // Locate the parent node. The parent may not be showing, in which case // we do nothing. id<BookmarkButtonControllerProtocol> parentController = diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm index a80b7e5..bb4e8b4 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm +++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm @@ -1329,6 +1329,32 @@ TEST_F(BookmarkBarControllerTest, NodeDeletedWhileMenuIsOpen) { [bar_ openAllBookmarksIncognitoWindow:item]; } +TEST_F(BookmarkBarControllerTest, NodeDeletedWhileContextMenuIsOpen) { + BookmarkModel* model = helper_.profile()->GetBookmarkModel(); + [bar_ loaded:model]; + + const BookmarkNode* parent = model->GetBookmarkBarNode(); + const BookmarkNode* folder = model->AddGroup(parent, + parent->GetChildCount(), + L"group"); + const BookmarkNode* framma = model->AddURL(folder, folder->GetChildCount(), + L"f1", + GURL("http://framma-lamma.com")); + + // Mock in a menu + id origMenu = [bar_ buttonContextMenu]; + id fakeMenu = [OCMockObject partialMockForObject:origMenu]; + [[fakeMenu expect] cancelTracking]; + [bar_ setButtonContextMenu:fakeMenu]; + + // Force a delete which should cancelTracking on the menu. + model->Remove(framma->GetParent(), framma->GetParent()->IndexOfChild(framma)); + + // Restore, then confirm cancelTracking was called. + [bar_ setButtonContextMenu:origMenu]; + [fakeMenu verify]; +} + TEST_F(BookmarkBarControllerTest, CloseFolderOnAnimate) { BookmarkModel* model = helper_.profile()->GetBookmarkModel(); const BookmarkNode* parent = model->GetBookmarkBarNode(); diff --git a/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h index 398b2b8..b5ba908 100644 --- a/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h +++ b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h @@ -33,6 +33,10 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver { // IBOutlet. The arg passed is nil. // Many notifications happen independently of node // (e.g. BeingDeleted), so |node| can be nil. + // + // |object| is NOT retained, since the expected use case is for + // ||object| to own the BookmarkModelObserverForCocoa and we don't + // want a retain cycle. BookmarkModelObserverForCocoa(const BookmarkNode* node, BookmarkModel* model, NSObject* object, @@ -40,7 +44,7 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver { DCHECK(model); node_ = node; model_ = model; - object_.reset([object retain]); + object_ = object; selector_ = selector; model_->AddObserver(this); } @@ -69,7 +73,7 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver { } virtual void BookmarkNodeChanged(BookmarkModel* model, const BookmarkNode* node) { - if (node_ == node) + if ((node_ == node) || (!node_)) Notify(); } virtual void BookmarkImportBeginning(BookmarkModel* model) { @@ -98,7 +102,7 @@ class BookmarkModelObserverForCocoa : public BookmarkModelObserver { private: const BookmarkNode* node_; // Weak; owned by a BookmarkModel. BookmarkModel* model_; // Weak; it is owned by a Profile. - scoped_nsobject<NSObject> object_; + NSObject* object_; // Weak, like a delegate. SEL selector_; void Notify() { |