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

11 files changed, 296 insertions, 13 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();
 }
 
diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc
index 370b1ff..d32ef5a 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -390,7 +390,7 @@ int BrowserActionsContainer::GetCurrentTabId() const {
 
 BrowserActionView* BrowserActionsContainer::GetBrowserActionView(
     Extension* extension) {
-  for (std::vector<BrowserActionView*>::iterator iter =
+  for (BrowserActionViews::iterator iter =
        browser_action_views_.begin(); iter != browser_action_views_.end();
        ++iter) {
     if ((*iter)->button()->extension() == extension)
@@ -859,6 +859,7 @@ void BrowserActionsContainer::WriteDragData(
           browser_action_views_[i]->GetIconWithBadge());
       drag_utils::SetDragImageOnDataObject(*canvas, button->width(),
           button->height(), press_x, press_y, data);
+
       // Fill in the remaining info.
       BrowserActionDragData drag_data(
           browser_action_views_[i]->button()->extension()->id(), i);
@@ -931,7 +932,7 @@ void BrowserActionsContainer::BrowserActionAdded(Extension* extension,
 
   // Add the new browser action to the vector and the view hierarchy.
   BrowserActionView* view = new BrowserActionView(extension, this);
-  browser_action_views_.push_back(view);
+  browser_action_views_.insert(browser_action_views_.begin() + index, view);
   AddChildView(index, view);
 
   // For details on why we do the following see the class comments in the
@@ -940,7 +941,8 @@ void BrowserActionsContainer::BrowserActionAdded(Extension* extension,
   // Determine if we need to increase (we only do that if the container was
   // showing all icons before the addition of this icon). We use -1 because
   // we don't want to count the view that we just added.
-  if (visible_actions < browser_action_views_.size() - 1) {
+  if (visible_actions < browser_action_views_.size() - 1 ||
+      extension->being_upgraded()) {
     // Some icons were hidden, don't increase the size of the container.
     OnBrowserActionVisibilityChanged();
   } else {
@@ -964,10 +966,7 @@ void BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
   if (popup_ && popup_->host()->extension() == extension)
     HidePopup();
 
-  // Before we change anything, determine the number of visible browser actions.
-  size_t visible_actions = VisibleBrowserActions();
-
-  for (std::vector<BrowserActionView*>::iterator iter =
+  for (BrowserActionViews::iterator iter =
        browser_action_views_.begin(); iter != browser_action_views_.end();
        ++iter) {
     if ((*iter)->button()->extension() == extension) {
@@ -975,9 +974,18 @@ void BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
       delete *iter;
       browser_action_views_.erase(iter);
 
+      // If the extension is being upgraded we don't want the bar to shrink
+      // because the icon is just going to get re-added to the same location.
+      if (extension->being_upgraded())
+        return;
+
       // For details on why we do the following see the class comments in the
       // header.
 
+      // Before we change anything, determine the number of visible browser
+      // actions.
+      size_t visible_actions = VisibleBrowserActions();
+
       // Calculate the target size we'll animate to (end state). This might be
       // the same size (if the icon we are removing is in the overflow bucket
       // and there are other icons there). We don't decrement visible_actions
diff --git a/chrome/browser/views/browser_actions_container.h b/chrome/browser/views/browser_actions_container.h
index 147b48b..acd868f 100644
--- a/chrome/browser/views/browser_actions_container.h
+++ b/chrome/browser/views/browser_actions_container.h
@@ -352,6 +352,8 @@ class BrowserActionsContainer
   ExtensionPopup* TestGetPopup() { return popup_; }
 
  private:
+  typedef std::vector<BrowserActionView*> BrowserActionViews;
+
   // ExtensionToolbarModel::Observer implementation.
   virtual void BrowserActionAdded(Extension* extension, int index);
   virtual void BrowserActionRemoved(Extension* extension);
@@ -396,7 +398,7 @@ class BrowserActionsContainer
   int ContainerMinSize() const;
 
   // The vector of browser actions (icons/image buttons for each action).
-  std::vector<BrowserActionView*> browser_action_views_;
+  BrowserActionViews browser_action_views_;
 
   NotificationRegistrar registrar_;
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 5b86a66..6357cbd 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1122,6 +1122,7 @@
         'browser/extensions/extension_messages_apitest.cc',
         'browser/extensions/extension_override_apitest.cc',
         'browser/extensions/extension_processes_apitest.cc',
+        'browser/extensions/extension_toolbar_model_unittest.cc',
         'browser/extensions/extension_toolstrip_apitest.cc',
         'browser/extensions/incognito_noscript_apitest.cc',
         'browser/extensions/isolated_world_apitest.cc',
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index f1dbbfd..1420f59 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -284,6 +284,11 @@ class Extension {
   bool GetBackgroundPageReady();
   void SetBackgroundPageReady();
 
+  // Getter and setter for the flag that specifies whether the extension is
+  // being upgraded.
+  bool being_upgraded() const { return being_upgraded_; }
+  void set_being_upgraded(bool value) { being_upgraded_ = value; }
+
   // App stuff.
   const URLPatternList& app_extent() const { return app_extent_; }
   const GURL& app_launch_url() const { return app_launch_url_; }
@@ -432,6 +437,9 @@ class Extension {
   // True if the background page is ready.
   bool background_page_ready_;
 
+  // True while the extension is being upgraded.
+  bool being_upgraded_;
+
   FRIEND_TEST(ExtensionTest, LoadPageActionHelper);
   FRIEND_TEST(TabStripModelTest, Apps);
 
diff --git a/chrome/test/data/extensions/api_test/browser_action/none/background.html b/chrome/test/data/extensions/api_test/browser_action/none/background.html
new file mode 100644
index 0000000..430e09a
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/none/background.html
@@ -0,0 +1,6 @@
+<html>
+<head>
+<script>
+</script>
+</head>
+</html>
diff --git a/chrome/test/data/extensions/api_test/browser_action/none/icon.png b/chrome/test/data/extensions/api_test/browser_action/none/icon.png
new file mode 100644
index 0000000..9a79a46
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/none/icon.png
diff --git a/chrome/test/data/extensions/api_test/browser_action/none/manifest.json b/chrome/test/data/extensions/api_test/browser_action/none/manifest.json
new file mode 100644
index 0000000..3ce6d75
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/none/manifest.json
@@ -0,0 +1,8 @@
+{
+  "name": "An extension with no browser action",
+  "version": "1.0",
+  "background_page": "background.html",
+  "permissions": [
+    "tabs", "http://*/*"
+  ]
+}