[Mac] Hover/Active Omnibox Icon States for Secondary UI MD

chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm

Index: chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm
diff --git a/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm b/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm
index fae302ed9a08ee6ed24f19e951e183453b4b5c3e..869f6c50bb381b491e14a761134c8abd82e261d3 100644
--- a/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm
+++ b/chrome/browser/ui/cocoa/bubble_anchor_helper_views.mm
@@ -7,6 +7,11 @@
 #import <Cocoa/Cocoa.h>
 
 #import "base/mac/scoped_nsobject.h"
+#import "chrome/browser/ui/cocoa/browser_window_controller.h"
+#import "chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h"
+#import "chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h"
+#import "chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h"
+#import "chrome/browser/ui/cocoa/location_bar/star_decoration.h"
 #include "ui/views/bubble/bubble_dialog_delegate.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -17,7 +22,8 @@ namespace {
 // Widget closes.
 class BubbleAnchorHelper : public views::WidgetObserver {
  public:
-  explicit BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble);
+  explicit BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble,

[tapted, 2017/05/16 07:39:37]

nit: explicit not required

[spqchan, 2017/05/17 01:39:39]

Done.

+                              LocationBarDecoration* decoration);
 
  private:
   // Observe |name| on the bubble parent window with a block to call ReAnchor().
@@ -40,21 +46,54 @@ class BubbleAnchorHelper : public views::WidgetObserver {
   CGFloat horizontal_offset_;  // Offset from the left or right.
   CGFloat vertical_offset_;    // Offset from the top.
 
+  // The omnibox decoration that |bubble_| is anchored to.
+  LocationBarDecoration* decoration_;
+
   DISALLOW_COPY_AND_ASSIGN(BubbleAnchorHelper);
 };
 
 }  // namespace
 
-void KeepBubbleAnchored(views::BubbleDialogDelegateView* bubble) {
-  new BubbleAnchorHelper(bubble);
+LocationBarDecoration* GetManagePasswordDecoration(
+    views::BubbleDialogDelegateView* bubble) {
+  LocationBarViewMac* locationBarBridge =

[tapted, 2017/05/16 07:39:37]

nit: locationBarBridge -> location_bar  (camel case only for new declarations
between @foo/@end)

[spqchan, 2017/05/17 01:39:39]

Done.

+      [[[bubble->parent_window() window] windowController] locationBarBridge];

[tapted, 2017/05/16 07:39:37]

I don't think this will compile in the 10.11 or 10.12 SDK.

[NSWindow windowController] used to be declared to return `id`, but now it's
NSWindowController*, which won't have locationBarBridge declared on it.

I think we need a helper method like

LocationBarForBubbleParent(..) which uses

+[BrowserWindowController browserWindowControllerForWindow]

(and also checks if that returned nil)

[spqchan, 2017/05/17 01:39:39]

Done.

+  return locationBarBridge ? locationBarBridge->manage_passwords_decoration()
+                           : nullptr;
+}
+
+LocationBarDecoration* GetStarDecoration(
+    views::BubbleDialogDelegateView* bubble) {
+  LocationBarViewMac* locationBarBridge =
+      [[[bubble->parent_window() window] windowController] locationBarBridge];
+  return locationBarBridge ? locationBarBridge->star_decoration() : nullptr;
+}
+
+LocationBarDecoration* GetPageInfoDecoration(
+    views::BubbleDialogDelegateView* bubble) {
+  LocationBarViewMac* locationBarBridge =
+      [[[bubble->parent_window() window] windowController] locationBarBridge];
+  return locationBarBridge ? locationBarBridge->GetPageInfoDecoration()
+                           : nullptr;
+}
+
+void KeepBubbleAnchored(views::BubbleDialogDelegateView* bubble,
+                        LocationBarDecoration* decoration) {
+  new BubbleAnchorHelper(bubble, decoration);
 }
 
-BubbleAnchorHelper::BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble)
-    : observer_tokens_([[NSMutableArray alloc] init]), bubble_(bubble) {
+BubbleAnchorHelper::BubbleAnchorHelper(views::BubbleDialogDelegateView* bubble,
+                                       LocationBarDecoration* decoration)
+    : observer_tokens_([[NSMutableArray alloc] init]),
+      bubble_(bubble),
+      decoration_(decoration) {
   DCHECK(bubble->GetWidget());
   DCHECK(bubble->parent_window());
   bubble->GetWidget()->AddObserver(this);
 
+  if (decoration_)
+    decoration_->SetActive(true);
+
   NSRect parent_frame = [[bubble->parent_window() window] frame];
   NSRect bubble_frame = [bubble->GetWidget()->GetNativeWindow() frame];
 
@@ -102,6 +141,8 @@ void BubbleAnchorHelper::ReAnchor() {
 }
 
 void BubbleAnchorHelper::OnWidgetDestroying(views::Widget* widget) {
+  if (decoration_)

[tapted, 2017/05/16 07:39:37]

This raises a new lifetime issue that didn't previously exist -
BubbleAnchorHelper::OnWidgetDestroying needs a guarantee that |decoration_|
hasn't been deleted.

I *think* WidgetOwnerNSWindowAdapter provides this guarantee, but we should put
an ownership comment up around line 50. E.g. 

// Weak. Lifetime is tied to the controller of bubble's parent window, which
owns a LocationBarViewMac that directly owns |decoration_|.

And here, a comment like,

// NativeWidgetMac guarantees that a child's OnWidgetDestroying() is invoked
before the parent NSWindow has closed.

[spqchan, 2017/05/17 01:39:39]

Done. Are there any other steps that we could perhaps take to make sure this is
guaranteed?

+    decoration_->SetActive(false);
   widget->RemoveObserver(this);
   for (id token in observer_tokens_.get())
     [[NSNotificationCenter defaultCenter] removeObserver:token];