[Mac] Enable MacViews bookmark bubble behind --enable-mac-views-dialogs.

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..c7e7bfc91a9a42357f603372dbd3765bf415d584 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -30,11 +30,13 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
+#include "chrome/browser/ui/bookmarks/bookmark_bubble_observer.h"
 #include "chrome/browser/ui/bookmarks/bookmark_editor.h"
 #include "chrome/browser/ui/bookmarks/bookmark_utils.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_instant_controller.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window_state.h"
@@ -95,6 +97,7 @@
 #import "ui/base/cocoa/nsview_additions.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/l10n_util_mac.h"
+#import "ui/gfx/mac/coordinate_conversion.h"
 #include "ui/gfx/mac/scoped_cocoa_disable_screen_updates.h"
 
 using bookmarks::BookmarkModel;
@@ -182,6 +185,49 @@ using content::Referrer;
 using content::RenderWidgetHostView;
 using content::WebContents;
 
+@interface BrowserWindowController ()
+// Declared for ViewsBookmarkBubbleObserver.
+- (void)viewsBookmarkBubbleClosed;
+@end
+
+namespace {
+
+// Manages toolbar visibility locking and bookmark button pulsing.
+class ViewsBookmarkBubbleObserver : public bookmarks::BookmarkBubbleObserver {

[tapted, 2015/08/21 07:19:18]

This doesn't feel right in this file, but I haven't come up with something
better yet.

One possibility might be to have the startPulsingBookmarkNode calls being done
in *Cocoa's* BookmarkBubbleController also use this observer interface (i.e.
they do FOR_EACH_OBSERVER instead). Then this Observer interface isn't
views-specfiic, and there is only one caller of start/stop pulsing.

Whatever is calling lockBarVisibility on Cocoa should somehow get looped in too.

And, either way, I think this class is complex enough to move to it's own file,
even if that means exposing some methods publicly

[jackhou1, 2015/08/24 08:22:04]

I decided to pull all the non-Views parts out to another CL:
https://codereview.chromium.org/1308293002/

+ public:
+  explicit ViewsBookmarkBubbleObserver(BrowserWindowController* controller)
+      : controller_(controller), lockOwner_([[NSObject alloc] init]) {}
+  ~ViewsBookmarkBubbleObserver() override {}
+
+  // bookmarks::BookmarkBubbleObserver.
+  void OnBookmarkBubbleShown(const BookmarkNode* node) override {
+    [controller_ lockBarVisibilityForOwner:lockOwner_
+                             withAnimation:NO
+                                     delay:NO];
+    pulsingBookmarkNode_ =
+        [[controller_ bookmarkBarController] startPulsingBookmarkNode:node];
+  }
+
+  void OnBookmarkBubbleHidden() override {
+    [controller_ releaseBarVisibilityForOwner:lockOwner_
+                                withAnimation:YES
+                                        delay:NO];
+    [[controller_ bookmarkBarController]
+        stopPulsingBookmarkNode:pulsingBookmarkNode_];
+    pulsingBookmarkNode_ = nullptr;
+    [controller_ viewsBookmarkBubbleClosed];
+  }
+
+ private:
+  BrowserWindowController* controller_;  // Weak, owns us.
+  const BookmarkNode* pulsingBookmarkNode_ = nullptr;
+  base::scoped_nsobject<NSObject> lockOwner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ViewsBookmarkBubbleObserver);
+};
+
+}  // namespace
+
 @interface NSWindow (NSPrivateApis)
 // Note: These functions are private, use -[NSObject respondsToSelector:]
 // before calling them.
@@ -1766,6 +1812,19 @@ using content::WebContents;
 // Show the bookmark bubble (e.g. user just clicked on the STAR).
 - (void)showBookmarkBubbleForURL:(const GURL&)url
                alreadyBookmarked:(BOOL)alreadyMarked {
+  if (chrome::ToolkitViewsDialogsEnabled()) {
+    if (viewsBookmarkBubbleObserver_.get())
+      return;
+
+    viewsBookmarkBubbleObserver_.reset(new ViewsBookmarkBubbleObserver(self));
+    chrome::ShowBookmarkBubbleViewsAtPoint(
+        gfx::ScreenPointFromNSPoint(
+            [[self window] convertBaseToScreen:[self bookmarkBubblePoint]]),
+        [[self window] contentView], viewsBookmarkBubbleObserver_.get(),
+        browser_.get(), url, alreadyMarked);
+    return;

[tapted, 2015/08/21 07:19:18]

DCHECK(viewsBookmarkBubbleObserver_) ?

[jackhou1, 2015/08/24 08:22:04]

Done. Is this to ensure it's not closed immediately?.

[tapted, 2015/08/25 03:10:16]

oops - missed a question. I guess that's part of it... Although TBH I didn't
really notice that the observer is being set just above, so the DCHECK is maybe
unnecessary - up to you if you want to keep it.

The point of having it is just to "bracket" the early exit case. That is, the
DCHECK basically adds the assertion "viewsBookmarkBubbleObserver_ is always
non-null when this method returns" -- this is clear for the early exit, and the
DCHECK does something similar for the later exit.

+  }
+
   if (!bookmarkBubbleController_) {
     BookmarkModel* model =
         BookmarkModelFactory::GetForProfile(browser_->profile());
@@ -1798,6 +1857,10 @@ using content::WebContents;
   bookmarkBubbleController_ = nil;
 }
 
+- (void)viewsBookmarkBubbleClosed {
+  viewsBookmarkBubbleObserver_.reset();
+}
+
 // Handle the editBookmarkNode: action sent from bookmark bubble controllers.
 - (void)editBookmarkNode:(id)sender {
   BOOL responds = [sender respondsToSelector:@selector(node)];