[Extensions] Fix chrome url override settings

chrome/browser/extensions/extension_override_apitest.cc

Index: chrome/browser/extensions/extension_override_apitest.cc
diff --git a/chrome/browser/extensions/extension_override_apitest.cc b/chrome/browser/extensions/extension_override_apitest.cc
index a865c970113001004fb4239750cbbc179236b424..9249b88a7f9a8f19edad397b7fe9f52a648e12c1 100644
--- a/chrome/browser/extensions/extension_override_apitest.cc
+++ b/chrome/browser/extensions/extension_override_apitest.cc
@@ -16,10 +16,13 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/common/constants.h"
+#include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
 
 using content::WebContents;
 
+namespace extensions {
+
 class ExtensionOverrideTest : public ExtensionApiTest {
  protected:
   bool CheckHistoryOverridesContainsNoDupes() {
@@ -28,43 +31,142 @@ class ExtensionOverrideTest : public ExtensionApiTest {
         browser()->profile()->GetPrefs()->GetDictionary(
             ExtensionWebUI::kExtensionURLOverrides);
 
-    const base::ListValue* values = NULL;
+    const base::ListValue* values = nullptr;
     if (!overrides->GetList("history", &values))
       return false;
 
     std::set<std::string> seen_overrides;
-    for (size_t i = 0; i < values->GetSize(); ++i) {
-      std::string value;
-      if (!values->GetString(i, &value))
-        return false;
-
-      if (seen_overrides.find(value) != seen_overrides.end())
+    for (const base::Value* val : *values) {
+      const base::DictionaryValue* dict = nullptr;
+      std::string entry;
+      if (!val->GetAsDictionary(&dict) || !dict->GetString("entry", &entry) ||
+          seen_overrides.count(entry) != 0)
         return false;
-
-      seen_overrides.insert(value);
+      seen_overrides.insert(entry);
     }
 
     return true;
   }
+
+  // Returns AssertionSuccess() if the given |web_contents| is being actively
+  // controlled by the extension with |extension_id|.
+  testing::AssertionResult ExtensionControlsPage(
+      content::WebContents* web_contents,
+      const std::string& extension_id) {
+    if (!web_contents->GetController().GetLastCommittedEntry())
+      return testing::AssertionFailure() << "No last committed entry.";
+    // We can't just use WebContents::GetLastCommittedURL() here because
+    // trickiness makes it think that it committed chrome://newtab.
+    GURL gurl = web_contents->GetController().GetLastCommittedEntry()->GetURL();
+    if (!gurl.SchemeIs(kExtensionScheme))
+      return testing::AssertionFailure() << gurl;

[Finnur, 2015/12/23 13:08:03]

Does this do what you expect, or do you need gurl.spec().c_str() or something?

[Devlin, 2015/12/30 03:10:40]

This works; GURL has an operator <<.

+    if (gurl.host() != extension_id)
+      return testing::AssertionFailure() << gurl;
+    return testing::AssertionSuccess();
+  }
+
+  base::FilePath data_dir() {
+    return test_data_dir_.AppendASCII("override");
+  }
 };
 
+// Basic test for overriding the NTP.
 IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, OverrideNewTab) {
-  ASSERT_TRUE(RunExtensionTest("override/newtab")) << message_;
+  const Extension* extension = LoadExtension(data_dir().AppendASCII("newtab"));
   {
-    extensions::ResultCatcher catcher;
     // Navigate to the new tab page.  The overridden new tab page
-    // will call chrome.test.notifyPass() .
+    // will call chrome.test.sendMessage('controlled by first').
+    ExtensionTestMessageListener listener(false);
     ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/"));
-    WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
-    ASSERT_TRUE(tab->GetController().GetVisibleEntry());
-    EXPECT_TRUE(tab->GetController().GetVisibleEntry()->GetURL().
-                SchemeIs(extensions::kExtensionScheme));
+    EXPECT_TRUE(ExtensionControlsPage(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        extension->id()));
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_EQ("controlled by first", listener.message());
+  }
+}
 
-    ASSERT_TRUE(catcher.GetNextResult());
+// Check having multiple extensions with the same override.
+IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, OverrideNewTabMultiple) {
+  // Prefer IDs because loading/unloading invalidates the extension ptrs.
+  const std::string extension1_id =
+      LoadExtension(data_dir().AppendASCII("newtab"))->id();
+  const std::string extension2_id =
+      LoadExtension(data_dir().AppendASCII("newtab2"))->id();
+  {
+    // Navigate to the new tab page. Last extension installed wins, so
+    // the new tab page should be controlled by the second extension.
+    ExtensionTestMessageListener listener(false);
+    ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/"));
+    EXPECT_TRUE(ExtensionControlsPage(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        extension2_id));
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_EQ("controlled by second", listener.message());
+  }
+
+  // Unload and reload the first extension. This should *not* result in the
+  // first extension moving to the front of the line.
+  ReloadExtension(extension1_id);
+
+  {
+    // The page should still be controlled by the second extension.
+    ExtensionTestMessageListener listener(false);
+    ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/"));
+    EXPECT_TRUE(ExtensionControlsPage(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        extension2_id));
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_EQ("controlled by second", listener.message());
   }
 
-  // TODO(erikkay) Load a second extension with the same override.
-  // Verify behavior, then unload the first and verify behavior, etc.
+  // Unload the (controlling) second extension. Now, and only now, should
+  // extension1 take over.
+  UnloadExtension(extension2_id);
+
+  {
+    ExtensionTestMessageListener listener(false);
+    ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/"));
+    EXPECT_TRUE(ExtensionControlsPage(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        extension1_id));
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_EQ("controlled by first", listener.message());
+  }
+}
+
+// Test that unloading an extension overriding the page reloads the page with
+// the proper url.
+IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest,
+                       OverridingExtensionUnloadedWithPageOpen) {
+  // Prefer IDs because loading/unloading invalidates the extension ptrs.
+  const std::string extension1_id =
+      LoadExtension(data_dir().AppendASCII("newtab"))->id();
+  const std::string extension2_id =
+      LoadExtension(data_dir().AppendASCII("newtab2"))->id();
+  {
+    // Navigate to the new tab page. Last extension installed wins, so
+    // the new tab page should be controlled by the second extension.
+    ExtensionTestMessageListener listener(false);
+    ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/"));
+    EXPECT_TRUE(ExtensionControlsPage(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        extension2_id));
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_EQ("controlled by second", listener.message());
+  }
+
+  {
+    // Unload the controlling extension. The page should be automatically
+    // reloaded with the new controlling extension.
+    ExtensionTestMessageListener listener(false);
+    UnloadExtension(extension2_id);
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+    EXPECT_EQ("controlled by first", listener.message());
+    EXPECT_TRUE(ExtensionControlsPage(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        extension1_id));
+  }
 }
 
 #if defined(OS_MACOSX)
@@ -74,7 +176,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, OverrideNewTab) {
 #define MAYBE_OverrideNewTabIncognito OverrideNewTabIncognito
 #endif
 IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, MAYBE_OverrideNewTabIncognito) {
-  ASSERT_TRUE(RunExtensionTest("override/newtab")) << message_;
+  LoadExtension(data_dir().AppendASCII("newtab"));
 
   // Navigate an incognito tab to the new tab page.  We should get the actual
   // new tab page because we can't load chrome-extension URLs in incognito.
@@ -83,7 +185,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, MAYBE_OverrideNewTabIncognito) {
   WebContents* tab = otr_browser->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(tab->GetController().GetVisibleEntry());
   EXPECT_FALSE(tab->GetController().GetVisibleEntry()->GetURL().
-               SchemeIs(extensions::kExtensionScheme));
+               SchemeIs(kExtensionScheme));
 }
 
 // Times out consistently on Win, http://crbug.com/45173.
@@ -96,7 +198,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, MAYBE_OverrideNewTabIncognito) {
 IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, MAYBE_OverrideHistory) {
   ASSERT_TRUE(RunExtensionTest("override/history")) << message_;
   {
-    extensions::ResultCatcher catcher;
+    ResultCatcher catcher;
     // Navigate to the history page.  The overridden history page
     // will call chrome.test.notifyPass() .
     ui_test_utils::NavigateToURL(browser(), GURL("chrome://history/"));
@@ -106,29 +208,35 @@ IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, MAYBE_OverrideHistory) {
 
 // Regression test for http://crbug.com/41442.
 IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, ShouldNotCreateDuplicateEntries) {
-  const extensions::Extension* extension =
+  const Extension* extension =
       LoadExtension(test_data_dir_.AppendASCII("override/history"));
   ASSERT_TRUE(extension);
 
   // Simulate several LoadExtension() calls happening over the lifetime of
   // a preferences file without corresponding UnloadExtension() calls.
   for (size_t i = 0; i < 3; ++i) {
-    ExtensionWebUI::RegisterChromeURLOverrides(
+    ExtensionWebUI::RegisterOrActivateChromeURLOverrides(
         browser()->profile(),
-        extensions::URLOverrides::GetChromeURLOverrides(extension));
+        URLOverrides::GetChromeURLOverrides(extension));
   }
 
   ASSERT_TRUE(CheckHistoryOverridesContainsNoDupes());
 }
 
+// TODO(devlin): This test seems a bit contrived. How would we end up with
+// duplicate entries created?
 IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, ShouldCleanUpDuplicateEntries) {
   // Simulate several LoadExtension() calls happening over the lifetime of
   // a preferences file without corresponding UnloadExtension() calls. This is
   // the same as the above test, except for that it is testing the case where
   // the file already contains dupes when an extension is loaded.
   base::ListValue* list = new base::ListValue();
-  for (size_t i = 0; i < 3; ++i)
-    list->Append(new base::StringValue("http://www.google.com/"));
+  for (size_t i = 0; i < 3; ++i) {
+    scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
+    dict->SetString("entry", "http://www.google.com/");
+    dict->SetBoolean("active", true);
+    list->Append(std::move(dict));
+  }
 
   {
     DictionaryPrefUpdate update(browser()->profile()->GetPrefs(),
@@ -138,7 +246,9 @@ IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, ShouldCleanUpDuplicateEntries) {
 
   ASSERT_FALSE(CheckHistoryOverridesContainsNoDupes());
 
-  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("override/history")));
+  ExtensionWebUI::InitializeChromeURLOverrides(profile());
 
   ASSERT_TRUE(CheckHistoryOverridesContainsNoDupes());
 }
+
+}  // namespace extensions