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..298370f5d7318dcdb7fef12fcc2ec605f324acf7 100644
--- a/chrome/browser/ui/extensions/extension_action_view_controller.cc
+++ b/chrome/browser/ui/extensions/extension_action_view_controller.cc
@@ -17,10 +17,11 @@
 #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/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"
@@ -148,14 +149,15 @@ bool ExtensionActionViewController::HasPopup(
 }
 
 void ExtensionActionViewController::HidePopup() {
-  if (is_showing_popup()) {
-    popup_host_->Close();
-    // We need to do these actions synchronously (instead of closing and then
-    // performing the rest of the cleanup in OnExtensionHostDestroyed()) because
-    // the extension host can close asynchronously, and we need to keep the view
-    // delegate up-to-date.
-    OnPopupClosed();
-  }
+  if (!is_showing_popup())
+    return;
+
+  popup_host_->Close();
+  // We need to do these actions synchronously (instead of closing and then
+  // performing the rest of the cleanup in OnExtensionHostDestroyed()) because
+  // the extension host can close asynchronously, and we need to keep the view
+  // delegate up-to-date.
+  OnPopupClosed();
 }
 
 gfx::NativeView ExtensionActionViewController::GetPopupNativeView() {
@@ -215,8 +217,13 @@ bool ExtensionActionViewController::ExecuteAction(PopupShowAction show_action,
       ExtensionAction::ACTION_SHOW_POPUP) {
     GURL popup_url = extension_action_->GetPopupUrl(
         SessionTabHelper::IdForTab(view_delegate_->GetCurrentWebContents()));
-    return GetPreferredPopupViewController()
-        ->TriggerPopupWithUrl(show_action, popup_url, grant_tab_permissions);
+
+    if (extension_action_->open_in_sidebar())
+      return GetPreferredPopupViewController()->TriggerSidebarWithUrl(

[Devlin, 2015/07/30 17:56:46]

The fact that so much of this code seems to be copy-pasted with s/popup/sidebar
suggests it could be factored better...

[ltilve, 2015/08/07 16:15:02]

We need to handle a case which opens a popup when a sidebar is already shown.
For this case, we were thinking that maybe is more clear having separate methods
for sidebar and popups.

+          popup_url);
+    else
+      return GetPreferredPopupViewController()->TriggerPopupWithUrl(
+          show_action, popup_url, grant_tab_permissions);
   }
   return false;
 }
@@ -300,20 +307,16 @@ bool ExtensionActionViewController::TriggerPopupWithUrl(
     PopupShowAction show_action,
     const GURL& popup_url,
     bool grant_tab_permissions) {
-  if (!ExtensionIsValid())
-    return false;
-
-  bool already_showing = is_showing_popup();
 
   // 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 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
   // already open).
-  if (already_showing)
+  if (is_showing_popup()) {
+    HideActivePopup();
     return false;
+  }
 
   scoped_ptr<extensions::ExtensionViewHost> host(
       extensions::ExtensionViewHostFactory::CreatePopupHost(popup_url,
@@ -326,21 +329,32 @@ bool ExtensionActionViewController::TriggerPopupWithUrl(
   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::ShowPopup,
-                   weak_factory_.GetWeakPtr(),
-                   base::Passed(host.Pass()),
-                   grant_tab_permissions,
-                   show_action));
-  } else {
-    ShowPopup(host.Pass(), grant_tab_permissions, show_action);
+  PressButtonWithSlideOutIfEnabled(base::Bind(
+      &ExtensionActionViewController::ShowPopup, weak_factory_.GetWeakPtr(),
+      base::Passed(host.Pass()), grant_tab_permissions, show_action));
+
+  return true;
+}
+
+bool ExtensionActionViewController::TriggerSidebarWithUrl(
+    const GURL& popup_url) {
+  if (sidebar_container_) {
+    HideActiveSidebar();
+    return false;
   }
 
+  HideActiveSidebar();
+
+  content::WebContents* web_contents = view_delegate_->GetCurrentWebContents();
+  sidebar_container_.reset(
+      new extensions::SidebarContainer(browser_, web_contents, popup_url));
+
+  if (toolbar_actions_bar_)
+    toolbar_actions_bar_->SetSidebarOwner(this);
+
+  PressButtonWithSlideOutIfEnabled(

[Devlin, 2015/07/30 17:56:46]

Wait, what?  This calls into PressButtonWithSlideOutIfEnabled, but that method
ignores the case of the sidebar.  This means that if you enable the extension
action redesign, |closure| will never be run.

[ltilve, 2015/08/07 16:15:02]

Fixed. You were totally right, good catch.

+      base::Bind(&ExtensionActionViewController::PressButton,
+                 weak_factory_.GetWeakPtr(), true));
   return true;
 }
 
@@ -354,17 +368,67 @@ void ExtensionActionViewController::ShowPopup(
     return;
   platform_delegate_->ShowPopup(
       popup_host.Pass(), grant_tab_permissions, show_action);
-  view_delegate_->OnPopupShown(grant_tab_permissions);
+  PressButton(grant_tab_permissions);
 }
 
 void ExtensionActionViewController::OnPopupClosed() {
-  popup_host_observer_.Remove(popup_host_);
-  popup_host_ = nullptr;
+  if (popup_host_) {
+    popup_host_observer_.Remove(popup_host_);
+    popup_host_ = nullptr;
+  }
+  toolbar_actions_bar_->SetPopupOwner(nullptr);
+  RaiseButton();
+}
+
+void ExtensionActionViewController::HideActiveSidebar() {
+  if (toolbar_actions_bar_) {
+    toolbar_actions_bar_->HideActiveSidebar();
+  } else {
+    DCHECK_EQ(ActionInfo::TYPE_PAGE, extension_action_->action_type());
+    // In the traditional toolbar, page actions only know how to close their own

[Devlin, 2015/07/30 17:56:46]

Obviously from copy paste (another reason not to do it), but this won't work
here.  A page action will need to close another sidebar, too.

[ltilve, 2015/08/07 16:15:02]

Done. Because the sidebar extension is only instantiated using browser action,
we modified it to handle page action as a error.
Also, toolbar_actions_bar is null only if this is a page action without
the toolbar redesigned turned on, so we handle it as a error in the sidebar
case.

+    // sidebar.
+    HideSidebar();
+  }
+}
+
+void ExtensionActionViewController::HideSidebar() {
+  if (!sidebar_container_)
+    return;
+
+  sidebar_container_.reset();
+  if (toolbar_actions_bar_)
+    toolbar_actions_bar_->SetSidebarOwner(nullptr);
+
+  RaiseButton();
+}
+
+void ExtensionActionViewController::RaiseButton() {
+  // Reset button state
   if (toolbar_actions_bar_) {
-    toolbar_actions_bar_->SetPopupOwner(nullptr);
     if (toolbar_actions_bar_->popped_out_action() == this &&
-        !view_delegate_->IsMenuRunning())
+        !view_delegate_->IsMenuRunning()) {
       toolbar_actions_bar_->UndoPopOut();
+    }
   }
   view_delegate_->OnPopupClosed();
 }
+
+void ExtensionActionViewController::PressButtonWithSlideOutIfEnabled(
+    const base::Closure& closure) {
+  if (!toolbar_actions_bar_ || toolbar_actions_bar_->IsActionVisible(this) ||
+      !extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
+    closure.Run();
+    return;
+  }
+
+  // We disabled PopOutAction for sidebar
+  if (extension_action_->open_in_sidebar())
+    return;
+
+  platform_delegate_->CloseOverflowMenu();
+  toolbar_actions_bar_->PopOutAction(this, closure);
+}
+
+void ExtensionActionViewController::PressButton(bool grant_tab_permissions) {
+  view_delegate_->OnPopupShown(grant_tab_permissions);
+}