Implement sidebar support for extension action popups

chrome/browser/extensions/sidebar_browsertest.cc

+// Copyright (c) 2015 The Chromium Authors. All rights reserved.

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

no (c) (Goes for all new files)

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

Done for all the new files.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/path_service.h"
+#include "chrome/browser/extensions/browser_action_test_util.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/sidebar_manager.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/common/extension.h"
+
+using content::NavigationController;
+using content::WebContents;
+using extensions::SidebarManager;
+
+namespace extensions {
+
+const char kSimplePage[] = "/simple_page.html";
+
+class SidebarTest : public ExtensionBrowserTest {

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

This looks like it can be split into two tests:
- a SidebarManager test, that tests opening and closing sidebars through the
SidebarManager API.  This can be a unittest.  This is where the majority of the
edge-case logic should be tested.
- a SidebarBrowserTest, that tests opening and closing sidebars with extension
actions (more of an end-to-end test).

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

Done. We are now having SidebarManager unnittests at this patch set, testing the
API, and browser tests that were moved to the next patch set, where the browser
actions are producing the expected output. 

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

Is there a reason the browser tests can't be in this patch set?

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

I have moved them to the next patch set because as they use Press() on the
BrowserAction to show/hide the sidebars, without the UI part, this
HasSidebarForCurrentTab() is false and the tests are not passing. Should I
include them in any case, expecting false for HasSidebarForCurrentTab() after
the click?

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

I don't see why it should.  HasSidebarForCurrentTab() is based solely on the
SidebarManager, which can set a sidebar independent of whether or not the
sidebar is visibly shown in the UI.  Handling the sidebar creation is done in
the ExtensionActionViewController, which is part of this patch.  So you should
be able to test it without it failing.

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

Done. We have created two separated tests for the case of the standard behaviour
of clicking to show/hide the sidebar, and to check open_in_sidebar. Please let
us know if you think it's better to merge them as a single one.

+ public:
+  SidebarTest() {}

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

Please add a destructor

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

Done.

+
+ protected:
+  // InProcessBrowserTest overrides.

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

s/ overrides./:
s/InProcessBrowserTest/ExtensionBrowserTest

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

Done.

+  void SetUpOnMainThread() override {
+    ExtensionBrowserTest::SetUpOnMainThread();
+
+    // Load test sidebar extension.
+    base::FilePath extension_path;
+    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extension_path));
+    extension_path = extension_path.AppendASCII("sidebar");
+    extension_ = LoadExtension(extension_path);
+
+    ASSERT_TRUE(extension_);
+
+    content_id_ = last_loaded_extension_id();
+  }
+
+  ExtensionAction* GetBrowserAction(const Extension& extension) {
+    return ExtensionActionManager::Get(browser()->profile())
+        ->GetBrowserAction(extension);
+  }
+
+  BrowserActionTestUtil* GetBrowserActionsBar() {
+    if (!browser_action_test_util_)
+      browser_action_test_util_.reset(new BrowserActionTestUtil(browser()));

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

again, no real point in lazily initializing this. Do it in setup.

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

Done.

+    return browser_action_test_util_.get();
+  }
+
+  void ClickExtensionBrowserAction() {
+    GetBrowserActionsBar()->Press(0);
+  }
+
+  void CreateSidebar(WebContents* temp) {
+    WebContents* tab = static_cast<WebContents*>(temp);

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

what does this line do (and the similar ones below)?

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

Done. Removed all of them.

+    SidebarManager* sidebar_manager =
+        SidebarManager::GetFromContext(browser()->profile());
+    GURL url("chrome-extension://" + content_id_ + kSimplePage);
+    sidebar_manager->CreateSidebar(tab, url, browser());
+  }
+
+  void HideSidebar(WebContents* temp) {
+    WebContents* tab = static_cast<WebContents*>(temp);
+    SidebarManager* sidebar_manager =
+        SidebarManager::GetFromContext(browser()->profile());
+    sidebar_manager->HideSidebar(tab);
+    EXPECT_FALSE(sidebar_manager->HasSidebar(tab));
+  }
+
+  WebContents* web_contents(int i) {
+    return static_cast<WebContents*>(
+        browser()->tab_strip_model()->GetWebContentsAt(i));
+  }
+
+  void DisableOpenInSidebar() {
+    GetBrowserAction(*extension_)->set_open_in_sidebar(false);
+  }
+
+  bool HasSidebarForCurrentTab() {
+    SidebarManager* sidebar_manager =
+        SidebarManager::GetFromContext(browser()->profile());
+    return sidebar_manager->HasSidebar(
+        browser()->tab_strip_model()->GetActiveWebContents());
+  }
+
+ private:
+  std::string content_id_;
+  const Extension* extension_;
+  scoped_ptr<BrowserActionTestUtil> browser_action_test_util_;

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

DISALLOW_COPY_AND_ASSIGN

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

Done.

+};
+
+// Tests that cliking on the browser action show/hides the sidebar
+IN_PROC_BROWSER_TEST_F(SidebarTest, CreateSidebar) {
+  ClickExtensionBrowserAction();
+  EXPECT_TRUE(HasSidebarForCurrentTab());
+  ClickExtensionBrowserAction();
+  EXPECT_FALSE(HasSidebarForCurrentTab());
+}
+
+// Tests that sidebars are not shown if open_in_sidebar: false
+IN_PROC_BROWSER_TEST_F(SidebarTest, CreateDisabledSidebar) {
+  DisableOpenInSidebar();
+  ClickExtensionBrowserAction();
+  EXPECT_FALSE(HasSidebarForCurrentTab());
+}
+
+// Tests that sidebar is only visible at the proper tab
+IN_PROC_BROWSER_TEST_F(SidebarTest, SwitchingTabs) {
+  ClickExtensionBrowserAction();
+  chrome::NewTab(browser());
+
+  // Make sure sidebar is not visbile for the newly opened tab.
+  EXPECT_FALSE(HasSidebarForCurrentTab());
+
+  // Switch back to the first tab.
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  tab_strip_model->ActivateTabAt(0, false);
+
+  // Make sure it is visible now.
+  EXPECT_TRUE(HasSidebarForCurrentTab());
+
+  ClickExtensionBrowserAction();
+
+  // Make sure it is not visible any more
+  EXPECT_FALSE(HasSidebarForCurrentTab());
+}
+
+// Tests hiding sidebars on inactive tabs
+IN_PROC_BROWSER_TEST_F(SidebarTest, SidebarOnInactiveTab) {
+  ClickExtensionBrowserAction();
+  chrome::NewTab(browser());
+
+  // Hide sidebar on inactive (first) tab.
+  HideSidebar(web_contents(0));
+
+  // Switch back to the first tab.
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  tab_strip_model->ActivateTabAt(0, false);
+
+  // Make sure sidebar is not visbile anymore.
+  EXPECT_FALSE(HasSidebarForCurrentTab());
+
+  // Show sidebar on inactive (second) tab.
+  CreateSidebar(web_contents(1));
+
+  // Make sure sidebar is not visible yet.
+  EXPECT_FALSE(HasSidebarForCurrentTab());
+}
+
+}  // namespace