[Mac] Refactor bookmark pulsing into BookmarkBubbleObserverCocoa.

chrome/browser/ui/cocoa/browser_window_controller.mm

Index: chrome/browser/ui/cocoa/browser_window_controller.mm
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.mm b/chrome/browser/ui/cocoa/browser_window_controller.mm
index b86492f79da7f30b1b6dd650a603f6e73c7dfe09..8f23806471d0e79e5c925be42d98b8c87805d377 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -40,6 +40,7 @@
 #include "chrome/browser/ui/browser_window_state.h"
 #import "chrome/browser/ui/cocoa/background_gradient_view.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h"
+#import "chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller.h"
 #import "chrome/browser/ui/cocoa/browser_window_cocoa.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller_private.h"
@@ -1766,36 +1767,30 @@ using content::WebContents;
 // Show the bookmark bubble (e.g. user just clicked on the STAR).
 - (void)showBookmarkBubbleForURL:(const GURL&)url
                alreadyBookmarked:(BOOL)alreadyMarked {
-  if (!bookmarkBubbleController_) {
+  if (!bookmarkBubbleObserver_.get()) {
+    bookmarkBubbleObserver_.reset(
+        new bookmarks::BookmarkBubbleObserverCocoa(self));
     BookmarkModel* model =
         BookmarkModelFactory::GetForProfile(browser_->profile());
     bookmarks::ManagedBookmarkService* managed =
         ManagedBookmarkServiceFactory::GetForProfile(browser_->profile());
     const BookmarkNode* node = model->GetMostRecentlyAddedUserNodeForURL(url);
-    bookmarkBubbleController_ =
-        [[BookmarkBubbleController alloc] initWithParentWindow:[self window]
-                                                       managed:managed
-                                                         model:model
-                                                          node:node
-                                             alreadyBookmarked:alreadyMarked];
+    bookmarkBubbleController_ = [[BookmarkBubbleController alloc]
+        initWithParentWindow:[self window]
+                    observer:bookmarkBubbleObserver_.get()

[tapted, 2015/08/25 01:13:21]

It might be nicer for the BookmarkBubbleObserverCocoa to alloc/init the
BookmarkBubbleController instead and pass |this| as the observer.


(Also this pattern looks more like a delegate than an observer.. but I guess
views is already like this so maybe it doesn't matter. Otherwise, it could call
AddObserver on the bubblecontroller (and BookmarkBubbleController has an
observer list rather than a single observer pointer).

[jackhou1, 2015/08/25 04:32:29]

Yeah, I went with this because it's the same as Views. Of your suggestions, I'd
prefer the AddObserver pattern, but I think it's simpler to leave this as is.
How strongly do you feel about it?

[tapted, 2015/08/25 05:46:34]

Not at all strongly - it's just an itch

+                     managed:managed
+                       model:model
+                        node:node
+           alreadyBookmarked:alreadyMarked];
     [bookmarkBubbleController_ showWindow:self];
-    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
-    [center addObserver:self
-               selector:@selector(bookmarkBubbleWindowWillClose:)
-                   name:NSWindowWillCloseNotification
-                 object:[bookmarkBubbleController_ window]];
+    DCHECK(bookmarkBubbleObserver_);
   }
 }
 
-// Nil out the weak bookmark bubble controller reference.
-- (void)bookmarkBubbleWindowWillClose:(NSNotification*)notification {
-  DCHECK_EQ([notification object], [bookmarkBubbleController_ window]);
-
-  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
-  [center removeObserver:self
-                    name:NSWindowWillCloseNotification
-                  object:[bookmarkBubbleController_ window]];
+- (void)bookmarkBubbleClosed {
+  // Nil out the weak bookmark bubble controller reference.
   bookmarkBubbleController_ = nil;

[tapted, 2015/08/25 01:13:21]

I think |bookmarkBubbleController_| can be removed from BrowserWindowController
completely now -- it's never read from, just set to nil AFAIK

[jackhou1, 2015/08/25 04:32:29]

It's used in browser_window_controller_private.mm.

[tapted, 2015/08/25 05:46:34]

Ah.. immersive fullscreen :/.  If it's just the [bookmarkBubbleController_
ok:self]; call, I *think* that's redundant, but I'm not confident.

I think "regular" fullscreen closes a bubble by way of -[BaseBubbleController
parentWindowWillToggleFullScreen:].

"immersive" fullscreen should close a bubble since a new window is created,
which should cause the bubble to lose key status and dismiss itself. Maybe that
doesn't happen. Anyway - something to explore in a follow-up perhaps.

+  bookmarkBubbleObserver_.reset();
 }
 
 // Handle the editBookmarkNode: action sent from bookmark bubble controllers.