Implement sidebar support for extension action popups

chrome/browser/ui/extensions/extension_action_view_controller.cc

Index: chrome/browser/ui/extensions/extension_action_view_controller.cc
diff --git a/chrome/browser/ui/extensions/extension_action_view_controller.cc b/chrome/browser/ui/extensions/extension_action_view_controller.cc
index 182a90eeab6ae47308477d7300ccf35e6b55d19c..e28980892ebf77d6b66e1cfc1f3af9e3ff34c6e0 100644
--- a/chrome/browser/ui/extensions/extension_action_view_controller.cc
+++ b/chrome/browser/ui/extensions/extension_action_view_controller.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/extensions/extension_action_view_controller.h"
 
 #include "base/logging.h"
+#include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
@@ -12,15 +13,19 @@
 #include "chrome/browser/extensions/extension_view.h"
 #include "chrome/browser/extensions/extension_view_host.h"
 #include "chrome/browser/extensions/extension_view_host_factory.h"
+#include "chrome/browser/extensions/sidebar_container.h"
+#include "chrome/browser/extensions/sidebar_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/extensions/accelerator_priority.h"
 #include "chrome/browser/ui/extensions/extension_action_platform_delegate.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_delegate.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
-#include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
+#include "chrome/common/pref_names.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
@@ -58,6 +63,15 @@ ExtensionActionViewController::ExtensionActionViewController(
 
 ExtensionActionViewController::~ExtensionActionViewController() {
   DCHECK(!is_showing_popup());
+
+  extensions::SidebarManager* sidebar_manager =
+      extensions::SidebarManager::GetFromContext(browser_->profile());
+
+  sidebar_manager->RemoveObserver(this);

[Devlin, 2015/06/19 19:56:10]

ScopedObserver

[ltilve, 2015/06/28 22:44:20]

Done.

+  for (std::set<content::WebContents*>::iterator it =
+           active_in_webcontents_.begin();
+       it != active_in_webcontents_.end(); ++it)
+    sidebar_manager->HideSidebar(*it);
 }
 
 const std::string& ExtensionActionViewController::GetId() const {
@@ -304,10 +318,12 @@ bool ExtensionActionViewController::TriggerPopupWithUrl(
     return false;
 
   bool already_showing = is_showing_popup();
+  bool use_sidebar = extension_action_->open_in_sidebar();
 
   // Always hide the current popup, even if it's not owned by this extension.
   // Only one popup should be visible at a time.
-  HideActivePopup();
+  if (!use_sidebar)

[Devlin, 2015/06/19 19:56:10]

This raises the question of if a popup and a sidebar can be showing at the same
time.  If the answer is yes, then we need to restructure the idea of a popup
owner, since you're setting the popup owner here even if it's a sidebar, but
only closing the current one if it's not.

[ltilve, 2015/06/28 22:44:20]

Done, popups and sidebars should not be showing at the same time. You were
totally right about the problems of ownership, the behaviour should be the same
for any case, this is, closing the former sidebar/popup.

[Devlin, 2015/07/07 21:39:29]

Why don't you think popups and sidebars should be able to both be present?  I
think it's probably okay, since sidebars are designed to be persistent
information added to a page.  I think it would be annoying to have a sidebar I
liked, but then be afraid to activate another popup because it closes that
sidebar... and it would be even weirder for the case of "pinned" sidebars
(though we'll still have to address something similar there when a new sidebar
opens).

[ltilve, 2015/07/09 22:15:50]

Done. I had misunderstood the ownership issue. We are allowing again opening an
extension popup without closing the visible sidebar. About the popup owner, as
sidebarManager owns the sidebars, we don't need to assign it to the
toolbarActionsBar, so we are not setting it any more if it's a sidebar. 

+    HideActivePopup();
 
   // If we were showing a popup already, then we treat the action to open the
   // same one as a desire to close it (like clicking a menu button that was
@@ -315,6 +331,27 @@ bool ExtensionActionViewController::TriggerPopupWithUrl(
   if (already_showing)
     return false;
 
+  if (use_sidebar) {
+    extensions::SidebarManager* sidebar_manager =
+        extensions::SidebarManager::GetFromContext(browser_->profile());
+
+    content::WebContents* web_contents =
+        view_delegate_->GetCurrentWebContents();
+
+    extensions::SidebarContainer* sidebar =
+        sidebar_manager->GetSidebarContainerFor(web_contents);
+    if (sidebar && sidebar->extension_id() == GetId()) {
+      sidebar_manager->HideSidebar(web_contents);
+      return false;
+    }
+
+    active_in_webcontents_.insert(web_contents);
+    sidebar_manager->AddObserver(this);
+
+    sidebar_manager->CreateSidebar(web_contents, popup_url, browser_);
+    return true;

[Devlin, 2015/06/19 19:56:10]

I think this should still handle sliding out the action if it's overflowed, if
we're going to try and have attribution.

[ltilve, 2015/06/28 22:44:20]

Done. After the previous change, the attribution problem that could happen with
overflowed popups and sidebars is apparently fixed.

+  }
+
   scoped_ptr<extensions::ExtensionViewHost> host(
       extensions::ExtensionViewHostFactory::CreatePopupHost(popup_url,
                                                             browser_));
@@ -368,3 +405,69 @@ void ExtensionActionViewController::OnPopupClosed() {
   }
   view_delegate_->OnPopupClosed();
 }
+
+void ExtensionActionViewController::OnPopupShown(bool grant_tab_permissions) {
+  view_delegate_->OnPopupShown(grant_tab_permissions);
+}
+
+void ExtensionActionViewController::OnSidebarHidden(
+    content::WebContents* tab,
+    const std::string& content_id) {
+  if (view_delegate_->GetCurrentWebContents() == tab && content_id == GetId()) {
+    view_delegate_->OnPopupClosed();
+    active_in_webcontents_.erase(
+        active_in_webcontents_.find(view_delegate_->GetCurrentWebContents()));
+    if (active_in_webcontents_.size() == 0) {
+      extensions::SidebarManager::GetFromContext(browser_->profile())
+          ->RemoveObserver(this);
+
+      if (toolbar_actions_bar_) {
+        toolbar_actions_bar_->SetPopupOwner(nullptr);
+        if (toolbar_actions_bar_->popped_out_action() == this &&
+            !view_delegate_->IsMenuRunning())
+          toolbar_actions_bar_->UndoPopOut();
+      }
+    }
+  }
+}
+
+void ExtensionActionViewController::OnSidebarSwitched(
+    content::WebContents* old_tab,
+    const std::string& old_content_id,
+    content::WebContents* new_tab,
+    const std::string& new_content_id) {
+  if (browser_->tab_strip_model()->GetIndexOfWebContents(
+          new_tab ? new_tab : old_tab) == TabStripModel::kNoTab)
+    return;
+
+  if (old_content_id == GetId() && old_content_id != new_content_id) {

[Devlin, 2015/06/19 19:56:10]

Under which circumstances would a sidebar be switched to one with the same id?

[ltilve, 2015/06/28 22:44:20]

Done. That code was refactored into a clearer UpdateButtonState function.

+    if (toolbar_actions_bar_) {
+      toolbar_actions_bar_->SetPopupOwner(nullptr);
+      if (toolbar_actions_bar_->popped_out_action() == this &&
+          !view_delegate_->IsMenuRunning())
+        toolbar_actions_bar_->UndoPopOut();
+    }
+    view_delegate_->OnPopupClosed();
+  }
+
+  if (view_delegate_->GetCurrentWebContents() == new_tab &&
+      new_content_id == GetId() && old_content_id != new_content_id) {
+    if (toolbar_actions_bar_)
+      toolbar_actions_bar_->SetPopupOwner(this);
+    if (toolbar_actions_bar_ && !toolbar_actions_bar_->IsActionVisible(this) &&
+        extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
+      platform_delegate_->CloseOverflowMenu();
+      toolbar_actions_bar_->PopOutAction(
+          this, base::Bind(&ExtensionActionViewController::OnPopupShown,
+                           weak_factory_.GetWeakPtr(), true));
+    } else {
+      // Without the popup corner arrow indicator, marking the browserAction
+      // icon
+      // is necessary for extension attribution
+      view_delegate_->OnPopupShown(true);
+    }
+  }
+
+  if (old_tab == new_tab && old_content_id == GetId())
+    OnSidebarHidden(old_tab, old_content_id);
+}