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..dc49a851d2ca5f7bd360dab03a3360c31d34e128 100644
--- a/chrome/browser/ui/extensions/extension_action_view_controller.cc
+++ b/chrome/browser/ui/extensions/extension_action_view_controller.cc
@@ -12,15 +12,18 @@
 #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/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"
@@ -54,10 +57,32 @@ ExtensionActionViewController::ExtensionActionViewController(
   DCHECK(extension_action->action_type() == ActionInfo::TYPE_PAGE ||
          extension_action->action_type() == ActionInfo::TYPE_BROWSER);
   DCHECK(extension);
+
+  extensions::SidebarManager* sidebar_manager =
+      extensions::SidebarManager::GetFromContext(browser_->profile());
+  sidebar_manager->AddObserver(this);
 }
 
 ExtensionActionViewController::~ExtensionActionViewController() {
   DCHECK(!is_showing_popup());
+
+  extensions::SidebarManager* sidebar_manager =
+      extensions::SidebarManager::GetFromContext(browser_->profile());
+
+  sidebar_manager->RemoveObserver(this);
+  if (GetPreferredPopupViewController() == this) {
+    int count = browser_->tab_strip_model()->count();
+    for (int i = 0; i < count; ++i) {
+      content::WebContents* contents =
+          browser_->tab_strip_model()->GetWebContentsAt(i);
+      if (contents) {
+        extensions::SidebarContainer* sidebar =
+            sidebar_manager->GetSidebarContainerFor(contents);
+        if (sidebar && sidebar->extension_id() == GetId())
+          sidebar_manager->HideSidebar(contents);
+      }
+    }
+  }
 }
 
 const std::string& ExtensionActionViewController::GetId() const {
@@ -304,6 +329,7 @@ bool ExtensionActionViewController::TriggerPopupWithUrl(
     return false;
 
   bool already_showing = is_showing_popup();

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

I think we should just re-brand most of these popup things to apply to either a
popup or a sidebar.  Something like is_showing_content().  That will simplify
quite a few of the checks later on.  You shouldn't have to add much new logic
for the sidebar.

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

We are having some doubts about mixing both functions in just one
(is_showing_content()), instead of maybe having another is_showing_sidebar(), as
we do that check in other parts.

For instance, in this TriggerPpopupWithUrl() if we are already showing a popup
we return; but if we are showing a sidebar, we close it. If we merge both
functions, we wouldn't be able to distinguish between both cases, and might need
some more checks after that. It happens quite the same with some other specific
popup things.

Maybe I'm missing some point here, so would be great if you let us know if this
is_showing_sidebar() approach makes sense.

[Devlin, 2015/07/10 20:18:42]

We actually do close the popup, too - it's just handled as part of
HideActivePopup().  And we can still have some sidebar specific logic,
obviously, just most of it shouldn't be.  So in this case, it would be something
like:

bool already_showing = is_showing_content();
bool use_sidebar = exetnsion_action_->open_in_sidebar();
HideActivePopup();

if (already_showing) {
  if (use_sidebar)
    sidebar_manager->HideSidebar(web_contents);
  return;
}

Are there places, other than the showing/hiding of the popup/sidebar, that we
care about whether it's a popup or a sidebar?

[ltilve, 2015/07/17 16:26:22]

We have done some refactoring to increase readability and to avoid redundant
code. We have introduced TriggerSidebarWithUrl with specific sidebar logic and
we don't need duplicated checks for is_sidebar, or if we are using the
redesigned toolbar.

+  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.
@@ -315,6 +341,29 @@ bool ExtensionActionViewController::TriggerPopupWithUrl(
   if (already_showing)
     return false;
 
+  extensions::SidebarManager* sidebar_manager =
+      extensions::SidebarManager::GetFromContext(browser_->profile());
+  content::WebContents* web_contents = view_delegate_->GetCurrentWebContents();
+
+  if (use_sidebar) {
+    extensions::SidebarContainer* sidebar =
+        sidebar_manager->GetSidebarContainerFor(web_contents);
+    if (sidebar && sidebar->extension_id() == GetId()) {
+      sidebar_manager->HideSidebar(web_contents);
+      return false;
+    }
+
+    sidebar_manager->AddObserver(this);
+
+    sidebar_manager->CreateSidebar(web_contents, popup_url, browser_);
+    return true;
+  }
+
+  // Hide sidebar if visible
+  if (sidebar_manager->GetSidebarContainerFor(web_contents) != nullptr) {
+    sidebar_manager->HideSidebar(web_contents);
+  }
+
   scoped_ptr<extensions::ExtensionViewHost> host(
       extensions::ExtensionViewHostFactory::CreatePopupHost(popup_url,
                                                             browser_));
@@ -368,3 +417,57 @@ 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) {
+  UpdateButtonState();
+}
+
+void ExtensionActionViewController::UpdateButtonState() {
+  if (GetPreferredPopupViewController() != this)
+    return;
+
+  // Should the button be depressed?
+  content::WebContents* current = view_delegate_->GetCurrentWebContents();
+  if (current) {
+    extensions::SidebarManager* sidebar_manager =
+        extensions::SidebarManager::GetFromContext(browser_->profile());
+    extensions::SidebarContainer* sidebar =
+        sidebar_manager->GetSidebarContainerFor(current);
+    if (sidebar) {
+      if (sidebar->extension_id() == GetId()) {
+        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,

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

I don't think we should have actions with a sidebar forced out.  It makes it
impossible to reason about reordering.

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

Could you please elaborate a bit more what you mean about this comment?

This current implementation slides out and in the browserAction from the
overview for hidden-in-menu extensions while the sidebar is visible (in the same
way that popups).

There is a case on which we are on a temporary state on which the attribution
was not explicit, and we have some doubts the expected behaviour to implement.
Clicking first on a hidden extension-1 that opens a sidebar (its pressed
browserAction slides out from the overflow), and after that, clicking on another
hidden extension-2 that opens a popup, slides out its button and hides
extension-1 one. If the popup is closed, we are seeing the sidebar visible
without any attribution.

We have seen that having 2 icons outside for the case of a simultaneous open
popup and sidebar makes all the popActions too complicated, so we understand
that in this case the expectable behaviour is sliding out again the sidebar
extension browserAction when the popup is closed. Is this correct?

[Devlin, 2015/07/10 20:18:42]

Nah.  Having any kind of slid out action for a persistent and non modal thing
(like a sidebar) just doesn't really make sense, because it leads to too many
confusing instances that are really hard to explain to the user.  In the case
you described, you sometimes have the extension shown, and sometimes not - based
on the presence of a popup.  From a UI standpoint, why should that affect it? 
And more complicatedly, if you have the action slid out, what happens if the
user tries to reorder it?  Or reorder others around it?  It's just very
difficult to come up with logical flows that would make intuitive sense to a
user.  Instead, we should just not worry about sliding out the extension that
owns the sidebar.  Since it's gated on user action, the attribution is less
important.

[ltilve, 2015/07/17 16:26:22]

Done. We are not sliding out the browser action now for sidebar extensions.

+                               weak_factory_.GetWeakPtr(), true));
+        } else {
+          // Without the popup corner arrow indicator, marking the browserAction
+          // icon
+          // is necessary for extension attribution
+          view_delegate_->OnPopupShown(true);
+        }
+        return;
+      }
+    }
+  }
+
+  // Reset button state
+  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();
+}