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/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 {
-
-  PendingExtensionMap::const_iterator it = pending_extension_map_.find(id);
-  if (it != pending_extension_map_.end()) {
-    *out_pending_extension_info = it->second;
+  PendingExtensionList::const_iterator iter = GetExtensionListConstIterById(id);

[Aaron Boodman, 2012/04/05 22:06:43]

It would be cleaner to make the pending list store linked_ptr<PendingInfo>
rather than raw PendingInfo structures. That way this can just be PendingInfo*
GetPendingInfoByID() and return NULL in the case where one doesn't exist.

Also you could remove the default constructor from PendingInfo which is only
there to support STL.

+  if (iter != pending_extension_list_.end()) {
+    *out_pending_extension_info = iter->second;
     return true;
   }
 
   return false;
 }
 
-void PendingExtensionManager::Remove(const std::string& id) {
-  pending_extension_map_.erase(id);
+bool PendingExtensionManager::Remove(const std::string& id) {
+  PendingExtensionList::iterator iter = GetExtensionListIterById(id);
+  if (iter != pending_extension_list_.end()) {
+    pending_extension_list_.erase(iter);
+    return true;
+  }
+
+  return false;
 }
 
 bool PendingExtensionManager::IsIdPending(const std::string& id) const {
-  return ContainsKey(pending_extension_map_, id);
+  PendingExtensionList::const_iterator iter = GetExtensionListConstIterById(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 {
+  PendingExtensionList::const_iterator iter;
+  for (iter = pending_extension_list_.begin();
+       iter != pending_extension_list_.end();
        ++iter) {
     Extension::Location install_source = iter->second.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->first);
   }
 }
 
 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)) {
+  PendingExtensionList::iterator iter = GetExtensionListIterById(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->second.install_source()
             << "  new location: " << install_source;
 
     Extension::Location higher_priority_location =
         Extension::GetHigherPriorityLocation(
-            install_source, pending_extension_map_[id].install_source());
+            install_source, iter->second.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(update_url,
+                                            should_allow_install,
+                                            is_from_sync,
+                                            install_silently,
+                                            install_source);
+
+    pending_extension_list_.push_back(std::make_pair(id, 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(std::make_pair(id, pending_extension_info));
 }
+
+PendingExtensionManager::PendingExtensionList::iterator
+    PendingExtensionManager::GetExtensionListIterById(const std::string& id) {
+  PendingExtensionList::iterator iter;
+  for (iter = pending_extension_list_.begin();

[Dmitry Polukhin, 2012/04/06 08:10:05]

It is basically std::find algorithm and IMHO it would be better to use std::find
instead of these two functions. I don't like function that return only iterator
(not pair(begin,end)) because reading caller code it is hard to make sure that
appropriate end() iterator is used.

I would add extension ID to PendingExtensionInfo struct so PendingExtensionList
will be able to contain PendingExtensionInfo instead of pair and add compare
function to use standard find algorithm.

+       iter != pending_extension_list_.end();
+       ++iter) {
+    if (id == iter->first) {
+      return iter;
+    }
+  }
+
+  return iter;
+}
+
+PendingExtensionManager::PendingExtensionList::const_iterator
+    PendingExtensionManager::GetExtensionListConstIterById(
+        const std::string& id) const {
+  PendingExtensionList::const_iterator iter;
+  for (iter = pending_extension_list_.begin();
+       iter != pending_extension_list_.end();
+       ++iter) {
+    if (id == iter->first) {
+      return iter;
+    }
+  }
+
+  return iter;
+}