[Extensions] Allow overriding a nested options page for opening a new instance

If there is an existing chrome://extensions page open, it should be able to be
overridden to show an extension's options page if that options page opens in
a view in the chrome://extensions page.

BUG=595253

Review URL: https://codereview.chromium.org/1805013005

Cr-Commit-Position: refs/heads/master@{#381555}

chrome/browser/extensions/extension_tab_util.cc

@@ -635,7 +635,8 @@ bool ExtensionTabUtil::OpenOptionsPage(const Extension* extension,
   }
 
   GURL url_to_navigate;
-  if (OptionsPageInfo::ShouldOpenInTab(extension)) {
+  bool open_in_tab = OptionsPageInfo::ShouldOpenInTab(extension);
+  if (open_in_tab) {
     // Options page tab is simply e.g. chrome-extension://.../options.html.
     url_to_navigate = OptionsPageInfo::GetOptionsPage(extension);
   } else {
@@ -653,8 +654,12 @@ bool ExtensionTabUtil::OpenOptionsPage(const Extension* extension,
       chrome::GetSingletonTabNavigateParams(browser, url_to_navigate));
   // We need to respect path differences because we don't want opening the
   // options page to close a page that might be open to extension content.
+  // However, if the options page opens inside the chrome://extensions page, we
+  // can override an existing page.
   // Note: default ref behavior is IGNORE_REF, which is correct.
-  params.path_behavior = chrome::NavigateParams::RESPECT;
+  params.path_behavior = open_in_tab
+                             ? chrome::NavigateParams::RESPECT
+                             : chrome::NavigateParams::IGNORE_AND_NAVIGATE;
   params.url = url_to_navigate;
   chrome::ShowSingletonTabOverwritingNTP(browser, params);
   return true;

chrome/browser/extensions/extension_tab_util_browsertest.cc

@@ -12,66 +12,73 @@
 
 namespace extensions {
 
+namespace {
+
+const GURL& GetActiveUrl(Browser* browser) {
+  return browser->tab_strip_model()
+      ->GetActiveWebContents()
+      ->GetLastCommittedURL();
+}
+
+}  // namespace
+
 using ExtensionTabUtilBrowserTest = ExtensionBrowserTest;
 
 IN_PROC_BROWSER_TEST_F(ExtensionTabUtilBrowserTest, OpenExtensionsOptionsPage) {
-  // Load an extension with an options page.
-  const Extension* extension =
+  // Load an extension with an options page that opens in a tab and one that
+  // opens in the chrome://extensions page in a view.
+  const Extension* options_in_tab =
       LoadExtension(test_data_dir_.AppendASCII("options_page"));
-  ASSERT_TRUE(extension);
-  ASSERT_TRUE(OptionsPageInfo::HasOptionsPage(extension));
+  const Extension* options_in_view =
+      LoadExtension(test_data_dir_.AppendASCII("options_page_in_view"));
+  ASSERT_TRUE(options_in_tab);
+  ASSERT_TRUE(options_in_view);
+  ASSERT_TRUE(OptionsPageInfo::HasOptionsPage(options_in_tab));
+  ASSERT_TRUE(OptionsPageInfo::HasOptionsPage(options_in_view));
 
   // Start at the new tab page, and then open the extension options page.
   ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab"));
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
-  GURL options_url = OptionsPageInfo::GetOptionsPage(extension);
-  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(extension, browser()));
+  GURL options_url = OptionsPageInfo::GetOptionsPage(options_in_tab);
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_tab, browser()));
 
   // Opening the options page should take the new tab and use it, so we should
   // have only one tab, and it should be open to the options page.
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
   EXPECT_TRUE(content::WaitForLoadStop(
                   browser()->tab_strip_model()->GetActiveWebContents()));
-  EXPECT_EQ(options_url,
-            browser()->tab_strip_model()->
-                GetActiveWebContents()->GetLastCommittedURL());
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
 
   // Calling OpenOptionsPage again shouldn't result in any new tabs, since we
   // re-use the existing options page.
-  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(extension, browser()));
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_tab, browser()));
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
   EXPECT_TRUE(content::WaitForLoadStop(
                   browser()->tab_strip_model()->GetActiveWebContents()));
-  EXPECT_EQ(options_url,
-            browser()->tab_strip_model()->
-                GetActiveWebContents()->GetLastCommittedURL());
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
 
   // Navigate to google.com (something non-newtab, non-options). Calling
   // OpenOptionsPage() should create a new tab and navigate it to the options
   // page. So we should have two total tabs, with the active tab pointing to
   // options.
   ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/"));
-  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(extension, browser()));
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_tab, browser()));
   EXPECT_EQ(2, browser()->tab_strip_model()->count());
   EXPECT_TRUE(content::WaitForLoadStop(
                   browser()->tab_strip_model()->GetActiveWebContents()));
-  EXPECT_EQ(options_url,
-            browser()->tab_strip_model()->
-                GetActiveWebContents()->GetLastCommittedURL());
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
 
   // Navigate the tab to a different extension URL, and call OpenOptionsPage().
   // We should not reuse the current tab since it's opened to a page that isn't
   // the options page, and we don't want to arbitrarily close extension content.
   // Regression test for crbug.com/587581.
-  ui_test_utils::NavigateToURL(
-      browser(), extension->GetResourceURL("other.html"));
-  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(extension, browser()));
+  ui_test_utils::NavigateToURL(browser(),
+                               options_in_tab->GetResourceURL("other.html"));
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_tab, browser()));
   EXPECT_EQ(3, browser()->tab_strip_model()->count());
   EXPECT_TRUE(content::WaitForLoadStop(
                   browser()->tab_strip_model()->GetActiveWebContents()));
-  EXPECT_EQ(options_url,
-            browser()->tab_strip_model()->
-                GetActiveWebContents()->GetLastCommittedURL());
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
 
   // If the user navigates to the options page e.g. by typing in the url, it
   // should not override the currently-open tab.
@@ -81,9 +88,35 @@ IN_PROC_BROWSER_TEST_F(ExtensionTabUtilBrowserTest, OpenExtensionsOptionsPage) {
       NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   EXPECT_EQ(4, browser()->tab_strip_model()->count());
-  EXPECT_EQ(options_url,
-            browser()->tab_strip_model()->
-                GetActiveWebContents()->GetLastCommittedURL());
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
+
+  // Test the extension that has the options page open in a view inside
+  // chrome://extensions.
+  // Triggering OpenOptionsPage() should create a new tab, since there are none
+  // to override.
+  options_url = GURL("chrome://extensions/?options=" + options_in_view->id());
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_view, browser()));
+  EXPECT_EQ(5, browser()->tab_strip_model()->count());
+  EXPECT_TRUE(content::WaitForLoadStop(
+      browser()->tab_strip_model()->GetActiveWebContents()));
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
+
+  // Calling it a second time should not create a new tab, since one already
+  // exists with that options page open.
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_view, browser()));
+  EXPECT_EQ(5, browser()->tab_strip_model()->count());
+  EXPECT_TRUE(content::WaitForLoadStop(
+      browser()->tab_strip_model()->GetActiveWebContents()));
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
+
+  // Navigate to chrome://extensions (no options). Calling OpenOptionsPage()
+  // should override that tab rather than opening a new tab. crbug.com/595253.
+  ui_test_utils::NavigateToURL(browser(), GURL("chrome://extensions"));
+  EXPECT_TRUE(ExtensionTabUtil::OpenOptionsPage(options_in_view, browser()));
+  EXPECT_EQ(5, browser()->tab_strip_model()->count());
+  EXPECT_TRUE(content::WaitForLoadStop(
+      browser()->tab_strip_model()->GetActiveWebContents()));
+  EXPECT_EQ(options_url, GetActiveUrl(browser()));
 }
 
 }  // namespace extensions

chrome/test/data/extensions/options_page_in_view/manifest.json

+{
+  "name": "Extension With Options Page",
+  "description": "An extension with an options page and an extra page",
+  "options_ui": { "page": "options.html", "open_in_tab": false },
+  "version": "0.1.1",
+  "manifest_version": 2
+}

chrome/test/data/extensions/options_page_in_view/options.html

+An options page

chrome/test/data/extensions/options_page_in_view/other.html

+Another page