Fix for Bug 50726 "Save extension list and "winning" prefs from extensions"

chrome/browser/extensions/extension_prefs_unittest.cc

Index: chrome/browser/extensions/extension_prefs_unittest.cc
diff --git a/chrome/browser/extensions/extension_prefs_unittest.cc b/chrome/browser/extensions/extension_prefs_unittest.cc
index c8d1967a70a3efd26e5eb88d3d55f62c1198e218..325ca9139bbd5e4b49279fd8eb7f3008cdb7f989 100644
--- a/chrome/browser/extensions/extension_prefs_unittest.cc
+++ b/chrome/browser/extensions/extension_prefs_unittest.cc
@@ -10,9 +10,14 @@
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/test_extension_prefs.h"
+#include "chrome/browser/prefs/pref_change_registrar.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/notification_details.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::Time;
@@ -32,7 +37,11 @@ class ExtensionPrefsTest : public testing::Test {
   // things don't break after any ExtensionPrefs startup work.
   virtual void Verify() = 0;
 
+  // This function is called to Register preference default values.
+  virtual void RegisterPreferences() {}
+
   virtual void SetUp() {
+    RegisterPreferences();
     Initialize();
   }
 
@@ -41,6 +50,7 @@ class ExtensionPrefsTest : public testing::Test {
 
     // Reset ExtensionPrefs, and re-verify.
     prefs_.RecreateExtensionPrefs();
+    RegisterPreferences();
     Verify();
   }
 
@@ -53,6 +63,16 @@ class ExtensionPrefsTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(ExtensionPrefsTest);
 };
 
+// Mock MockObserver that allows the notifications to be tracked.
+class MockObserver : public NotificationObserver {

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Drop this in favor of chrome/test/mock_notification_observer.h

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+ public:
+  MockObserver() {}
+  virtual ~MockObserver() {}
+  MOCK_METHOD3(Observe, void(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details));
+};
+
 // Tests the LastPingDay/SetLastPingDay functions.
 class ExtensionPrefsLastPingDay : public ExtensionPrefsTest {
  public:
@@ -185,6 +205,10 @@ class ExtensionPrefsBlacklist : public ExtensionPrefsTest {
     std::set<std::string> blacklisted_ids;
     blacklisted_ids.insert(extensions_[0]->id());
     blacklisted_ids.insert(not_installed_id_);
+
+    // TODO(battre) This prints some nasty "Error parsing installation time of
+    // extension" messages because the extension preferences have incomplete
+    // dictionaries (they contain only the blacklist-property).

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Triggering error messages in unit_tests is a common thing to happen, so if they
are really legitimate at this point, you don't need to take special action to
shut them up.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

     prefs()->UpdateBlacklist(blacklisted_ids);
   }
 
@@ -336,7 +360,7 @@ TEST_F(ExtensionPrefsOnExtensionInstalled,
        ExtensionPrefsOnExtensionInstalled) {}
 
 class ExtensionPrefsAppLaunchIndex : public ExtensionPrefsTest {
-public:
+ public:
   virtual void Initialize() {
     // No extensions yet.
     EXPECT_EQ(0, prefs()->GetNextAppLaunchIndex());
@@ -363,7 +387,346 @@ public:
     EXPECT_EQ(-1, prefs()->GetAppLaunchIndex("foo"));
   }
 
-private:
+ private:
   scoped_refptr<Extension> extension_;
 };
 TEST_F(ExtensionPrefsAppLaunchIndex, ExtensionPrefsAppLaunchIndex) {}
+
+namespace keys = extension_manifest_keys;
+
+// Use constants to avoid confusing std::map with hard-coded strings.

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

I don't understand? Constants are good, but not for the reason you're giving.

[battre (please use the other), 2010/11/19 16:03:18]

just copy and pasted that comment. I have removed it now.

+const char kPref1[] = "path1.subpath";
+const char kPref2[] = "path2";
+const char kPref3[] = "path3";
+const char kPref4[] = "path4";
+
+// Default values in case an extension pref value is not overridden.
+const char kDefaultPref1[] = "default pref 1";
+const char kDefaultPref2[] = "default pref 2";
+const char kDefaultPref3[] = "default pref 3";
+const char kDefaultPref4[] = "default pref 4";

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

These declarations should be moved to the top of the file IMHO. But please
consider the style guide for an authoritive answer.

[battre (please use the other), 2010/11/19 16:03:18]

Moved it, but did not find anything in the style guide.
Done.

+
+class ExtensionPrefsPreferencesBase : public ExtensionPrefsTest {
+ public:
+  ExtensionPrefsPreferencesBase()
+      : ExtensionPrefsTest(),
+        ext1(NULL),
+        ext2(NULL),
+        ext3(NULL),
+        installed() {
+    DictionaryValue simple_dict;
+    std::string error;
+
+    simple_dict.SetString(keys::kVersion, "1.0.0.0");
+    simple_dict.SetString(keys::kName, "unused");
+
+    ext1_scoped_ = Extension::Create(
+        prefs_.temp_dir().AppendASCII("ext1"), Extension::INVALID,
+        simple_dict, false, &error);
+    ext2_scoped_ = Extension::Create(
+        prefs_.temp_dir().AppendASCII("ext2"), Extension::INVALID,
+        simple_dict, false, &error);
+    ext3_scoped_ = Extension::Create(
+        prefs_.temp_dir().AppendASCII("ext3"), Extension::INVALID,
+        simple_dict, false, &error);
+
+    ext1 = ext1_scoped_.get();
+    ext2 = ext2_scoped_.get();
+    ext3 = ext3_scoped_.get();
+
+    for (size_t i = 0; i < arraysize(installed); ++i)
+      installed[i] = false;
+  }
+
+  void RegisterPreferences() {
+    prefs()->pref_service()->RegisterStringPref(kPref1, kDefaultPref1);
+    prefs()->pref_service()->RegisterStringPref(kPref2, kDefaultPref2);
+    prefs()->pref_service()->RegisterStringPref(kPref3, kDefaultPref3);
+    prefs()->pref_service()->RegisterStringPref(kPref4, kDefaultPref4);
+  }
+
+  void InstallExtControlledPref(Extension *ext,

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Instead of shortening to Ext, why not just InstallPref? The context should be
given be the test class name anyway.

[battre (please use the other), 2010/11/19 16:03:18]

Unfortunately, Extension Preferences have a different meaning in
|ExtensionPrefs|. I think your suggestion to call the new ones "Extension
Controlled Preferences" makes things clearer and should be used consistently.

[Mattias Nissler (ping if slow), 2010/11/19 16:52:20]

I see.

+                            const std::string& key,
+                            Value* val) {

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

fix indentation.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

[Mattias Nissler (ping if slow), 2010/11/19 16:52:20]

Not done?

[battre (please use the other), 2010/11/19 18:00:39]

Done.

+    // Install extension the first time a preference is set for it.
+    Extension* extensions[] = {ext1, ext2, ext3};
+    for (int i = 0; i < 3; ++i) {
+      if (ext == extensions[i] && !installed[i]) {
+        prefs()->OnExtensionInstalled(ext, Extension::ENABLED, true);
+        installed[i] = true;
+        break;
+      }
+    }
+
+    prefs()->SetExtensionControlledPref(ext->id(), key, val);
+  }
+
+  void UninstallExtension(const std::string& extension_id) {
+    prefs()->OnExtensionUninstalled(extension_id, Extension::INTERNAL, false);
+  }
+
+  // Weak references, for convenience.
+  Extension* ext1;
+  Extension* ext2;
+  Extension* ext3;

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Missing trailing _

[battre (please use the other), 2010/11/19 16:03:18]

Was just copy&pasted, but changed now.
Done.

[Mattias Nissler (ping if slow), 2010/11/19 16:52:20]

Not done?

[battre (please use the other), 2010/11/19 18:00:39]

Done.

+
+  // Flags indicating whether each of the extensions has been installed, yet.
+  bool installed[3];
+
+ private:
+  scoped_refptr<Extension> ext1_scoped_;
+  scoped_refptr<Extension> ext2_scoped_;
+  scoped_refptr<Extension> ext3_scoped_;
+};
+
+class ExtensionPrefsInstallOneExtension
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+  }
+  virtual void Verify() {
+    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val1", actual);
+  }
+};
+TEST_F(ExtensionPrefsInstallOneExtension, ExtensionPrefsInstallOneExtension) {}
+
+// Make sure the last-installed extension wins.
+class ExtensionPrefsInstallMultipleExtensions
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext2, kPref1, Value::CreateStringValue("val2"));
+    InstallExtControlledPref(ext3, kPref1, Value::CreateStringValue("val3"));
+  }
+  virtual void Verify() {
+    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val3", actual);
+  }
+};
+TEST_F(ExtensionPrefsInstallMultipleExtensions,
+    ExtensionPrefsInstallMultipleExtensions) {}
+
+// Make sure the last-installed extension wins for each preference.
+class ExtensionPrefsInstallOverwrittenExtensions

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

I could argue that this test case covers also what the previous one tests, so
maybe remove the previous one?

[battre (please use the other), 2010/11/19 16:03:18]

Done.

[Mattias Nissler (ping if slow), 2010/11/19 16:52:20]

No change?

[battre (please use the other), 2010/11/19 18:00:39]

Done.

+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext2, kPref1, Value::CreateStringValue("val2"));
+    InstallExtControlledPref(ext3, kPref1, Value::CreateStringValue("val3"));
+
+    InstallExtControlledPref(ext1, kPref2, Value::CreateStringValue("val4"));
+    InstallExtControlledPref(ext2, kPref2, Value::CreateStringValue("val5"));
+
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val6"));
+    InstallExtControlledPref(ext1, kPref2, Value::CreateStringValue("val7"));
+    InstallExtControlledPref(ext1, kPref3, Value::CreateStringValue("val8"));
+  }
+  virtual void Verify() {
+    std::string actual;
+    actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val3", actual);
+    actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ("val5", actual);
+    actual = prefs()->pref_service()->GetString(kPref3);
+    EXPECT_EQ("val8", actual);
+  }
+};
+TEST_F(ExtensionPrefsInstallOverwrittenExtensions,
+    ExtensionPrefsInstallOverwrittenExtensions) {}
+
+// Make sure the last-installed extension wins even if other extensions set
+// the same or different preferences later.
+class ExtensionPrefsInstallInterleavedExtensions
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext2, kPref2, Value::CreateStringValue("val2"));
+    InstallExtControlledPref(ext3, kPref3, Value::CreateStringValue("val3"));
+
+    InstallExtControlledPref(ext3, kPref3, Value::CreateStringValue("val4"));
+    InstallExtControlledPref(ext2, kPref3, Value::CreateStringValue("val5"));
+    InstallExtControlledPref(ext1, kPref3, Value::CreateStringValue("val6"));
+
+    InstallExtControlledPref(ext3, kPref1, Value::CreateStringValue("val7"));
+  }
+  virtual void Verify() {
+    std::string actual;
+    actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val7", actual);
+    actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ("val2", actual);
+    actual = prefs()->pref_service()->GetString(kPref3);
+    EXPECT_EQ("val4", actual);
+  }
+};
+TEST_F(ExtensionPrefsInstallInterleavedExtensions,
+    ExtensionPrefsInstallInterleavedExtensions) {}
+
+class ExtensionPrefsUninstallOnlyExtension
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext1, kPref2, Value::CreateStringValue("val2"));
+
+    UninstallExtension(ext1->id());
+  }
+  virtual void Verify() {
+    std::string actual;
+    actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ(kDefaultPref1, actual);
+    actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ(kDefaultPref2, actual);
+  }
+};
+TEST_F(ExtensionPrefsUninstallOnlyExtension,
+    ExtensionPrefsUninstallOnlyExtension) {}
+
+// Tests uninstalling an extension that wasn't winning for any preferences.
+class ExtensionPrefsUninstallIrrelevantExtension
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext2, kPref1, Value::CreateStringValue("val2"));
+
+    InstallExtControlledPref(ext1, kPref2, Value::CreateStringValue("val3"));
+    InstallExtControlledPref(ext2, kPref2, Value::CreateStringValue("val4"));
+
+    UninstallExtension(ext1->id());
+  }
+  virtual void Verify() {
+    std::string actual;
+    actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val2", actual);
+    actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ("val4", actual);
+  }
+};
+TEST_F(ExtensionPrefsUninstallIrrelevantExtension,
+    ExtensionPrefsUninstallIrrelevantExtension) {}
+
+// Tests uninstalling an extension that was winning for all preferences.
+class ExtensionPrefsUninstallExtensionFromTop
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext2, kPref1, Value::CreateStringValue("val2"));
+    InstallExtControlledPref(ext3, kPref1, Value::CreateStringValue("val3"));
+
+    InstallExtControlledPref(ext1, kPref2, Value::CreateStringValue("val4"));
+    InstallExtControlledPref(ext3, kPref2, Value::CreateStringValue("val5"));
+
+    UninstallExtension(ext3->id());
+  }
+  virtual void Verify() {
+    std::string actual;
+    actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val2", actual);
+    actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ("val4", actual);
+  }
+};
+TEST_F(ExtensionPrefsUninstallExtensionFromTop,
+    ExtensionPrefsUninstallExtensionFromTop) {}
+
+// Tests uninstalling an extension that was winning for only some preferences.
+class ExtensionPrefsUninstallExtensionFromMiddle
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    InstallExtControlledPref(ext2, kPref1, Value::CreateStringValue("val2"));
+    InstallExtControlledPref(ext3, kPref1, Value::CreateStringValue("val3"));
+
+    InstallExtControlledPref(ext1, kPref2, Value::CreateStringValue("val4"));
+    InstallExtControlledPref(ext2, kPref2, Value::CreateStringValue("val5"));
+
+    InstallExtControlledPref(ext1, kPref3, Value::CreateStringValue("val6"));
+
+    InstallExtControlledPref(ext2, kPref4, Value::CreateStringValue("val7"));
+
+    UninstallExtension(ext2->id());
+  }
+  virtual void Verify() {
+    std::string actual;
+    actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val3", actual);
+    actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ("val4", actual);
+    actual = prefs()->pref_service()->GetString(kPref3);
+    EXPECT_EQ("val6", actual);
+    actual = prefs()->pref_service()->GetString(kPref4);
+    EXPECT_EQ(kDefaultPref4, actual);
+  }
+};
+TEST_F(ExtensionPrefsUninstallExtensionFromMiddle,
+    ExtensionPrefsUninstallExtensionFromMiddle) {}
+
+// Tests uninstalling an extension that was winning for only some preferences.

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Update comment.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

[Mattias Nissler (ping if slow), 2010/11/19 16:52:20]

Not done?

[battre (please use the other), 2010/11/19 18:00:39]

Done.

+class ExtensionPrefsNotifyWhenNeeded
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    using testing::_;
+    using testing::Mock;
+    using testing::StrEq;
+
+    scoped_ptr<MockObserver> observer(new MockObserver());
+    PrefChangeRegistrar registrar;
+    registrar.Init(prefs()->pref_service());
+    registrar.Add(kPref1, observer.get());
+
+    EXPECT_CALL(*observer, Observe(_, _, _));
+    InstallExtControlledPref(ext1, kPref1,
+        Value::CreateStringValue("https://www.chromium.org"));
+    Mock::VerifyAndClearExpectations(observer.get());
+
+    EXPECT_CALL(*observer, Observe(_, _, _)).Times(0);
+    InstallExtControlledPref(ext1, kPref1,
+        Value::CreateStringValue("https://www.chromium.org"));
+    Mock::VerifyAndClearExpectations(observer.get());
+
+    EXPECT_CALL(*observer, Observe(_, _, _)).Times(2);
+    InstallExtControlledPref(ext1, kPref1,
+        Value::CreateStringValue("chrome://newtab"));
+
+    UninstallExtension(ext1->id());
+    registrar.Remove(kPref1, observer.get());
+  }
+  virtual void Verify() {
+    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ(kDefaultPref1, actual);
+  }
+};
+TEST_F(ExtensionPrefsNotifyWhenNeeded,
+    ExtensionPrefsNotifyWhenNeeded) {}
+
+// Tests disabling an extension
+class ExtensionPrefsDisableExt
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val1", actual);
+    prefs()->SetExtensionState(ext1, Extension::DISABLED);
+  }
+  virtual void Verify() {
+    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ(kDefaultPref1, actual);
+  }
+};
+TEST_F(ExtensionPrefsDisableExt,  ExtensionPrefsDisableExt) {}
+
+// Tests disabling and reenabling an extension
+class ExtensionPrefsReenableExt
+    : public ExtensionPrefsPreferencesBase {
+  virtual void Initialize() {
+    InstallExtControlledPref(ext1, kPref1, Value::CreateStringValue("val1"));
+    prefs()->SetExtensionState(ext1, Extension::DISABLED);
+    prefs()->SetExtensionState(ext1, Extension::ENABLED);
+  }
+  virtual void Verify() {
+    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    EXPECT_EQ("val1", actual);
+  }
+};
+TEST_F(ExtensionPrefsDisableExt,  ExtensionPrefsReenableExt) {}
+

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

No trailing newlines please.

[battre (please use the other), 2010/11/19 16:03:18]

Hopefully done. Eclipse messes this up.

[Mattias Nissler (ping if slow), 2010/11/19 16:52:20]

I see, not a big deal :)
However, you should get to the point where you control your editor and not the
other way round :)