Apps on NTP should be in order of installation

chrome/browser/extensions/pending_extension_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/extensions/pending_extension_manager.h"
+
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/extensions/pending_extension_manager.h"
+#include "chrome/browser/extensions/pending_extension_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace {
 
 // Install predicate used by AddFromExternalUpdateUrl().
 bool AlwaysInstall(const Extension& extension) {
   return true;
 }
 
 }  // namespace
 
 PendingExtensionManager::PendingExtensionManager(
     const ExtensionServiceInterface& service)
     : service_(service) {
 }
 
 PendingExtensionManager::~PendingExtensionManager() {}
 
-bool PendingExtensionManager::GetById(
-    const std::string& id,
-    PendingExtensionInfo* out_pending_extension_info) const {
+const PendingExtensionInfo* PendingExtensionManager::GetById(
+    const std::string& id) const {
+  std::list<PendingExtensionInfo>::const_iterator iter =
+      std::find_if(pending_extension_list_.begin(),

[Aaron Boodman, 2012/04/09 17:02:14]

I know Dmitry suggested std::find in a previous review, but I think he was
imagining that you'd be able to compare PendingInfo structs directly.

Personally, I typically don't find find_if useful. It's just as easy to write
the for loop directly, and the code is easier to understand.

But you can leave it this way if you like, so long as you move the comparator
inside this module as suggested in my other comments.

[Dmitry Polukhin, 2012/04/10 08:57:26]

Actually you can use std::find directly you just need to make
PendingExtensionInfo comparable std::string. std::find prototype is:

template<class InputIterator, class T>
InputIterator find ( InputIterator first, InputIterator last, const T& value )

So T will be std::string (i.e. extensions ID) so deference iterator will return
PendingExtensionInfo that will be compared with 'value'. But IMHO, find_if is
also fine and might be easier to understand than direct compare between
PendingExtensionInfo and string.

[mitchellwrosen, 2012/04/13 22:20:18]

I went back and changed them all to explicit iteration, and THEN I saw Dmitry's
comment posted. I'll leave it be for this patch set, and if you guys want me to
change it back to using find_if, or find, that's fine too.

+                   pending_extension_list_.end(),
+                   IsId(id));
 
-  PendingExtensionMap::const_iterator it = pending_extension_map_.find(id);
-  if (it != pending_extension_map_.end()) {
-    *out_pending_extension_info = it->second;
+  if (iter != pending_extension_list_.end()) {
+    return &(*iter);
+  }
+
+  return NULL;
+}
+
+bool PendingExtensionManager::Remove(const std::string& id) {
+  std::list<PendingExtensionInfo>::iterator iter =
+      std::find_if(pending_extension_list_.begin(),
+                   pending_extension_list_.end(),
+                   IsId(id));
+
+  if (iter != pending_extension_list_.end()) {
+    pending_extension_list_.erase(iter);
     return true;
   }
 
   return false;
 }
 
-void PendingExtensionManager::Remove(const std::string& id) {
-  pending_extension_map_.erase(id);
-}
+bool PendingExtensionManager::IsIdPending(const std::string& id) const {
+  std::list<PendingExtensionInfo>::const_iterator iter =
+      std::find_if(pending_extension_list_.begin(),
+                   pending_extension_list_.end(),
+                   IsId(id));
 
-bool PendingExtensionManager::IsIdPending(const std::string& id) const {
-  return ContainsKey(pending_extension_map_, id);
+  return iter != pending_extension_list_.end();
 }
 
 bool PendingExtensionManager::AddFromSync(
     const std::string& id,
     const GURL& update_url,
     PendingExtensionInfo::ShouldAllowInstallPredicate should_allow_install,
     bool install_silently) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (service_.GetInstalledExtension(id)) {
@@ skipping 65 matching lines @@
       kUpdateUrl,
       &AlwaysInstall,
       kIsFromSync,
       kInstallSilently,
       install_source);
 
   return true;
 }
 
 void PendingExtensionManager::GetPendingIdsForUpdateCheck(
-    std::set<std::string>* out_ids_for_update_check) const {
-  PendingExtensionMap::const_iterator iter;
-  for (iter = pending_extension_map_.begin();
-       iter != pending_extension_map_.end();
+    std::list<std::string>* out_ids_for_update_check) const {
+  std::list<PendingExtensionInfo>::const_iterator iter;
+  for (iter = pending_extension_list_.begin();
+       iter != pending_extension_list_.end();
        ++iter) {
-    Extension::Location install_source = iter->second.install_source();
+    Extension::Location install_source = iter->install_source();
 
     // Some install sources read a CRX from the filesystem.  They can
     // not be fetched from an update URL, so don't include them in the
     // set of ids.
     if (install_source == Extension::EXTERNAL_PREF ||
         install_source == Extension::EXTERNAL_REGISTRY)
       continue;
 
-    out_ids_for_update_check->insert(iter->first);
+    out_ids_for_update_check->push_back(iter->id());
   }
 }
 
 bool PendingExtensionManager::AddExtensionImpl(
     const std::string& id,
     const GURL& update_url,
     PendingExtensionInfo::ShouldAllowInstallPredicate should_allow_install,
     bool is_from_sync,
     bool install_silently,
     Extension::Location install_source) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Will add a pending extension record unless this variable is set to false.
   bool should_add_pending_record = true;
 
-  if (ContainsKey(pending_extension_map_, id)) {
+  std::list<PendingExtensionInfo>::iterator iter =
+      std::find_if(pending_extension_list_.begin(),
+                   pending_extension_list_.end(),
+                   IsId(id));
+
+  if (iter != pending_extension_list_.end()) {
     // Bugs in this code will manifest as sporadic incorrect extension
     // locations in situations where multiple install sources run at the
     // same time. For example, on first login to a chrome os machine, an
     // extension may be requested by sync and the default extension set.
     // The following logging will help diagnose such issues.
     VLOG(1) << "Extension id " << id
             << " was entered for update more than once."
-            << "  old location: " << pending_extension_map_[id].install_source()
+            << "  old location: " << iter->install_source()
             << "  new location: " << install_source;
 
     Extension::Location higher_priority_location =
         Extension::GetHigherPriorityLocation(
-            install_source, pending_extension_map_[id].install_source());
+            install_source, iter->install_source());
 
     if (higher_priority_location == install_source) {
       VLOG(1) << "Overwrite existing record.";
 
     } else {
       VLOG(1) << "Keep existing record.";
       should_add_pending_record = false;
     }
   }
 
   if (should_add_pending_record) {
-    pending_extension_map_[id] = PendingExtensionInfo(
-        update_url,
-        should_allow_install,
-        is_from_sync,
-        install_silently,
-        install_source);
+    PendingExtensionInfo new_pending_record(id,
+                                            update_url,
+                                            should_allow_install,
+                                            is_from_sync,
+                                            install_silently,
+                                            install_source);
+
+    pending_extension_list_.push_back(new_pending_record);
     return true;
   }
   return false;
 }
 
 void PendingExtensionManager::AddForTesting(
     const std::string& id,
     const PendingExtensionInfo& pending_extension_info) {
-  pending_extension_map_[id] = pending_extension_info;
+  pending_extension_list_.push_back(pending_extension_info);
 }