Preserve order of extensions when they auto-update.

Also added tests for the ExtensionToolbarModel.

BUG=33401
TEST=ExtensionToolbarModelTest (new test).

Review URL: http://codereview.chromium.org/587002

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

4 files changed, 255 insertions, 5 deletions

diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index d561120..4c9bd39 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -92,9 +92,19 @@ void ExtensionToolbarModel::AddExtension(Extension* extension) {
   if (!extension->browser_action())
     return;
 
-  toolitems_.push_back(extension);
-  FOR_EACH_OBSERVER(Observer, observers_,
-                    BrowserActionAdded(extension, toolitems_.size() - 1));
+  if (extension->id() == last_extension_removed_ &&
+      last_extension_removed_index_ < toolitems_.size()) {
+    toolitems_.insert(begin() + last_extension_removed_index_, extension);
+    FOR_EACH_OBSERVER(Observer, observers_,
+        BrowserActionAdded(extension, last_extension_removed_index_));
+  } else {
+    toolitems_.push_back(extension);
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(extension, toolitems_.size() - 1));
+  }
+
+  last_extension_removed_ = "";
+  last_extension_removed_index_ = -1;
 
   UpdatePrefs();
 }
@@ -105,6 +115,9 @@ void ExtensionToolbarModel::RemoveExtension(Extension* extension) {
     return;
   }
 
+  last_extension_removed_ = extension->id();
+  last_extension_removed_index_ = pos - begin();
+
   toolitems_.erase(pos);
   FOR_EACH_OBSERVER(Observer, observers_,
                     BrowserActionRemoved(extension));
diff --git a/chrome/browser/extensions/extension_toolbar_model.h b/chrome/browser/extensions/extension_toolbar_model.h
index 3889f3d..1718050 100644
--- a/chrome/browser/extensions/extension_toolbar_model.h
+++ b/chrome/browser/extensions/extension_toolbar_model.h
@@ -76,6 +76,12 @@ class ExtensionToolbarModel : public NotificationObserver {
   // Ordered list of browser action buttons.
   ExtensionList toolitems_;
 
+  // Keeps track of what the last extension to get disabled was.
+  std::string last_extension_removed_;
+
+  // Keeps track of where the last extension to get disabled was in the list.
+  size_t last_extension_removed_index_;
+
   NotificationRegistrar registrar_;
 };
 
diff --git a/chrome/browser/extensions/extension_toolbar_model_unittest.cc b/chrome/browser/extensions/extension_toolbar_model_unittest.cc
new file mode 100644
index 0000000..068df03
--- /dev/null
+++ b/chrome/browser/extensions/extension_toolbar_model_unittest.cc
@@ -0,0 +1,222 @@
+// Copyright (c) 2010 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/browser.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_toolbar_model.h"
+#include "chrome/browser/extensions/extensions_service.h"
+#include "chrome/test/in_process_browser_test.h"
+
+// An InProcessBrowserTest for testing the ExtensionToolbarModel.
+// TODO(erikkay) It's unfortunate that this needs to be an in-proc browser test.
+// It would be nice to refactor things so that ExtensionsService could run
+// without so much of the browser in place.
+class ExtensionToolbarModelTest : public ExtensionBrowserTest,
+                                  public ExtensionToolbarModel::Observer {
+ public:
+  virtual void SetUp() {
+    inserted_count_ = 0;
+    removed_count_ = 0;
+    moved_count_ = 0;
+
+    ExtensionBrowserTest::SetUp();
+  }
+
+  virtual Browser* CreateBrowser(Profile* profile) {
+    Browser* b = InProcessBrowserTest::CreateBrowser(profile);
+    ExtensionsService* service = b->profile()->GetExtensionsService();
+    model_ = service->toolbar_model();
+    model_->AddObserver(this);
+    return b;
+  }
+
+  virtual void CleanUpOnMainThread() {
+    model_->RemoveObserver(this);
+  }
+
+  virtual void BrowserActionAdded(Extension* extension, int index) {
+    inserted_count_++;
+  }
+
+  virtual void BrowserActionRemoved(Extension* extension) {
+    removed_count_++;
+  }
+
+  virtual void BrowserActionMoved(Extension* extension, int index) {
+    moved_count_++;
+  }
+
+  Extension* ExtensionAt(int index) {
+    for (ExtensionList::iterator i = model_->begin(); i < model_->end(); ++i) {
+      if (index-- == 0)
+        return *i;
+    }
+    return NULL;
+  }
+
+ protected:
+  ExtensionToolbarModel* model_;
+
+  int inserted_count_;
+  int removed_count_;
+  int moved_count_;
+};
+
+IN_PROC_BROWSER_TEST_F(ExtensionToolbarModelTest, Basic) {
+  // Load an extension with no browser action.
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("none")));
+
+  // This extension should not be in the model (has no browser action).
+  EXPECT_EQ(0, inserted_count_);
+  EXPECT_EQ(0u, model_->size());
+  ASSERT_EQ(NULL, ExtensionAt(0));
+
+  // Load an extension with a browser action.
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("basics")));
+
+  // We should now find our extension in the model.
+  EXPECT_EQ(1, inserted_count_);
+  EXPECT_EQ(1u, model_->size());
+  Extension* extension = ExtensionAt(0);
+  ASSERT_TRUE(NULL != extension);
+  EXPECT_STREQ("A browser action with no icon that makes the page red",
+               extension->name().c_str());
+
+  // Should be a no-op, but still fires the events.
+  model_->MoveBrowserAction(extension, 0);
+  EXPECT_EQ(1, moved_count_);
+  EXPECT_EQ(1u, model_->size());
+  Extension* extension2 = ExtensionAt(0);
+  EXPECT_EQ(extension, extension2);
+
+  UnloadExtension(extension->id());
+  EXPECT_EQ(1, removed_count_);
+  EXPECT_EQ(0u, model_->size());
+  EXPECT_EQ(NULL, ExtensionAt(0));
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionToolbarModelTest, ReorderAndReinsert) {
+  // Load an extension with a browser action.
+  FilePath extension_a_path(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("basics"));
+  ASSERT_TRUE(LoadExtension(extension_a_path));
+
+  // First extension loaded.
+  EXPECT_EQ(1, inserted_count_);
+  EXPECT_EQ(1u, model_->size());
+  Extension* extensionA = ExtensionAt(0);
+  ASSERT_TRUE(NULL != extensionA);
+  EXPECT_STREQ("A browser action with no icon that makes the page red",
+               extensionA->name().c_str());
+
+  // Load another extension with a browser action.
+  FilePath extension_b_path(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("popup"));
+  ASSERT_TRUE(LoadExtension(extension_b_path));
+
+  // Second extension loaded.
+  EXPECT_EQ(2, inserted_count_);
+  EXPECT_EQ(2u, model_->size());
+  Extension* extensionB = ExtensionAt(1);
+  ASSERT_TRUE(NULL != extensionB);
+  EXPECT_STREQ("Popup tester", extensionB->name().c_str());
+
+  // Load yet another extension with a browser action.
+  FilePath extension_c_path(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("remove_popup"));
+  ASSERT_TRUE(LoadExtension(extension_c_path));
+
+  // Third extension loaded.
+  EXPECT_EQ(3, inserted_count_);
+  EXPECT_EQ(3u, model_->size());
+  Extension* extensionC = ExtensionAt(2);
+  ASSERT_TRUE(NULL != extensionC);
+  EXPECT_STREQ("A page action which removes a popup.",
+               extensionC->name().c_str());
+
+  // Order is now A, B, C. Let's put C first.
+  model_->MoveBrowserAction(extensionC, 0);
+  EXPECT_EQ(1, moved_count_);
+  EXPECT_EQ(3u, model_->size());
+  EXPECT_EQ(extensionC, ExtensionAt(0));
+  EXPECT_EQ(extensionA, ExtensionAt(1));
+  EXPECT_EQ(extensionB, ExtensionAt(2));
+  EXPECT_EQ(NULL, ExtensionAt(3));
+
+  // Order is now C, A, B. Let's put A last.
+  model_->MoveBrowserAction(extensionA, 2);
+  EXPECT_EQ(2, moved_count_);
+  EXPECT_EQ(3u, model_->size());
+  EXPECT_EQ(extensionC, ExtensionAt(0));
+  EXPECT_EQ(extensionB, ExtensionAt(1));
+  EXPECT_EQ(extensionA, ExtensionAt(2));
+  EXPECT_EQ(NULL, ExtensionAt(3));
+
+  // Order is now C, B, A. Let's remove B.
+  std::string idB = extensionB->id();
+  UnloadExtension(idB);
+  EXPECT_EQ(1, removed_count_);
+  EXPECT_EQ(2u, model_->size());
+  EXPECT_EQ(extensionC, ExtensionAt(0));
+  EXPECT_EQ(extensionA, ExtensionAt(1));
+  EXPECT_EQ(NULL, ExtensionAt(2));
+
+  // Load extension B again.
+  ASSERT_TRUE(LoadExtension(extension_b_path));
+
+  // Extension B loaded again.
+  EXPECT_EQ(4, inserted_count_);
+  EXPECT_EQ(3u, model_->size());
+  // Make sure it gets its old spot in the list. We should get the same
+  // extension again, otherwise the order has changed.
+  ASSERT_STREQ(idB.c_str(), ExtensionAt(1)->id().c_str());
+
+  // Unload B again.
+  UnloadExtension(idB);
+  EXPECT_EQ(2, removed_count_);
+  EXPECT_EQ(2u, model_->size());
+  EXPECT_EQ(extensionC, ExtensionAt(0));
+  EXPECT_EQ(extensionA, ExtensionAt(1));
+  EXPECT_EQ(NULL, ExtensionAt(2));
+
+  // Order is now C, A. Flip it.
+  model_->MoveBrowserAction(extensionA, 0);
+  EXPECT_EQ(3, moved_count_);
+  EXPECT_EQ(2u, model_->size());
+  EXPECT_EQ(extensionA, ExtensionAt(0));
+  EXPECT_EQ(extensionC, ExtensionAt(1));
+  EXPECT_EQ(NULL, ExtensionAt(2));
+
+  // Move A to the location it already occupies.
+  model_->MoveBrowserAction(extensionA, 0);
+  EXPECT_EQ(4, moved_count_);
+  EXPECT_EQ(2u, model_->size());
+  EXPECT_EQ(extensionA, ExtensionAt(0));
+  EXPECT_EQ(extensionC, ExtensionAt(1));
+  EXPECT_EQ(NULL, ExtensionAt(2));
+
+  // Order is now A, C. Remove C.
+  std::string idC = extensionC->id();
+  UnloadExtension(idC);
+  EXPECT_EQ(3, removed_count_);
+  EXPECT_EQ(1u, model_->size());
+  EXPECT_EQ(extensionA, ExtensionAt(0));
+  EXPECT_EQ(NULL, ExtensionAt(1));
+
+  // Load extension C again.
+  ASSERT_TRUE(LoadExtension(extension_c_path));
+
+  // Extension C loaded again.
+  EXPECT_EQ(5, inserted_count_);
+  EXPECT_EQ(2u, model_->size());
+  // Make sure it gets its old spot in the list (at the very end).
+  ASSERT_STREQ(idC.c_str(), ExtensionAt(1)->id().c_str());
+}
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 3834ed5..48dfc2f 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -627,6 +627,13 @@ void ExtensionsService::OnExtensionLoaded(Extension* extension,
             allow_privilege_increase || !Extension::IsPrivilegeIncrease(
                 old, extension);
 
+        // Extensions get upgraded if silent upgrades are allowed, otherwise
+        // they get disabled.
+        if (allow_silent_upgrade) {
+          old->set_being_upgraded(true);
+          extension->set_being_upgraded(true);
+        }
+
         // To upgrade an extension in place, unload the old one and
         // then load the new one.
         UnloadExtension(old->id());
@@ -690,6 +697,8 @@ void ExtensionsService::OnExtensionLoaded(Extension* extension,
     }
   }
 
+  extension->set_being_upgraded(false);
+
   UpdateActiveExtensionsInCrashReporter();
 }