Remove stl_util's deletion function use from chrome/browser/extensions/api/content_settings.

chrome/browser/extensions/api/content_settings/content_settings_store.cc

Index: chrome/browser/extensions/api/content_settings/content_settings_store.cc
diff --git a/chrome/browser/extensions/api/content_settings/content_settings_store.cc b/chrome/browser/extensions/api/content_settings/content_settings_store.cc
index 3df22217d4eda7481216e34241e2ca84cd04c33c..38c83f9ddece81d0365f5c447cc02c5154b54bf5 100644
--- a/chrome/browser/extensions/api/content_settings/content_settings_store.cc
+++ b/chrome/browser/extensions/api/content_settings/content_settings_store.cc
@@ -4,15 +4,14 @@
 
 #include "chrome/browser/extensions/api/content_settings/content_settings_store.h"
 
+#include <algorithm>
 #include <memory>
 #include <set>
 #include <utility>
-#include <vector>
 
 #include "base/debug/alias.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
-#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/content_settings/content_settings_api_constants.h"
@@ -36,8 +35,10 @@ namespace helpers = content_settings_helpers;
 namespace keys = content_settings_api_constants;
 
 struct ContentSettingsStore::ExtensionEntry {
-  // Extension id
+  // Extension id.
   std::string id;
+  // Installation time.
+  base::Time install_time;
   // Whether extension is enabled in the profile.
   bool enabled;
   // Content settings.
@@ -53,7 +54,6 @@ ContentSettingsStore::ContentSettingsStore() {
 }
 
 ContentSettingsStore::~ContentSettingsStore() {
-  base::STLDeleteValues(&entries_);
 }
 
 std::unique_ptr<RuleIterator> ContentSettingsStore::GetRuleIterator(
@@ -61,16 +61,14 @@ std::unique_ptr<RuleIterator> ContentSettingsStore::GetRuleIterator(
     const content_settings::ResourceIdentifier& identifier,
     bool incognito) const {
   std::vector<std::unique_ptr<RuleIterator>> iterators;
-  // Iterate the extensions based on install time (last installed extensions
-  // first).
-  ExtensionEntryMap::const_reverse_iterator entry_it;
 
   // The individual |RuleIterators| shouldn't lock; pass |lock_| to the
   // |ConcatenationIterator| in a locked state.
   std::unique_ptr<base::AutoLock> auto_lock(new base::AutoLock(lock_));
 
-  for (entry_it = entries_.rbegin(); entry_it != entries_.rend(); ++entry_it) {
-    auto* entry = entry_it->second;
+  // Iterate the extensions based on install time (last installed extensions

[Devlin, 2016/12/14 15:54:01]

not your code, but something like "most recently installed" sounds better to me
than "last installed".

[Avi (use Gerrit), 2016/12/14 19:29:06]

Done.

+  // first).
+  for (const auto& entry : entries_) {
     if (!entry->enabled)
       continue;
 
@@ -128,13 +126,21 @@ void ContentSettingsStore::RegisterExtension(
     const base::Time& install_time,
     bool is_enabled) {
   base::AutoLock lock(lock_);
-  ExtensionEntryMap::iterator i = FindEntry(ext_id);
+  auto i = FindEntry(ext_id);

[Devlin, 2016/12/14 15:54:01]

Using auto with all these FindEntry's makes me a little nervous when we have
const and non-const iterator versions, since it makes it easier to accidentally
modify the value (the compiler will happily return the non-const iterator if you
perform a modification, even if you didn't mean to).  If we keep these, I wonder
if we could either not use auto or name them differently?

That said, it looks like when we get the const iterators, we really only need
the value.  I think it might clean some of these methods up to have FindEntry()
return a const ExtensionEntry* (or nullptr) (and then maybe rename the iterator
FindEntry to FindIterator or something?).  This would make it clear what the
purpose was in each case, and would get rid of a lot of the (*i)-> syntax.

e.g.

const ExtensionEntry* FindEntry(const std::string& id) {
  auto iter = FindIterator(id);
  return iter == entries_.end() ? nullptr : iter->get();
}

ExtensionEntries::iterator FindIterator(const std::string& id) {
  std::find_if(...);
}

WDYT?

[Avi (use Gerrit), 2016/12/14 19:29:06]

Sounds reasonable, though I can't delegate like that. FindEntry needs to be
const, and so it can't rely on a non-const FindIterator.

   ExtensionEntry* entry;

[Devlin, 2016/12/14 15:54:01]

init to nullptr

[Avi (use Gerrit), 2016/12/14 19:29:06]

Done.

   if (i != entries_.end()) {
-    entry = i->second;
+    entry = i->get();
   } else {
     entry = new ExtensionEntry;
-    entries_.insert(std::make_pair(install_time, entry));
+    entry->install_time = install_time;
+    entries_.push_back(base::WrapUnique(entry));
+    std::stable_sort(entries_.begin(), entries_.end(),

[Devlin, 2016/12/14 15:54:01]

Would it be better to use std::find_if() to find the position of the new item
and then use vector::insert?  Realistically, probably won't make that much of a
difference since the vector should be otherwise sorted, but it makes it more
clear what the state is, IMO.

[Avi (use Gerrit), 2016/12/14 19:29:06]

std::upper_bound?

[Devlin, 2016/12/14 19:54:34]

Even better. :)

+                     [](const std::unique_ptr<ExtensionEntry>& a,
+                        const std::unique_ptr<ExtensionEntry>& b) {
+                       // Sort in reverse-chronological order to maintain the
+                       // invariant.
+                       return a->install_time > b->install_time;
+                     });
   }
 
   entry->id = ext_id;
@@ -147,14 +153,13 @@ void ContentSettingsStore::UnregisterExtension(
   bool notify_incognito = false;
   {
     base::AutoLock lock(lock_);
-    ExtensionEntryMap::iterator i = FindEntry(ext_id);
+    auto i = FindEntry(ext_id);
     if (i == entries_.end())
       return;
-    notify = !i->second->settings.empty();
-    notify_incognito = !i->second->incognito_persistent_settings.empty() ||
-                       !i->second->incognito_session_only_settings.empty();
+    notify = !(*i)->settings.empty();
+    notify_incognito = !(*i)->incognito_persistent_settings.empty() ||
+                       !(*i)->incognito_session_only_settings.empty();
 
-    delete i->second;
     entries_.erase(i);
   }
   if (notify)
@@ -169,14 +174,14 @@ void ContentSettingsStore::SetExtensionState(
   bool notify_incognito = false;
   {
     base::AutoLock lock(lock_);
-    ExtensionEntryMap::const_iterator i = FindEntry(ext_id);
+    auto i = FindEntry(ext_id);
     if (i == entries_.end())
       return;
-    notify = !i->second->settings.empty();
-    notify_incognito = !i->second->incognito_persistent_settings.empty() ||
-                       !i->second->incognito_session_only_settings.empty();
+    notify = !(*i)->settings.empty();
+    notify_incognito = !(*i)->incognito_persistent_settings.empty() ||
+                       !(*i)->incognito_session_only_settings.empty();
 
-    i->second->enabled = is_enabled;
+    (*i)->enabled = is_enabled;
   }
   if (notify)
     NotifyOfContentSettingChanged(ext_id, false);
@@ -187,46 +192,46 @@ void ContentSettingsStore::SetExtensionState(
 OriginIdentifierValueMap* ContentSettingsStore::GetValueMap(
     const std::string& ext_id,
     ExtensionPrefsScope scope) {
-  ExtensionEntryMap::const_iterator i = FindEntry(ext_id);
+  auto i = FindEntry(ext_id);
   if (i != entries_.end()) {
     switch (scope) {
       case kExtensionPrefsScopeRegular:
-        return &(i->second->settings);
+        return &((*i)->settings);
       case kExtensionPrefsScopeRegularOnly:
         // TODO(bauerb): Implement regular-only content settings.
         NOTREACHED();
-        return NULL;
+        return nullptr;
       case kExtensionPrefsScopeIncognitoPersistent:
-        return &(i->second->incognito_persistent_settings);
+        return &((*i)->incognito_persistent_settings);
       case kExtensionPrefsScopeIncognitoSessionOnly:
-        return &(i->second->incognito_session_only_settings);
+        return &((*i)->incognito_session_only_settings);
     }
   }
-  return NULL;
+  return nullptr;
 }
 
 const OriginIdentifierValueMap* ContentSettingsStore::GetValueMap(
     const std::string& ext_id,
     ExtensionPrefsScope scope) const {
-  ExtensionEntryMap::const_iterator i = FindEntry(ext_id);
+  auto i = FindEntry(ext_id);
   if (i == entries_.end())
-    return NULL;
+    return nullptr;
 
   switch (scope) {
     case kExtensionPrefsScopeRegular:
-      return &(i->second->settings);
+      return &((*i)->settings);
     case kExtensionPrefsScopeRegularOnly:
       // TODO(bauerb): Implement regular-only content settings.
       NOTREACHED();
-      return NULL;
+      return nullptr;
     case kExtensionPrefsScopeIncognitoPersistent:
-      return &(i->second->incognito_persistent_settings);
+      return &((*i)->incognito_persistent_settings);
     case kExtensionPrefsScopeIncognitoSessionOnly:
-      return &(i->second->incognito_session_only_settings);
+      return &((*i)->incognito_session_only_settings);
   }
 
   NOTREACHED();
-  return NULL;
+  return nullptr;
 }
 
 void ContentSettingsStore::ClearContentSettingsForExtension(
@@ -375,24 +380,20 @@ bool ContentSettingsStore::OnCorrectThread() {
          BrowserThread::CurrentlyOn(BrowserThread::UI);
 }
 
-ContentSettingsStore::ExtensionEntryMap::iterator
+ContentSettingsStore::ExtensionEntries::iterator
 ContentSettingsStore::FindEntry(const std::string& ext_id) {
-  ExtensionEntryMap::iterator i;
-  for (i = entries_.begin(); i != entries_.end(); ++i) {
-    if (i->second->id == ext_id)
-      return i;
-  }
-  return entries_.end();
+  return std::find_if(entries_.begin(), entries_.end(),
+                      [ext_id](const std::unique_ptr<ExtensionEntry>& entry) {
+                        return entry->id == ext_id;
+                      });
 }
 
-ContentSettingsStore::ExtensionEntryMap::const_iterator
+ContentSettingsStore::ExtensionEntries::const_iterator
 ContentSettingsStore::FindEntry(const std::string& ext_id) const {
-  ExtensionEntryMap::const_iterator i;
-  for (i = entries_.begin(); i != entries_.end(); ++i) {
-    if (i->second->id == ext_id)
-      return i;
-  }
-  return entries_.end();
+  return std::find_if(entries_.begin(), entries_.end(),
+                      [ext_id](const std::unique_ptr<ExtensionEntry>& entry) {
+                        return entry->id == ext_id;
+                      });
 }
 
 }  // namespace extensions