[Extensions Menu] Fix removal flow when popups are out

If an extension is displaying a popup (and is popped out from the
menu), we need to be careful about how it is deleted in order to
allow the UI to properly clean itself up. In particular, the
ExtensionActionViewController has to outlive the cleanup process,
undoing the popup.

Fix this, and add the ability to inspect a popup from a browsertest.

These changes, in combination with non-destructively repopulating the
menu, fix the BrowserActionInteractiveTests with the Extensions Menu.

Bug: 984654
Change-Id: I6cb9ea3ffb312a0fae9ee326eedb58b14c74d20d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003423
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734484}

chrome/browser/ui/views/extensions/extensions_menu_test_util.cc

@@ -7,6 +7,7 @@
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/extensions/extension_action_view_controller.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
 #include "chrome/browser/ui/views/extensions/extension_popup.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_button.h"
@@ -84,8 +85,10 @@ int ExtensionsMenuTestUtil::VisibleBrowserActions() {
 }
 
 void ExtensionsMenuTestUtil::InspectPopup(int index) {
-  // TODO(https://crbug.com/984654): Implement this.
-  NOTREACHED();
+  ExtensionsMenuItemView* view = GetMenuItemViewAtIndex(index);
+  DCHECK(view);
+  static_cast<ExtensionActionViewController*>(view->view_controller())
+      ->InspectPopup();
 }
 
 bool ExtensionsMenuTestUtil::HasIcon(int index) {

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -276,13 +276,19 @@ void ExtensionsToolbarContainer::OnToolbarActionRemoved(
   // could be handled inside the model and be invisible to the container when
   // permissions are unchanged.
 
-  // Delete the icon first so it unregisters it from the action.
+  auto iter = std::find_if(
+      actions_.begin(), actions_.end(),
+      [action_id](const auto& item) { return item->GetId() == action_id; });
+  DCHECK(iter != actions_.end());
+  // Ensure the action outlives the UI element to perform any cleanup.
+  std::unique_ptr<ToolbarActionViewController> controller = std::move(*iter);
+  actions_.erase(iter);
+  // Undo the popout, if necessary. Actions expect to not be popped out while
+  // destroying.
+  if (popped_out_action_ == controller.get())
+    UndoPopOut();
+
   icons_.erase(action_id);
-  base::EraseIf(
-      actions_,
-      [action_id](const std::unique_ptr<ToolbarActionViewController>& item) {
-        return item->GetId() == action_id;
-      });
 }
 
 void ExtensionsToolbarContainer::OnToolbarActionMoved(