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
diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index d5611201..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 @@
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 @@
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..17180506 100644
--- a/chrome/browser/extensions/extension_toolbar_model.h
+++ b/chrome/browser/extensions/extension_toolbar_model.h
@@ -76,6 +76,12 @@
// 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 @@
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 @@
}
}
+ 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..d32ef5a5 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -390,7 +390,7 @@
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 @@
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 @@
// 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 @@
// 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 @@
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 @@
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 @@
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 @@
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 5b86a667..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 @@
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 @@
// 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..430e09ae
--- /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
Binary files differ
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..3ce6d7522
--- /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://*/*"
+ ]
+}
