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

 // Copyright 2016 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.
 
 #include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
 
 #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/grit/locale_settings.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
@@ skipping 83 matching lines @@
       ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION);
   return true;
 }
 
 bool ToolbarActionsBarBubbleViews::Accept() {
   delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_EXECUTE);
   return true;
 }
 
 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;
 }
 
 void ToolbarActionsBarBubbleViews::Init() {
   ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
   SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kVertical, 0, 0,
       provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL)));
 
   // Add the content string.
@@ skipping 38 matching lines @@
 
 base::string16 ToolbarActionsBarBubbleViews::GetDialogButtonLabel(
     ui::DialogButton button) const {
   return button == ui::DIALOG_BUTTON_OK ? delegate_->GetActionButtonText()
                                         : delegate_->GetDismissButtonText();
 }
 
 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();
 }