[Mac] Refactor bookmark pulsing into BookmarkBubbleObserverCocoa.

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.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/bookmarks/bookmark_bar_controller.h"
 
 #include "base/mac/bundle_locations.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/sys_string_conversions.h"
@@ skipping 15 matching lines @@
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_window.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_button.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_context_menu_cocoa_controller.h"
 #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"
 #import "chrome/browser/ui/cocoa/presentation_mode_controller.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
 #import "chrome/browser/ui/cocoa/view_id_util.h"
 #import "chrome/browser/ui/cocoa/view_resizer.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ skipping 230 matching lines @@
 
     innerContentAnimationsEnabled_ = YES;
     stateAnimationsEnabled_ = YES;
 
     // Register for theme changes, bookmark button pulsing, ...
     NSNotificationCenter* defaultCenter = [NSNotificationCenter defaultCenter];
     [defaultCenter addObserver:self
                       selector:@selector(themeDidChangeNotification:)
                           name:kBrowserThemeDidChangeNotification
                         object:nil];
-    [defaultCenter addObserver:self
-                      selector:@selector(pulseBookmarkNotification:)
-                          name:bookmark_button::kPulseBookmarkButtonNotification
-                        object:nil];
 
     contextMenuController_.reset(
         [[BookmarkContextMenuCocoaController alloc]
             initWithBookmarkBarController:self]);
 
     // This call triggers an -awakeFromNib, which builds the bar, which might
     // use |folderImage_| and |contextMenuController_|. Ensure it happens after
     // |folderImage_| is loaded and |contextMenuController_| is created.
     [[self animatableView] setResizeDelegate:resizeDelegate];
   }
   return self;
 }
 
 - (Browser*)browser {
   return browser_;
 }
 
 - (BookmarkContextMenuCocoaController*)menuController {
   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;
+- (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();
+  }
+  return 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
-  for (BookmarkButton* button in [self buttons]) {
-    if ([button bookmarkNode] == node) {
-      [button setIsContinuousPulsing:doPulse];
-      return;
-    }
-  }
-  if ([managedBookmarksButton_ bookmarkNode] == node) {
-    [managedBookmarksButton_ setIsContinuousPulsing:doPulse];
+- (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 ([supervisedBookmarksButton_ bookmarkNode] == node) {
-    [supervisedBookmarksButton_ 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;
-  }
-  if ([otherBookmarksButton_ bookmarkNode] == node) {
-    [otherBookmarksButton_ setIsContinuousPulsing:doPulse];
-    return;
-  }
-  if (node->parent() == bookmarkModel_->bookmark_bar_node()) {
-    [offTheSideButton_ setIsContinuousPulsing:doPulse];
-    return;
-  }
 
-  NOTREACHED() << "no bookmark button found to pulse!";
+  [pulsingButton_ setIsContinuousPulsing:NO];
+  pulsingButton_ = nil;
+  pulsingBookmarkObserver_.reset();
 }
 
 - (void)dealloc {
   [self browserWillBeDestroyed];
   [super dealloc];
 }
 
 - (void)browserWillBeDestroyed {
   // Clear delegate so it doesn't get called during stopAnimation.
   [[self animatableView] setResizeDelegate:nil];
@@ skipping 1396 matching lines @@
   return folderTarget_.get();
 }
 
 - (int)displayedButtonCount {
   return displayedButtonCount_;
 }
 
 // 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];
   }
   [buttons_ removeAllObjects];
   [self clearMenuTagMap];
   displayedButtonCount_ = 0;
 
   // Make sure there are no stale pointers in the pasteboard.  This
   // can be important if a bookmark is deleted (via bookmark sync)
@@ skipping 539 matching lines @@
 - (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 =
       [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()];
   // If we deleted the only item on the "off the side" menu we no
   // longer need to show it.
   [self reconfigureBookmarkBar];
 }
 
 // TODO(jrg): linear searching is bad.
 // Need a BookmarkNode-->NSCell mapping.
@@ skipping 574 matching lines @@
 - (id<BookmarkButtonControllerProtocol>)controllerForNode:
     (const BookmarkNode*)node {
   // See if it's in the bar, then if it is in the hierarchy of visible
   // folder menus.
   if (bookmarkModel_->bookmark_bar_node() == node)
     return self;
   return [folderController_ controllerForNode:node];
 }
 
 @end