Proper initialization for known_disabled.

As of http://crrev.com/237473 we can finally have proper initialization of this code path (before we would have no way to tell the difference between a user who has never initialized and one which initialized to an empty list which was pruned out on write).

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@238285 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/extension_prefs.cc

@@ -1200,16 +1200,18 @@ bool ExtensionPrefs::IsExtensionDisabled(
 }
 
 ExtensionIdList ExtensionPrefs::GetToolbarOrder() {
-  return GetExtensionPrefAsContainer<ExtensionIdList>(prefs::kExtensionToolbar);
+  ExtensionIdList id_list_out;
+  GetUserExtensionPrefIntoContainer(prefs::kExtensionToolbar, &id_list_out);
+  return id_list_out;
 }
 
 void ExtensionPrefs::SetToolbarOrder(const ExtensionIdList& extension_ids) {
   SetExtensionPrefFromContainer(prefs::kExtensionToolbar, extension_ids);
 }
 
-ExtensionIdSet ExtensionPrefs::GetKnownDisabled() {
-  return GetExtensionPrefAsContainer<ExtensionIdSet>(
-      prefs::kExtensionKnownDisabled);
+bool ExtensionPrefs::GetKnownDisabled(ExtensionIdSet* id_set_out) {
+  return GetUserExtensionPrefIntoContainer(prefs::kExtensionKnownDisabled,
+                                           id_set_out);
 }
 
 void ExtensionPrefs::SetKnownDisabled(const ExtensionIdSet& extension_ids) {
@@ -1869,21 +1871,28 @@ void ExtensionPrefs::RegisterProfilePrefs(
 }
 
 template <class ExtensionIdContainer>
-ExtensionIdContainer ExtensionPrefs::GetExtensionPrefAsContainer(
-    const char* pref) {
-  ExtensionIdContainer extension_ids;
-  const ListValue* list_of_values = prefs_->GetList(pref);
-  if (!list_of_values)
-    return extension_ids;
+bool ExtensionPrefs::GetUserExtensionPrefIntoContainer(
+    const char* pref,
+    ExtensionIdContainer* id_container_out) {
+  DCHECK(id_container_out->empty());
+
+  const Value* user_pref_value = prefs_->GetUserPrefValue(pref);
+  const ListValue* user_pref_as_list;
+  if (!user_pref_value || !user_pref_value->GetAsList(&user_pref_as_list))
+    return false;
 
   std::insert_iterator<ExtensionIdContainer> insert_iterator(
-      extension_ids, extension_ids.end());
+      *id_container_out, id_container_out->end());
   std::string extension_id;
-  for (size_t i = 0; i < list_of_values->GetSize(); ++i) {
-    if (list_of_values->GetString(i, &extension_id))
-      insert_iterator = extension_id;
+  for (base::ListValue::const_iterator value_it = user_pref_as_list->begin();
+       value_it != user_pref_as_list->end(); ++value_it) {
+    if (!(*value_it)->GetAsString(&extension_id)) {
+      NOTREACHED();
+      continue;
+    }
+    insert_iterator = extension_id;
   }
-  return extension_ids;
+  return true;
 }
 
 template <class ExtensionIdContainer>

chrome/browser/extensions/extension_prefs.h

@@ -182,8 +182,11 @@ class ExtensionPrefs : public ExtensionScopedPrefs,
   ExtensionIdList GetToolbarOrder();
   void SetToolbarOrder(const ExtensionIdList& extension_ids);
 
-  // Get/Set the list of known disabled extension IDs.
-  ExtensionIdSet GetKnownDisabled();
+  // Gets the set of known disabled extension IDs into |id_set_out|. Returns
+  // false iff the set of known disabled extension IDs hasn't been set yet.
+  bool GetKnownDisabled(ExtensionIdSet* id_set_out);
+
+  // Sets the set of known disabled extension IDs.
   void SetKnownDisabled(const ExtensionIdSet& extension_ids);
 
   // Called when an extension is installed, so that prefs get created.
@@ -608,10 +611,13 @@ class ExtensionPrefs : public ExtensionScopedPrefs,
   bool DoesExtensionHaveState(const std::string& id,
                               Extension::State check_state) const;
 
-  // Reads the list of strings for |pref| from prefs into an
-  // ExtensionIdContainer.
+  // Reads the list of strings for |pref| from user prefs into
+  // |id_container_out|. Returns false if the pref wasn't found in the user
+  // pref store.
   template <class ExtensionIdContainer>
-  ExtensionIdContainer GetExtensionPrefAsContainer(const char* pref);
+  bool GetUserExtensionPrefIntoContainer(
+      const char* pref,
+      ExtensionIdContainer* id_container_out);
 
   // Writes the list of strings contained in |strings| to |pref| in prefs.
   template <class ExtensionIdContainer>

chrome/browser/extensions/extension_prefs_unittest.cc

@@ -22,6 +22,7 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/test/mock_notification_observer.h"
+#include "extensions/common/extension.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_info.h"
@@ -102,18 +103,18 @@ class ExtensionPrefsToolbarOrder : public ExtensionPrefsTest {
     list_.push_back(prefs_.AddExtensionAndReturnId("1"));
     list_.push_back(prefs_.AddExtensionAndReturnId("2"));
     list_.push_back(prefs_.AddExtensionAndReturnId("3"));
-    std::vector<std::string> before_list = prefs()->GetToolbarOrder();
+    ExtensionIdList before_list = prefs()->GetToolbarOrder();
     EXPECT_TRUE(before_list.empty());
     prefs()->SetToolbarOrder(list_);
   }
 
   virtual void Verify() OVERRIDE {
-    std::vector<std::string> result = prefs()->GetToolbarOrder();
+    ExtensionIdList result = prefs()->GetToolbarOrder();
     ASSERT_EQ(list_, result);
   }
 
  private:
-  std::vector<std::string> list_;
+  ExtensionIdList list_;
 };
 TEST_F(ExtensionPrefsToolbarOrder, ToolbarOrder) {}
 
@@ -121,21 +122,30 @@ TEST_F(ExtensionPrefsToolbarOrder, ToolbarOrder) {}
 class ExtensionPrefsKnownDisabled : public ExtensionPrefsTest {
  public:
   virtual void Initialize() OVERRIDE {
+    ExtensionIdSet before_set;
+    EXPECT_FALSE(prefs()->GetKnownDisabled(&before_set));
+    EXPECT_TRUE(before_set.empty());
+
+    // Initialize to an empty list and confirm that GetKnownDisabled() returns
+    // true and an empty list.
+    prefs()->SetKnownDisabled(before_set);
+    EXPECT_TRUE(prefs()->GetKnownDisabled(&before_set));
+    EXPECT_TRUE(before_set.empty());
+
     set_.insert(prefs_.AddExtensionAndReturnId("1"));
     set_.insert(prefs_.AddExtensionAndReturnId("2"));
     set_.insert(prefs_.AddExtensionAndReturnId("3"));
-    std::set<std::string> before_set = prefs()->GetKnownDisabled();
-    EXPECT_TRUE(before_set.empty());
     prefs()->SetKnownDisabled(set_);
   }
 
   virtual void Verify() OVERRIDE {
-    std::set<std::string> result = prefs()->GetKnownDisabled();
+    ExtensionIdSet result;
+    EXPECT_TRUE(prefs()->GetKnownDisabled(&result));
     ASSERT_EQ(set_, result);
   }
 
  private:
-  std::set<std::string> set_;
+  ExtensionIdSet set_;
 };
 TEST_F(ExtensionPrefsKnownDisabled, KnownDisabled) {}
 
@@ -776,19 +786,19 @@ class ExtensionPrefsBlacklistedExtensions : public ExtensionPrefsTest {
 
   virtual void Verify() OVERRIDE {
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       EXPECT_EQ(ids, prefs()->GetBlacklistedExtensions());
     }
     prefs()->SetExtensionBlacklisted(extension_a_->id(), true);
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       ids.insert(extension_a_->id());
       EXPECT_EQ(ids, prefs()->GetBlacklistedExtensions());
     }
     prefs()->SetExtensionBlacklisted(extension_b_->id(), true);
     prefs()->SetExtensionBlacklisted(extension_c_->id(), true);
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       ids.insert(extension_a_->id());
       ids.insert(extension_b_->id());
       ids.insert(extension_c_->id());
@@ -796,7 +806,7 @@ class ExtensionPrefsBlacklistedExtensions : public ExtensionPrefsTest {
     }
     prefs()->SetExtensionBlacklisted(extension_a_->id(), false);
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       ids.insert(extension_b_->id());
       ids.insert(extension_c_->id());
       EXPECT_EQ(ids, prefs()->GetBlacklistedExtensions());
@@ -804,7 +814,7 @@ class ExtensionPrefsBlacklistedExtensions : public ExtensionPrefsTest {
     prefs()->SetExtensionBlacklisted(extension_b_->id(), false);
     prefs()->SetExtensionBlacklisted(extension_c_->id(), false);
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       EXPECT_EQ(ids, prefs()->GetBlacklistedExtensions());
     }
 
@@ -820,7 +830,7 @@ class ExtensionPrefsBlacklistedExtensions : public ExtensionPrefsTest {
 
     EXPECT_TRUE(prefs()->GetExtensionPref(arbitrary_id));
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       ids.insert(arbitrary_id);
       ids.insert(extension_a_->id());
       EXPECT_EQ(ids, prefs()->GetBlacklistedExtensions());
@@ -829,7 +839,7 @@ class ExtensionPrefsBlacklistedExtensions : public ExtensionPrefsTest {
     prefs()->SetExtensionBlacklisted(extension_a_->id(), false);
     EXPECT_FALSE(prefs()->GetExtensionPref(arbitrary_id));
     {
-      std::set<std::string> ids;
+      ExtensionIdSet ids;
       EXPECT_EQ(ids, prefs()->GetBlacklistedExtensions());
     }
   }

chrome/browser/extensions/extension_service.cc

@@ -1424,9 +1424,8 @@ bool ExtensionService::IsUnacknowledgedExternalExtension(
 }
 
 void ExtensionService::ReconcileKnownDisabled() {
-  const ExtensionIdSet known_disabled_ids =
-      extension_prefs_->GetKnownDisabled();
-  if (known_disabled_ids.empty() && !disabled_extensions_.is_empty()) {
+  ExtensionIdSet known_disabled_ids;
+  if (!extension_prefs_->GetKnownDisabled(&known_disabled_ids)) {
     extension_prefs_->SetKnownDisabled(disabled_extensions_.GetIDs());
     UMA_HISTOGRAM_BOOLEAN("Extensions.KnownDisabledInitialized", true);
     return;