Cater for the "double close" when clicking "Learn More" on auto-dismiss extension bubbles.

chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc

Index: chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
index 8842a43d252a97c2bfbc1c7029109a0e5aaec212..624106a496252f78c975ec7b3e76dac23790796f 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
@@ -101,7 +101,11 @@ bool ToolbarActionsBarBubbleViews::Accept() {
 }
 
 bool ToolbarActionsBarBubbleViews::Close() {
-  delegate_->OnBubbleClosed(close_reason_);
+  // When LinkClicked() calls OnBubbleClosed(), the Widget may close

[Devlin, 2017/04/19 15:04:22]

This sounds a bit off to me.  Maybe "delegate_->OnBubbleClosed() will be called
by LinkClicked() since the bubble may close automatically in response..."

[tapted, 2017/04/20 11:55:42]

Done.

+  // automatically in response to the delegate action (e.g. losing focus when
+  // a browser is raised).

[Devlin, 2017/04/19 15:04:22]

browser is raised -> tab is opened?

[tapted, 2017/04/20 11:55:42]

Done.

+  if (close_reason_ != ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE)

[Devlin, 2017/04/19 15:04:22]

close_reason_ is only set for LinkClicked(), which now (like Cancel and Accept)
calls OnBubbleClosed() directly.  Perhaps this would be better as a bool
closed_through_user_action_ or similar, and then here we check
if (!closed_through_user_action_)
  delgate_->OnBubbleClosed(CLOSE_DISMISS_DEACTIVATION);

WDYT?  Alternatively, we could set close_reason_ in the other methods to make
this slightly less weird, and then check if (close_reason_ ==
DIMISS_BY_DEACTIVATION), but that seems a little roundabout.

[tapted, 2017/04/20 11:55:41]

I went for a bool. Called it |delegate_notified_of_close_| .. I guess
technically it's "about_to_notifiy_delegate_of_close_" but that sounds a little
transient.

+    delegate_->OnBubbleClosed(close_reason_);
   return true;
 }
 
@@ -160,5 +164,12 @@ base::string16 ToolbarActionsBarBubbleViews::GetDialogButtonLabel(
 void ToolbarActionsBarBubbleViews::LinkClicked(views::Link* link,
                                                int event_flags) {
   close_reason_ = ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE;
+  // Force the delegate action to happen here rather than in Close() so that
+  // Close() can distinguish between the codepaths.

[Devlin, 2017/04/19 15:04:22]

I don't follow the "so that Close() can distinguish between the codepaths" part
- Close() distinguishes based on the close_reason_.  Maybe rephrase? 

[tapted, 2017/04/20 11:55:41]

I think this captures it:

  // Force the delegate action to happen here rather than in Close(). Close() is
  // called from Widget::CanClose() so attempting to open a link there can cause
  // recursion.

+  delegate_->OnBubbleClosed(close_reason_);
+  // Note that the Widget may or may not already be closed at this point,
+  // depending on delegate_->ShouldCloseOnDeactivate(). Widget::Close() protects
+  // against multiple calls (so long as they are not nested), and Widget
+  // destruction is asynchronous, so it is safe to call Close() again.
   GetWidget()->Close();
 }