[Mac] Refactor bookmark pulsing into BookmarkBubbleObserverCocoa.

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm

Index: chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
index c540d72f54bb2f4701b877e084dc905253b913eb..6a6356ba665ba927725d92f45e66edd168c4c5d1 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
@@ -33,6 +33,7 @@
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_folder_target.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.h"
+#import "chrome/browser/ui/cocoa/bookmarks/bookmark_model_observer_for_cocoa.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/menu_button.h"
@@ -283,10 +284,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
                       selector:@selector(themeDidChangeNotification:)
                           name:kBrowserThemeDidChangeNotification
                         object:nil];
-    [defaultCenter addObserver:self
-                      selector:@selector(pulseBookmarkNotification:)
-                          name:bookmark_button::kPulseBookmarkButtonNotification
-                        object:nil];
 
     contextMenuController_.reset(
         [[BookmarkContextMenuCocoaController alloc]
@@ -308,45 +305,59 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   return contextMenuController_.get();
 }
 
-- (void)pulseBookmarkNotification:(NSNotification*)notification {
-  NSDictionary* dict = [notification userInfo];
-  const BookmarkNode* node = NULL;
-  NSValue *value = [dict objectForKey:bookmark_button::kBookmarkKey];
-  DCHECK(value);
-  if (value)
-    node = static_cast<const BookmarkNode*>([value pointerValue]);
-  NSNumber* number = [dict objectForKey:bookmark_button::kBookmarkPulseFlagKey];
-  DCHECK(number);
-  BOOL doPulse = number ? [number boolValue] : NO;
-
-  // 3 cases:
-  // button on the bar: flash it
-  // button in "other bookmarks" folder: flash other bookmarks
-  // button in "off the side" folder: flash the chevron
-  for (BookmarkButton* button in [self buttons]) {
-    if ([button bookmarkNode] == node) {
-      [button setIsContinuousPulsing:doPulse];
-      return;
+- (BookmarkButton*)bookmarkButtonForNode:(const BookmarkNode*)node {

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

declare this in BookmarkBarController(Private)? - probably next to buttonForNode
(should probably have a comment, and the definition may need to move to keep the
order consistent)

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

Added declaration. The order is pretty inconsistent in this file, so I think it
makes more sense to have it here above -startPulsingBookmarkNode.

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

Acknowledged.

+  // Find the closest parent that is visible on the bar.
+  BookmarkButton* result = nil;

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

I think it's more chromeystyle to omit `result` -- instead: `while (node)` and
e.g. `return button;` Then `return nil;` at the end (still preceded by the
NOTREACHED() perhaps).

Although I guess then then it falls into `no else after return` as well, so the
`else if` would become regular `if`, and then they could lose their curlies too.

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

Done.

+  while (node && !result) {
+    // 3 cases:
+    // button on the bar: flash it
+    // button in "other bookmarks" folder: flash other bookmarks
+    // button in "off the side" folder: flash the chevron
+    if (node->parent() == bookmarkModel_->bookmark_bar_node()) {
+      for (BookmarkButton* button in [self buttons]) {
+        if ([button bookmarkNode] == node) {
+          [button setIsContinuousPulsing:YES];
+          result = button;
+        }
+      }
+    } else if ([managedBookmarksButton_ bookmarkNode] == node) {

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

a thought: would the managedBookmarksButton_'s parent ever be
`bookmarkModel_->bookmark_bar_node()`, potentially making this clause get
skipped since the first `if` is true?

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

I don't think so, since it works with the otherBookmarksButton_. I'm not sure
how to test it with managed/supervised. In any case, changed the order so that
it should be covered. Normal/other/off-the-side still work.

+      result = managedBookmarksButton_;
+    } else if ([supervisedBookmarksButton_ bookmarkNode] == node) {
+      result = supervisedBookmarksButton_;
+    } else if ([otherBookmarksButton_ bookmarkNode] == node) {
+      result = otherBookmarksButton_;
+    } else if (node->parent() == bookmarkModel_->bookmark_bar_node()) {
+      result = offTheSideButton_;
     }
+    node = node->parent();
   }
-  if ([managedBookmarksButton_ bookmarkNode] == node) {
-    [managedBookmarksButton_ setIsContinuousPulsing:doPulse];
-    return;
-  }
-  if ([supervisedBookmarksButton_ bookmarkNode] == node) {
-    [supervisedBookmarksButton_ setIsContinuousPulsing:doPulse];
-    return;
-  }
-  if ([otherBookmarksButton_ bookmarkNode] == node) {
-    [otherBookmarksButton_ setIsContinuousPulsing:doPulse];
+  return result;
+}
+
+- (void)startPulsingBookmarkNode:(const BookmarkNode*)node {
+  if (pulsingButton_)

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

Here maybe just start with

pulsingBookmarkObserver_.reset();

the destructor of the Cocoa observer can be responsible for calling
stopPulsingBookmarkNode - OR, if pulsingBookmarkObserver_ holds the
scoped_nsobject to the |pulsingButton_| instead of BookmarkBarController it
might would make
stopPulsingBookmarkNode even simpler: just pulsingBookmarkObserver_.reset(); --
pulsingBookmarkObserver_::~destroy could just call [setIsContinuousPulsing:NO],
and the ObjC block below wouldn't be needed

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

As per previous comment, I'd prefer keep |pulsingButton_| private in
BookmarkBarController.

+    [self stopPulsingBookmarkNode];
+
+  pulsingButton_ = [self bookmarkButtonForNode:node];
+  if (!pulsingButton_)
     return;
-  }
-  if (node->parent() == bookmarkModel_->bookmark_bar_node()) {
-    [offTheSideButton_ setIsContinuousPulsing:doPulse];
+
+  [pulsingButton_ setIsContinuousPulsing:YES];
+  pulsingBookmarkObserver_.reset(
+      new BookmarkModelObserverForCocoa(bookmarkModel_, ^(BOOL) {
+        // Stop pulsing if anything happened to the node.
+        [self stopPulsingBookmarkNode];
+      }));
+  pulsingBookmarkObserver_->StartObservingNode(node);
+}
+
+- (void)stopPulsingBookmarkNode {
+  if (!pulsingButton_)
     return;
-  }
 
-  NOTREACHED() << "no bookmark button found to pulse!";
+  [pulsingButton_ setIsContinuousPulsing:NO];
+  pulsingButton_ = nil;
+  pulsingBookmarkObserver_.reset();
 }
 
 - (void)dealloc {
@@ -1763,6 +1774,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
 // Delete all buttons (bookmarks, chevron, "other bookmarks") from the
 // bookmark bar; reset knowledge of bookmarks.
 - (void)clearBookmarkBar {
+  [self stopPulsingBookmarkNode];
   for (BookmarkButton* button in buttons_.get()) {
     [button setDelegate:nil];
     [button removeFromSuperview];
@@ -2322,6 +2334,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   id<BookmarkButtonControllerProtocol> parentController =
       [self controllerForNode:oldParent];
   [parentController removeButton:index animate:YES];
+
   // If we go from 1 --> 0 bookmarks we may need to show the
   // "bookmarks go here" text container.
   [self showOrHideNoItemContainerForNode:model->bookmark_bar_node()];