diff options
author | rdevlin.cronin <rdevlin.cronin@chromium.org> | 2014-10-07 14:18:53 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-10-07 21:19:14 +0000 |
commit | 267be09bdb873d429fac4ed309f20f191e7f470c (patch) | |
tree | 9b25ed3fcdda177fe2f65953dfc4b4ba4e38a56d | |
parent | 3531e1110c2c057b89a192b2ca05d95ca98f4aec (diff) | |
download | chromium_src-267be09bdb873d429fac4ed309f20f191e7f470c.zip chromium_src-267be09bdb873d429fac4ed309f20f191e7f470c.tar.gz chromium_src-267be09bdb873d429fac4ed309f20f191e7f470c.tar.bz2 |
Make ExtensionToolbarModel have an incognito version
Previously, the UI code was responsible for all the
incognito logic of the ExtensionToolbarModel. This led to
a) more confusion
b) more repetition (views and UI each implemented it)
c) less consistency (since incognito windows didn't share
prefs amongst themselves, which they should - like
cookies).
Fix this by making ExtensionToolbarModel have its own
incognito instance, which handles the saving of prefs (or
rather, not), and only has items which are enabled in
incognito.
BUG=420215
BUG=420216
Review URL: https://codereview.chromium.org/626833005
Cr-Commit-Position: refs/heads/master@{#298574}
11 files changed, 296 insertions, 366 deletions
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc index 03a46bb..ed8f079 100644 --- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc +++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc @@ -12,6 +12,7 @@ #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/extension_tab_util.h" #include "chrome/browser/extensions/extension_toolbar_model.h" +#include "chrome/browser/extensions/extension_util.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_commands.h" @@ -22,8 +23,10 @@ #include "content/public/browser/notification_service.h" #include "content/public/browser/web_contents.h" #include "content/public/test/browser_test_utils.h" +#include "extensions/browser/extension_registry.h" #include "extensions/browser/extension_system.h" #include "extensions/browser/notification_types.h" +#include "extensions/browser/test_extension_registry_observer.h" #include "extensions/common/feature_switch.h" #include "extensions/test/result_catcher.h" #include "grit/theme_resources.h" @@ -471,6 +474,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, IncognitoBasic) { // Open an incognito window and test that the browser action isn't there by // default. Profile* incognito_profile = browser()->profile()->GetOffTheRecordProfile(); + base::RunLoop().RunUntilIdle(); // Wait for profile initialization. Browser* incognito_browser = new Browser(Browser::CreateParams(incognito_profile, browser()->host_desktop_type())); @@ -479,15 +483,15 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, IncognitoBasic) { BrowserActionTestUtil(incognito_browser).NumberOfBrowserActions()); // Now enable the extension in incognito mode, and test that the browser - // action shows up. Note that we don't update the existing window at the - // moment, so we just create a new one. - extensions::ExtensionPrefs::Get(browser()->profile()) - ->SetIsIncognitoEnabled(extension->id(), true); + // action shows up. + // SetIsIncognitoEnabled() requires a reload of the extension, so we have to + // wait for it. + TestExtensionRegistryObserver registry_observer( + ExtensionRegistry::Get(profile()), extension->id()); + extensions::util::SetIsIncognitoEnabled( + extension->id(), browser()->profile(), true); + registry_observer.WaitForExtensionLoaded(); - chrome::CloseWindow(incognito_browser); - incognito_browser = - new Browser(Browser::CreateParams(incognito_profile, - browser()->host_desktop_type())); ASSERT_EQ(1, BrowserActionTestUtil(incognito_browser).NumberOfBrowserActions()); @@ -495,84 +499,6 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, IncognitoBasic) { // incognito. } -IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, IncognitoDragging) { - ExtensionService* service = extensions::ExtensionSystem::Get( - browser()->profile())->extension_service(); - - // The tooltips for each respective browser action. - const char kTooltipA[] = "Alpha"; - const char kTooltipB[] = "Beta"; - const char kTooltipC[] = "Gamma"; - - const size_t size_before = service->extensions()->size(); - - base::FilePath test_dir = test_data_dir_.AppendASCII("browser_action"); - const Extension* extension_a = InstallExtension( - test_dir.AppendASCII("empty_browser_action_alpha.crx"), 1); - const Extension* extension_b = InstallExtension( - test_dir.AppendASCII("empty_browser_action_beta.crx"), 1); - const Extension* extension_c = InstallExtension( - test_dir.AppendASCII("empty_browser_action_gamma.crx"), 1); - ASSERT_TRUE(extension_a); - ASSERT_TRUE(extension_b); - ASSERT_TRUE(extension_c); - - // Test that there are 3 browser actions in the toolbar. - ASSERT_EQ(size_before + 3, service->extensions()->size()); - ASSERT_EQ(3, GetBrowserActionsBar().NumberOfBrowserActions()); - - // Now enable 2 of the extensions in incognito mode, and test that the browser - // actions show up. - extensions::ExtensionPrefs* prefs = - extensions::ExtensionPrefs::Get(browser()->profile()); - prefs->SetIsIncognitoEnabled(extension_a->id(), true); - prefs->SetIsIncognitoEnabled(extension_c->id(), true); - - Profile* incognito_profile = browser()->profile()->GetOffTheRecordProfile(); - Browser* incognito_browser = - new Browser(Browser::CreateParams(incognito_profile, - browser()->host_desktop_type())); - BrowserActionTestUtil incognito_bar(incognito_browser); - - // Navigate just to have a tab in this window, otherwise wonky things happen. - ui_test_utils::OpenURLOffTheRecord(browser()->profile(), GURL("about:blank")); - - ASSERT_EQ(2, incognito_bar.NumberOfBrowserActions()); - - // Ensure that the browser actions are in the right order (ABC). - EXPECT_EQ(kTooltipA, GetBrowserActionsBar().GetTooltip(0)); - EXPECT_EQ(kTooltipB, GetBrowserActionsBar().GetTooltip(1)); - EXPECT_EQ(kTooltipC, GetBrowserActionsBar().GetTooltip(2)); - - EXPECT_EQ(kTooltipA, incognito_bar.GetTooltip(0)); - EXPECT_EQ(kTooltipC, incognito_bar.GetTooltip(1)); - - // Now rearrange them and ensure that they are rearranged correctly in both - // regular and incognito mode. - - // ABC -> CAB - ExtensionToolbarModel* toolbar_model = ExtensionToolbarModel::Get( - browser()->profile()); - toolbar_model->MoveExtensionIcon(extension_c, 0); - - EXPECT_EQ(kTooltipC, GetBrowserActionsBar().GetTooltip(0)); - EXPECT_EQ(kTooltipA, GetBrowserActionsBar().GetTooltip(1)); - EXPECT_EQ(kTooltipB, GetBrowserActionsBar().GetTooltip(2)); - - EXPECT_EQ(kTooltipC, incognito_bar.GetTooltip(0)); - EXPECT_EQ(kTooltipA, incognito_bar.GetTooltip(1)); - - // CAB -> CBA - toolbar_model->MoveExtensionIcon(extension_b, 1); - - EXPECT_EQ(kTooltipC, GetBrowserActionsBar().GetTooltip(0)); - EXPECT_EQ(kTooltipB, GetBrowserActionsBar().GetTooltip(1)); - EXPECT_EQ(kTooltipA, GetBrowserActionsBar().GetTooltip(2)); - - EXPECT_EQ(kTooltipC, incognito_bar.GetTooltip(0)); - EXPECT_EQ(kTooltipA, incognito_bar.GetTooltip(1)); -} - // Tests that events are dispatched to the correct profile for split mode // extensions. IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, IncognitoSplit) { @@ -587,6 +513,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, IncognitoSplit) { Browser* incognito_browser = new Browser(Browser::CreateParams(incognito_profile, browser()->host_desktop_type())); + base::RunLoop().RunUntilIdle(); // Wait for profile initialization. // Navigate just to have a tab in this window, otherwise wonky things happen. ui_test_utils::OpenURLOffTheRecord(browser()->profile(), GURL("about:blank")); ASSERT_EQ(1, diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc index 1858603..71ad468 100644 --- a/chrome/browser/extensions/extension_toolbar_model.cc +++ b/chrome/browser/extensions/extension_toolbar_model.cc @@ -53,11 +53,15 @@ ExtensionToolbarModel::ExtensionToolbarModel(Profile* profile, base::Bind(&ExtensionToolbarModel::OnReady, weak_ptr_factory_.GetWeakPtr())); visible_icon_count_ = prefs_->GetInteger(pref_names::kToolbarSize); - pref_change_registrar_.Init(prefs_); - pref_change_callback_ = - base::Bind(&ExtensionToolbarModel::OnExtensionToolbarPrefChange, - base::Unretained(this)); - pref_change_registrar_.Add(pref_names::kToolbar, pref_change_callback_); + + // We only care about watching the prefs if not in incognito mode. + if (!profile_->IsOffTheRecord()) { + pref_change_registrar_.Init(prefs_); + pref_change_callback_ = + base::Bind(&ExtensionToolbarModel::OnExtensionToolbarPrefChange, + base::Unretained(this)); + pref_change_registrar_.Add(pref_names::kToolbar, pref_change_callback_); + } } ExtensionToolbarModel::~ExtensionToolbarModel() { @@ -77,7 +81,7 @@ void ExtensionToolbarModel::RemoveObserver(Observer* observer) { } void ExtensionToolbarModel::MoveExtensionIcon(const Extension* extension, - int index) { + size_t index) { ExtensionList::iterator pos = std::find(toolbar_items_.begin(), toolbar_items_.end(), extension); if (pos == toolbar_items_.end()) { @@ -86,32 +90,26 @@ void ExtensionToolbarModel::MoveExtensionIcon(const Extension* extension, } toolbar_items_.erase(pos); - ExtensionIdList::iterator pos_id; - pos_id = std::find(last_known_positions_.begin(), - last_known_positions_.end(), extension->id()); + ExtensionIdList::iterator pos_id = std::find(last_known_positions_.begin(), + last_known_positions_.end(), + extension->id()); if (pos_id != last_known_positions_.end()) last_known_positions_.erase(pos_id); - int i = 0; - bool inserted = false; - for (ExtensionList::iterator iter = toolbar_items_.begin(); - iter != toolbar_items_.end(); - ++iter, ++i) { - if (i == index) { - pos_id = std::find(last_known_positions_.begin(), - last_known_positions_.end(), (*iter)->id()); - last_known_positions_.insert(pos_id, extension->id()); - - toolbar_items_.insert(iter, make_scoped_refptr(extension)); - inserted = true; - break; - } - } - - if (!inserted) { - DCHECK_EQ(index, static_cast<int>(toolbar_items_.size())); + if (index < toolbar_items_.size()) { + // If the index is not at the end, find the item currently at |index|, and + // insert |extension| before it in both |toolbar_items_| and + // |last_known_positions_|. + ExtensionList::iterator iter = toolbar_items_.begin() + index; + last_known_positions_.insert(std::find(last_known_positions_.begin(), + last_known_positions_.end(), + (*iter)->id()), + extension->id()); + toolbar_items_.insert(iter, extension); + } else { + // Otherwise, put |extension| at the end. + DCHECK_EQ(toolbar_items_.size(), index); index = toolbar_items_.size(); - toolbar_items_.push_back(make_scoped_refptr(extension)); last_known_positions_.push_back(extension->id()); } @@ -126,10 +124,11 @@ void ExtensionToolbarModel::SetVisibleIconCount(int count) { visible_icon_count_ = count == static_cast<int>(toolbar_items_.size()) ? -1 : count; - // Only set the prefs if we're not in highlight mode. Highlight mode is - // designed to be a transitory state, and should not persist across browser - // restarts (though it may be re-entered). - if (!is_highlighting_) { + // Only set the prefs if we're not in highlight mode and the profile is not + // incognito. Highlight mode is designed to be a transitory state, and should + // not persist across browser restarts (though it may be re-entered), and we + // don't store anything in incognito. + if (!is_highlighting_ && !profile_->IsOffTheRecord()) { // Additionally, if we are using the new toolbar, any icons which are in the // overflow menu are considered "hidden". But it so happens that the times // we are likely to call SetVisibleIconCount() are also those when we are @@ -212,7 +211,7 @@ void ExtensionToolbarModel::Observe( int new_size = 0; int new_index = 0; if (visible) { - // If this action used to be hidden, we can't possible be showing all. + // If this action used to be hidden, we can't possibly be showing all. DCHECK_NE(-1, visible_icon_count_); // Grow the bar by one and move the extension to the end of the visibles. new_size = visible_icon_count_ + 1; @@ -239,7 +238,7 @@ void ExtensionToolbarModel::Observe( void ExtensionToolbarModel::OnReady() { ExtensionRegistry* registry = ExtensionRegistry::Get(profile_); - InitializeExtensionList(registry->enabled_extensions()); + InitializeExtensionList(); // Wait until the extension system is ready before observing any further // changes so that the toolbar buttons can be shown in their stable ordering // taken from prefs. @@ -264,7 +263,7 @@ size_t ExtensionToolbarModel::FindNewPositionFromLastKnownGood( // Found an id, need to see if it is visible. for (ExtensionList::const_iterator iter_ext = toolbar_items_.begin(); iter_ext < toolbar_items_.end(); ++iter_ext) { - if ((*iter_ext)->id().compare(*iter_id) == 0) { + if ((*iter_ext)->id() == (*iter_id)) { // This extension is visible, update the index value. ++new_index; break; @@ -277,6 +276,11 @@ size_t ExtensionToolbarModel::FindNewPositionFromLastKnownGood( } bool ExtensionToolbarModel::ShouldAddExtension(const Extension* extension) { + // In incognito mode, don't add any extensions that aren't incognito-enabled. + if (profile_->IsOffTheRecord() && + !util::IsIncognitoEnabled(extension->id(), profile_)) + return false; + ExtensionActionManager* action_manager = ExtensionActionManager::Get(profile_); if (include_all_extensions_) { @@ -284,8 +288,7 @@ bool ExtensionToolbarModel::ShouldAddExtension(const Extension* extension) { // we want to show each extension regardless. // TODO(devlin): Extension actions which are not visible should be moved to // the overflow menu by default. - return action_manager->GetBrowserAction(*extension) || - action_manager->GetPageAction(*extension); + return action_manager->GetExtensionAction(*extension) != NULL; } return action_manager->GetBrowserAction(*extension) && @@ -297,24 +300,16 @@ void ExtensionToolbarModel::AddExtension(const Extension* extension) { if (!ShouldAddExtension(extension)) return; - size_t new_index = toolbar_items_.size(); - // See if we have a last known good position for this extension. - ExtensionIdList::iterator last_pos = std::find(last_known_positions_.begin(), - last_known_positions_.end(), - extension->id()); - if (last_pos != last_known_positions_.end()) { - new_index = FindNewPositionFromLastKnownGood(extension); - if (new_index != toolbar_items_.size()) { - toolbar_items_.insert(toolbar_items_.begin() + new_index, - make_scoped_refptr(extension)); - } else { - toolbar_items_.push_back(make_scoped_refptr(extension)); - } - } else { - // This is a never before seen extension, that was added to the end. Make - // sure to reflect that. (|new_index| was set above.) - toolbar_items_.push_back(make_scoped_refptr(extension)); + bool is_new_extension = + std::find(last_known_positions_.begin(), + last_known_positions_.end(), + extension->id()) == last_known_positions_.end(); + size_t new_index = is_new_extension ? toolbar_items_.size() : + FindNewPositionFromLastKnownGood(extension); + toolbar_items_.insert(toolbar_items_.begin() + new_index, + make_scoped_refptr(extension)); + if (is_new_extension) { last_known_positions_.push_back(extension->id()); UpdatePrefs(); } @@ -359,6 +354,16 @@ void ExtensionToolbarModel::RemoveExtension(const Extension* extension) { UpdatePrefs(); } +void ExtensionToolbarModel::ClearItems() { + size_t items_count = toolbar_items_.size(); + for (size_t i = 0; i < items_count; ++i) { + const Extension* extension = toolbar_items_.back().get(); + toolbar_items_.pop_back(); + FOR_EACH_OBSERVER(Observer, observers_, ToolbarExtensionRemoved(extension)); + } + DCHECK(toolbar_items_.empty()); +} + // Combine the currently enabled extensions that have browser actions (which // we get from the ExtensionRegistry) with the ordering we get from the // pref service. For robustness we use a somewhat inefficient process: @@ -366,35 +371,33 @@ void ExtensionToolbarModel::RemoveExtension(const Extension* extension) { // have holes. // 2. Create a vector of extensions that did not have a pref value. // 3. Remove holes from the sorted vector and append the unsorted vector. -void ExtensionToolbarModel::InitializeExtensionList( - const ExtensionSet& extensions) { +void ExtensionToolbarModel::InitializeExtensionList() { last_known_positions_ = extension_prefs_->GetToolbarOrder(); - Populate(last_known_positions_, extensions); + if (profile_->IsOffTheRecord()) + IncognitoPopulate(); + else + Populate(last_known_positions_); extensions_initialized_ = true; MaybeUpdateVisibilityPrefs(); FOR_EACH_OBSERVER(Observer, observers_, ToolbarVisibleCountChanged()); } -void ExtensionToolbarModel::Populate(const ExtensionIdList& positions, - const ExtensionSet& extensions) { +void ExtensionToolbarModel::Populate(const ExtensionIdList& positions) { + DCHECK(!profile_->IsOffTheRecord()); + const ExtensionSet& extensions = + ExtensionRegistry::Get(profile_)->enabled_extensions(); // Items that have explicit positions. - ExtensionList sorted; - sorted.resize(positions.size(), NULL); + ExtensionList sorted(positions.size(), NULL); // The items that don't have explicit positions. ExtensionList unsorted; - ExtensionActionManager* extension_action_manager = - ExtensionActionManager::Get(profile_); - // Create the lists. int hidden = 0; - for (ExtensionSet::const_iterator it = extensions.begin(); - it != extensions.end(); - ++it) { - const Extension* extension = it->get(); - if (!ShouldAddExtension(extension)) { - if (extension_action_manager->GetBrowserAction(*extension)) + for (const scoped_refptr<const Extension>& extension : extensions) { + if (!ShouldAddExtension(extension.get())) { + if (!ExtensionActionAPI::GetBrowserActionVisibility(extension_prefs_, + extension->id())) ++hidden; continue; } @@ -404,48 +407,37 @@ void ExtensionToolbarModel::Populate(const ExtensionIdList& positions, if (pos != positions.end()) sorted[pos - positions.begin()] = extension; else - unsorted.push_back(make_scoped_refptr(extension)); + unsorted.push_back(extension); } - size_t items_count = toolbar_items_.size(); - for (size_t i = 0; i < items_count; i++) { - const Extension* extension = toolbar_items_.back().get(); - // By popping the extension here (before calling BrowserActionRemoved), - // we will not shrink visible count by one after BrowserActionRemoved - // calls SetVisibleCount. - toolbar_items_.pop_back(); - FOR_EACH_OBSERVER( - Observer, observers_, ToolbarExtensionRemoved(extension)); - } - DCHECK(toolbar_items_.empty()); + // Clear the current items, if any. + ClearItems(); // Merge the lists. toolbar_items_.reserve(sorted.size() + unsorted.size()); - for (ExtensionList::const_iterator iter = sorted.begin(); - iter != sorted.end(); ++iter) { + for (const scoped_refptr<const Extension>& extension : sorted) { // It's possible for the extension order to contain items that aren't // actually loaded on this machine. For example, when extension sync is on, // we sync the extension order as-is but double-check with the user before // syncing NPAPI-containing extensions, so if one of those is not actually // synced, we'll get a NULL in the list. This sort of case can also happen // if some error prevents an extension from loading. - if (iter->get() != NULL) { - toolbar_items_.push_back(*iter); + if (extension.get() != NULL) { + toolbar_items_.push_back(extension); FOR_EACH_OBSERVER( Observer, observers_, - ToolbarExtensionAdded(iter->get(), toolbar_items_.size() - 1)); + ToolbarExtensionAdded(extension.get(), toolbar_items_.size() - 1)); } } - for (ExtensionList::const_iterator iter = unsorted.begin(); - iter != unsorted.end(); ++iter) { - if (iter->get() != NULL) { - toolbar_items_.push_back(*iter); + for (const scoped_refptr<const Extension>& extension : unsorted) { + if (extension.get() != NULL) { + toolbar_items_.push_back(extension); FOR_EACH_OBSERVER( Observer, observers_, - ToolbarExtensionAdded(iter->get(), toolbar_items_.size() - 1)); + ToolbarExtensionAdded(extension.get(), toolbar_items_.size() - 1)); } } @@ -465,8 +457,40 @@ void ExtensionToolbarModel::Populate(const ExtensionIdList& positions, } } +void ExtensionToolbarModel::IncognitoPopulate() { + DCHECK(profile_->IsOffTheRecord()); + // Clear the current items, if any. + ClearItems(); + + const ExtensionToolbarModel* original_model = + ExtensionToolbarModel::Get(profile_->GetOriginalProfile()); + + // Find the absolute value of the original model's count. + int original_visible = original_model->GetVisibleIconCount(); + if (original_visible == -1) + original_visible = original_model->toolbar_items_.size(); + + // In incognito mode, we show only those extensions that are + // incognito-enabled. Further, any actions that were overflowed in regular + // mode are still overflowed. Order is the same as in regular mode. + visible_icon_count_ = 0; + for (ExtensionList::const_iterator iter = + original_model->toolbar_items_.begin(); + iter != original_model->toolbar_items_.end(); ++iter) { + if (ShouldAddExtension(iter->get())) { + toolbar_items_.push_back(*iter); + if (iter - original_model->toolbar_items_.begin() < original_visible) + ++visible_icon_count_; + FOR_EACH_OBSERVER( + Observer, + observers_, + ToolbarExtensionAdded(iter->get(), toolbar_items_.size() - 1)); + } + } +} + void ExtensionToolbarModel::UpdatePrefs() { - if (!extension_prefs_) + if (!extension_prefs_ || profile_->IsOffTheRecord()) return; // Don't observe change caused by self. @@ -479,7 +503,7 @@ void ExtensionToolbarModel::MaybeUpdateVisibilityPref( const Extension* extension, int index) { // We only update the visibility pref for hidden/not hidden based on the // overflow menu with the new toolbar design. - if (include_all_extensions_) { + if (include_all_extensions_ && !profile_->IsOffTheRecord()) { bool visible = index < visible_icon_count_ || visible_icon_count_ == -1; if (visible != ExtensionActionAPI::GetBrowserActionVisibility( extension_prefs_, extension->id())) { @@ -511,33 +535,6 @@ void ExtensionToolbarModel::MaybeUpdateVisibilityPrefs() { MaybeUpdateVisibilityPref(toolbar_items_[i].get(), i); } -int ExtensionToolbarModel::IncognitoIndexToOriginal(int incognito_index) { - int original_index = 0, i = 0; - for (ExtensionList::iterator iter = toolbar_items_.begin(); - iter != toolbar_items_.end(); - ++iter, ++original_index) { - if (util::IsIncognitoEnabled((*iter)->id(), profile_)) { - if (incognito_index == i) - break; - ++i; - } - } - return original_index; -} - -int ExtensionToolbarModel::OriginalIndexToIncognito(int original_index) { - int incognito_index = 0, i = 0; - for (ExtensionList::iterator iter = toolbar_items_.begin(); - iter != toolbar_items_.end(); - ++iter, ++i) { - if (original_index == i) - break; - if (util::IsIncognitoEnabled((*iter)->id(), profile_)) - ++incognito_index; - } - return incognito_index; -} - void ExtensionToolbarModel::OnExtensionToolbarPrefChange() { // If extensions are not ready, defer to later Populate() call. if (!extensions_initialized_) @@ -556,8 +553,7 @@ void ExtensionToolbarModel::OnExtensionToolbarPrefChange() { last_known_positions_.swap(pref_positions); // Re-populate. - Populate(last_known_positions_, - ExtensionRegistry::Get(profile_)->enabled_extensions()); + Populate(last_known_positions_); if (last_known_positions_.size() > pref_position_size) { // Need to update pref because we have extra icons. But can't call diff --git a/chrome/browser/extensions/extension_toolbar_model.h b/chrome/browser/extensions/extension_toolbar_model.h index b1fd01a..f4a2ad7 100644 --- a/chrome/browser/extensions/extension_toolbar_model.h +++ b/chrome/browser/extensions/extension_toolbar_model.h @@ -93,7 +93,7 @@ class ExtensionToolbarModel : public content::NotificationObserver, void RemoveObserver(Observer* observer); // Moves the given |extension|'s icon to the given |index|. - void MoveExtensionIcon(const Extension* extension, int index); + void MoveExtensionIcon(const Extension* extension, size_t index); // Sets the number of extension icons that should be visible. // If count == size(), this will set the visible icon count to -1, meaning @@ -111,11 +111,6 @@ class ExtensionToolbarModel : public content::NotificationObserver, bool is_highlighting() const { return is_highlighting_; } - // Utility functions for converting between an index into the list of - // incognito-enabled browser actions, and the list of all browser actions. - int IncognitoIndexToOriginal(int incognito_index); - int OriginalIndexToIncognito(int original_index); - void OnExtensionToolbarPrefChange(); // Finds the Observer associated with |browser| and tells it to display a @@ -173,11 +168,13 @@ class ExtensionToolbarModel : public content::NotificationObserver, content::BrowserContext* browser_context) override; // To be called after the extension service is ready; gets loaded extensions - // from the extension service and their saved order from the pref service - // and constructs |toolbar_items_| from these data. - void InitializeExtensionList(const ExtensionSet& extensions); - void Populate(const ExtensionIdList& positions, - const ExtensionSet& extensions); + // from the ExtensionRegistry and their saved order from the pref service + // and constructs |toolbar_items_| from these data. IncognitoPopulate() + // takes the shortcut - looking at the regular model's content and modifying + // it. + void InitializeExtensionList(); + void Populate(const ExtensionIdList& positions); + void IncognitoPopulate(); // Save the model to prefs. void UpdatePrefs(); @@ -200,6 +197,9 @@ class ExtensionToolbarModel : public content::NotificationObserver, void AddExtension(const Extension* extension); void RemoveExtension(const Extension* extension); + // Removes all current items (because we're going to [re]Populate()). + void ClearItems(); + // Our observers. ObserverList<Observer> observers_; diff --git a/chrome/browser/extensions/extension_toolbar_model_factory.cc b/chrome/browser/extensions/extension_toolbar_model_factory.cc index cfe7f9b..a51904c 100644 --- a/chrome/browser/extensions/extension_toolbar_model_factory.cc +++ b/chrome/browser/extensions/extension_toolbar_model_factory.cc @@ -45,7 +45,7 @@ KeyedService* ExtensionToolbarModelFactory::BuildServiceInstanceFor( content::BrowserContext* ExtensionToolbarModelFactory::GetBrowserContextToUse( content::BrowserContext* context) const { - return ExtensionsBrowserClient::Get()->GetOriginalContext(context); + return context; } bool ExtensionToolbarModelFactory::ServiceIsCreatedWithBrowserContext() const { diff --git a/chrome/browser/extensions/extension_toolbar_model_unittest.cc b/chrome/browser/extensions/extension_toolbar_model_unittest.cc index 71a79c0..c84bad6 100644 --- a/chrome/browser/extensions/extension_toolbar_model_unittest.cc +++ b/chrome/browser/extensions/extension_toolbar_model_unittest.cc @@ -9,6 +9,7 @@ #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/extension_service_test_base.h" #include "chrome/browser/extensions/extension_toolbar_model.h" +#include "chrome/browser/extensions/extension_toolbar_model_factory.h" #include "chrome/browser/extensions/test_extension_system.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/extensions/api/extension_action/action_info.h" @@ -26,6 +27,38 @@ namespace extensions { namespace { +// Creates a new ExtensionToolbarModel for the given |context|. +KeyedService* BuildToolbarModel(content::BrowserContext* context) { + return new ExtensionToolbarModel(Profile::FromBrowserContext(context), + ExtensionPrefs::Get(context)); +} + +// Given a |profile|, assigns the testing keyed service function to +// BuildToolbarModel() and uses it to create and initialize a new +// ExtensionToolbarModel. +ExtensionToolbarModel* CreateToolbarModelForProfile(Profile* profile) { + ExtensionToolbarModel* model = ExtensionToolbarModel::Get(profile); + if (model) + return model; + + // No existing model means it's a new profile (since we, by default, don't + // create the ToolbarModel in testing). + ExtensionToolbarModelFactory::GetInstance()->SetTestingFactory( + profile, &BuildToolbarModel); + model = ExtensionToolbarModel::Get(profile); + // Fake the extension system ready signal. + // HACK ALERT! In production, the ready task on ExtensionSystem (and most + // everything else on it, too) is shared between incognito and normal + // profiles, but a TestExtensionSystem doesn't have the concept of "shared". + // Because of this, we have to set any new profile's TestExtensionSystem's + // ready task, too. + static_cast<TestExtensionSystem*>(ExtensionSystem::Get(profile))->SetReady(); + // Run tasks posted to TestExtensionSystem. + base::RunLoop().RunUntilIdle(); + + return model; +} + // Create an extension. If |action_key| is non-NULL, it should point to either // kBrowserAction or kPageAction, and the extension will have the associated // action. @@ -137,9 +170,13 @@ class ExtensionToolbarModelUnitTest : public ExtensionServiceTestBase { // Returns the extension at the given index in the toolbar model, or NULL // if one does not exist. + // If |model| is specified, it is used. Otherwise, this defaults to + // |toolbar_model_|. + const Extension* GetExtensionAtIndex( + size_t index, const ExtensionToolbarModel* model) const; const Extension* GetExtensionAtIndex(size_t index) const; - ExtensionToolbarModel* toolbar_model() { return toolbar_model_.get(); } + ExtensionToolbarModel* toolbar_model() { return toolbar_model_; } const ExtensionToolbarModelTestObserver* observer() const { return model_observer_.get(); @@ -161,8 +198,8 @@ class ExtensionToolbarModelUnitTest : public ExtensionServiceTestBase { testing::AssertionResult AddAndVerifyExtensions( const ExtensionList& extensions); - // The associated (and owned) toolbar model. - scoped_ptr<ExtensionToolbarModel> toolbar_model_; + // The toolbar model associated with the testing profile. + ExtensionToolbarModel* toolbar_model_; // The test observer to track events. Must come after toolbar_model_ so that // it is destroyed and removes itself as an observer first. @@ -181,14 +218,8 @@ class ExtensionToolbarModelUnitTest : public ExtensionServiceTestBase { void ExtensionToolbarModelUnitTest::Init() { InitializeEmptyExtensionService(); - toolbar_model_.reset( - new ExtensionToolbarModel(profile(), ExtensionPrefs::Get(profile()))); - model_observer_.reset( - new ExtensionToolbarModelTestObserver(toolbar_model_.get())); - static_cast<TestExtensionSystem*>(ExtensionSystem::Get(profile()))-> - SetReady(); - // Run tasks posted to TestExtensionSystem. - base::RunLoop().RunUntilIdle(); + toolbar_model_ = CreateToolbarModelForProfile(profile()); + model_observer_.reset(new ExtensionToolbarModelTestObserver(toolbar_model_)); } testing::AssertionResult ExtensionToolbarModelUnitTest::AddExtension( @@ -253,12 +284,17 @@ ExtensionToolbarModelUnitTest::AddBrowserActionExtensions() { } const Extension* ExtensionToolbarModelUnitTest::GetExtensionAtIndex( - size_t index) const { - return index < toolbar_model_->toolbar_items().size() - ? toolbar_model_->toolbar_items()[index].get() + size_t index, const ExtensionToolbarModel* model) const { + return index < model->toolbar_items().size() + ? model->toolbar_items()[index].get() : NULL; } +const Extension* ExtensionToolbarModelUnitTest::GetExtensionAtIndex( + size_t index) const { + return GetExtensionAtIndex(index, toolbar_model_); +} + testing::AssertionResult ExtensionToolbarModelUnitTest::AddAndVerifyExtensions( const ExtensionList& extensions) { for (ExtensionList::const_iterator iter = extensions.begin(); @@ -739,6 +775,77 @@ TEST_F(ExtensionToolbarModelUnitTest, EXPECT_EQ(browser_action_c(), GetExtensionAtIndex(2u)); } +TEST_F(ExtensionToolbarModelUnitTest, ExtensionToolbarIncognitoModeTest) { + Init(); + ASSERT_TRUE(AddBrowserActionExtensions()); + + // Give two extensions incognito access. + // Note: We use ExtensionPrefs::SetIsIncognitoEnabled instead of + // util::SetIsIncognitoEnabled because the latter tries to reload the + // extension, which can cause problems in our unittest set up (and reloading + // the extension is irrelevant to us). + ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile()); + extension_prefs->SetIsIncognitoEnabled(browser_action_b()->id(), true); + extension_prefs->SetIsIncognitoEnabled(browser_action_c()->id(), true); + + // Move C to the second index. + toolbar_model()->MoveExtensionIcon(browser_action_c(), 1u); + // Set visible count to 2 so that c is overflowed. State is A C [B]. + toolbar_model()->SetVisibleIconCount(2); + EXPECT_EQ(1u, observer()->moved_count()); + + // Get an incognito profile and toolbar. + ExtensionToolbarModel* incognito_model = + CreateToolbarModelForProfile(profile()->GetOffTheRecordProfile()); + + ExtensionToolbarModelTestObserver incognito_observer(incognito_model); + EXPECT_EQ(0u, incognito_observer.moved_count()); + + // We should have two items, C and B, and the order should be preserved from + // the original model. + EXPECT_EQ(2u, incognito_model->toolbar_items().size()); + EXPECT_EQ(browser_action_c(), GetExtensionAtIndex(0u, incognito_model)); + EXPECT_EQ(browser_action_b(), GetExtensionAtIndex(1u, incognito_model)); + + // Extensions in the overflow menu in the regular toolbar should remain in + // overflow in the incognito toolbar. So, we should have C [B]. + EXPECT_EQ(1, incognito_model->GetVisibleIconCount()); + // The regular model should still have two icons visible. + EXPECT_EQ(2, toolbar_model()->GetVisibleIconCount()); + + // Changing the incognito model size should not affect the regular model. + incognito_model->SetVisibleIconCount(0); + EXPECT_EQ(0, incognito_model->GetVisibleIconCount()); + EXPECT_EQ(2, toolbar_model()->GetVisibleIconCount()); + + // Expanding the incognito model to 2 should register as "all icons" (-1), + // since it is all of the incognito-enabled extensions. + incognito_model->SetVisibleIconCount(2u); + EXPECT_EQ(-1, incognito_model->GetVisibleIconCount()); + + // Moving icons in the incognito toolbar should not affect the regular + // toolbar. Incognito currently has C B... + incognito_model->MoveExtensionIcon(browser_action_b(), 0u); + // So now it should be B C... + EXPECT_EQ(1u, incognito_observer.moved_count()); + EXPECT_EQ(browser_action_b(), GetExtensionAtIndex(0u, incognito_model)); + EXPECT_EQ(browser_action_c(), GetExtensionAtIndex(1u, incognito_model)); + // ... and the regular toolbar should be unaffected. + EXPECT_EQ(browser_action_a(), GetExtensionAtIndex(0u)); + EXPECT_EQ(browser_action_c(), GetExtensionAtIndex(1u)); + EXPECT_EQ(browser_action_b(), GetExtensionAtIndex(2u)); + + // Similarly, the observer for the regular model should not have received + // any updates. + EXPECT_EQ(1u, observer()->moved_count()); + + // And performing moves on the regular model should have no effect on the + // incognito model or its observers. + toolbar_model()->MoveExtensionIcon(browser_action_c(), 2u); + EXPECT_EQ(2u, observer()->moved_count()); + EXPECT_EQ(1u, incognito_observer.moved_count()); +} + // Test that hiding actions on the toolbar results in sending them to the // overflow menu when the redesign switch is enabled. TEST_F(ExtensionToolbarModelUnitTest, diff --git a/chrome/browser/ui/cocoa/extensions/browser_action_button.mm b/chrome/browser/ui/cocoa/extensions/browser_action_button.mm index 008b603..61af4e7 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_action_button.mm +++ b/chrome/browser/ui/cocoa/extensions/browser_action_button.mm @@ -109,7 +109,7 @@ class ExtensionActionIconFactoryBridge [cell setTabId:tabId]; ExtensionAction* browser_action = extensions::ExtensionActionManager::Get(browser->profile())-> - GetBrowserAction(*extension); + GetExtensionAction(*extension); CHECK(browser_action) << "Don't create a BrowserActionButton if there is no browser action."; [cell setExtensionAction:browser_action]; diff --git a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm index 497aeba..682f1b5 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm +++ b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm @@ -12,7 +12,6 @@ #include "chrome/browser/extensions/extension_action.h" #include "chrome/browser/extensions/extension_action_manager.h" #include "chrome/browser/extensions/extension_toolbar_model.h" -#include "chrome/browser/extensions/extension_util.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sessions/session_tab_helper.h" #include "chrome/browser/ui/browser.h" @@ -141,10 +140,6 @@ const CGFloat kBrowserActionBubbleYOffset = 3.0; shouldGrant:(BOOL)shouldGrant; - (BOOL)browserActionClicked:(BrowserActionButton*)button; -// Returns whether the given extension should be displayed. Only displays -// incognito-enabled extensions in incognito mode. Otherwise returns YES. -- (BOOL)shouldDisplayBrowserAction:(const Extension*)extension; - // The reason |frame| is specified in these chevron functions is because the // container may be animating and the end frame of the animation should be // passed instead of the current frame (which may be off and cause the chevron @@ -494,9 +489,6 @@ class ExtensionServiceObserverBridge for (ExtensionList::const_iterator iter = toolbarModel_->toolbar_items().begin(); iter != toolbarModel_->toolbar_items().end(); ++iter) { - if (![self shouldDisplayBrowserAction:iter->get()]) - continue; - [self createActionButtonForExtension:iter->get() withIndex:i++]; } @@ -506,16 +498,6 @@ class ExtensionServiceObserverBridge - (void)createActionButtonForExtension:(const Extension*)extension withIndex:(NSUInteger)index { - if (!extensions::ExtensionActionManager::Get(profile_)-> - GetBrowserAction(*extension)) - return; - - if (![self shouldDisplayBrowserAction:extension]) - return; - - if (profile_->IsOffTheRecord()) - index = toolbarModel_->OriginalIndexToIncognito(index); - // Show the container if it's the first button. Otherwise it will be shown // already. if ([self buttonCount] == 0) @@ -551,18 +533,11 @@ class ExtensionServiceObserverBridge } - (void)removeActionButtonForExtension:(const Extension*)extension { - if (!extensions::ActionInfo::GetBrowserActionInfo(extension)) - return; - NSString* buttonKey = base::SysUTF8ToNSString(extension->id()); if (!buttonKey) return; BrowserActionButton* button = [buttons_ objectForKey:buttonKey]; - // This could be the case in incognito, where only a subset of extensions are - // shown. - if (!button) - return; [button removeFromSuperview]; // It may or may not be hidden, but it won't matter to NSMutableArray either @@ -586,8 +561,6 @@ class ExtensionServiceObserverBridge for (ExtensionList::const_iterator iter = toolbarModel_->toolbar_items().begin(); iter != toolbarModel_->toolbar_items().end(); ++iter) { - if (![self shouldDisplayBrowserAction:iter->get()]) - continue; BrowserActionButton* button = [self buttonForExtension:(iter->get())]; if (!button) continue; @@ -697,8 +670,7 @@ class ExtensionServiceObserverBridge } [self updateGrippyCursors]; - if (!profile_->IsOffTheRecord()) - toolbarModel_->SetVisibleIconCount([self visibleButtonCount]); + toolbarModel_->SetVisibleIconCount([self visibleButtonCount]); [[NSNotificationCenter defaultCenter] postNotificationName:kBrowserActionGrippyDragFinishedNotification @@ -788,12 +760,6 @@ class ExtensionServiceObserverBridge shouldGrant:YES]; } -- (BOOL)shouldDisplayBrowserAction:(const Extension*)extension { - // Only display incognito-enabled extensions while in incognito mode. - return !profile_->IsOffTheRecord() || - extensions::util::IsIncognitoEnabled(extension->id(), profile_); -} - - (void)showChevronIfNecessaryInFrame:(NSRect)frame animate:(BOOL)animate { [self setChevronHidden:([self buttonCount] == [self visibleButtonCount]) inFrame:frame @@ -887,8 +853,6 @@ class ExtensionServiceObserverBridge #pragma mark Testing Methods - (NSButton*)buttonWithIndex:(NSUInteger)index { - if (profile_->IsOffTheRecord()) - index = toolbarModel_->IncognitoIndexToOriginal(index); const extensions::ExtensionList& toolbar_items = toolbarModel_->toolbar_items(); if (index < toolbar_items.size()) { diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.cc b/chrome/browser/ui/views/toolbar/browser_actions_container.cc index 9361759..4f52dd8 100644 --- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc +++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc @@ -7,7 +7,6 @@ #include "base/compiler_specific.h" #include "base/stl_util.h" #include "chrome/browser/extensions/extension_action_manager.h" -#include "chrome/browser/extensions/extension_util.h" #include "chrome/browser/extensions/extension_view_host.h" #include "chrome/browser/extensions/tab_helper.h" #include "chrome/browser/profiles/profile.h" @@ -94,7 +93,7 @@ BrowserActionsContainer::BrowserActionsContainer( owner_view_(owner_view), main_container_(main_container), popup_owner_(NULL), - model_(NULL), + model_(extensions::ExtensionToolbarModel::Get(browser->profile())), container_width_(0), resize_area_(NULL), chevron_(NULL), @@ -102,9 +101,7 @@ BrowserActionsContainer::BrowserActionsContainer( resize_amount_(0), animation_target_size_(0) { set_id(VIEW_ID_BROWSER_ACTION_TOOLBAR); - - model_ = extensions::ExtensionToolbarModel::Get(browser->profile()); - if (model_) + if (model_) // |model_| can be NULL in views unittests. model_->AddObserver(this); bool overflow_experiment = @@ -187,9 +184,6 @@ void BrowserActionsContainer::CreateBrowserActionViews() { const extensions::ExtensionList& toolbar_items = model_->toolbar_items(); for (extensions::ExtensionList::const_iterator i(toolbar_items.begin()); i != toolbar_items.end(); ++i) { - if (!ShouldDisplayBrowserAction(i->get())) - continue; - BrowserActionView* view = new BrowserActionView(i->get(), action_manager->GetExtensionAction(**i), @@ -234,17 +228,14 @@ void BrowserActionsContainer::ExecuteExtensionCommand( void BrowserActionsContainer::NotifyActionMovedToOverflow() { // When an action is moved to overflow, we shrink the size of the container // by 1. - if (!profile_->IsOffTheRecord()) { - int icon_count = model_->GetVisibleIconCount(); - // Since this happens when an icon moves from the main bar to overflow, we - // can't possibly have had no visible icons on the main bar. - DCHECK_NE(0, icon_count); - if (icon_count == -1) - icon_count = browser_action_views_.size(); - model_->SetVisibleIconCount(icon_count - 1); - } - Animate(gfx::Tween::EASE_OUT, - VisibleBrowserActionsAfterAnimation() - 1); + int icon_count = model_->GetVisibleIconCount(); + // Since this happens when an icon moves from the main bar to overflow, we + // can't possibly have had no visible icons on the main bar. + DCHECK_NE(0, icon_count); + if (icon_count == -1) + icon_count = browser_action_views_.size(); + model_->SetVisibleIconCount(icon_count - 1); + Animate(gfx::Tween::EASE_OUT, icon_count - 1); } bool BrowserActionsContainer::ShownInsideMenu() const { @@ -520,9 +511,6 @@ int BrowserActionsContainer::OnPerformDrop( if (i > data.index()) --i; - if (profile_->IsOffTheRecord()) - i = model_->IncognitoIndexToOriginal(i); - // If this was a drag between containers, we will have to adjust the number of // visible icons. bool drag_between_containers = @@ -537,7 +525,7 @@ int BrowserActionsContainer::OnPerformDrop( // Let the main container update the model. if (in_overflow_mode()) main_container_->NotifyActionMovedToOverflow(); - else if (!profile_->IsOffTheRecord()) // This is the main container. + else // This is the main container. model_->SetVisibleIconCount(model_->GetVisibleIconCount() + 1); // The size changed, so we need to animate. @@ -605,12 +593,8 @@ void BrowserActionsContainer::OnResize(int resize_amount, bool done_resizing) { // Save off the desired number of visible icons. We do this now instead of at // the end of the animation so that even if the browser is shut down while // animating, the right value will be restored on next run. - // NOTE: Don't save the icon count in incognito because there may be fewer - // icons in that mode. The result is that the container in a normal window is - // always at least as wide as in an incognito window. int visible_icons = WidthToIconCount(container_width_); - if (!profile_->IsOffTheRecord()) - model_->SetVisibleIconCount(visible_icons); + model_->SetVisibleIconCount(visible_icons); Animate(gfx::Tween::EASE_OUT, visible_icons); } @@ -709,7 +693,6 @@ void BrowserActionsContainer::OnThemeChanged() { void BrowserActionsContainer::ViewHierarchyChanged( const ViewHierarchyChangedDetails& details) { - // No extensions (e.g., incognito). if (!model_) return; @@ -758,12 +741,7 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension, if (chevron_) chevron_->CloseMenu(); - if (!ShouldDisplayBrowserAction(extension)) - return; - // Add the new browser action to the vector and the view hierarchy. - if (profile_->IsOffTheRecord()) - index = model_->OriginalIndexToIncognito(index); BrowserActionView* view = new BrowserActionView(extension, extensions::ExtensionActionManager::Get(profile_)-> @@ -839,12 +817,6 @@ void BrowserActionsContainer::ToolbarExtensionRemoved( void BrowserActionsContainer::ToolbarExtensionMoved(const Extension* extension, int index) { - if (!ShouldDisplayBrowserAction(extension)) - return; - - if (profile_->IsOffTheRecord()) - index = model_->OriginalIndexToIncognito(index); - DCHECK(index >= 0 && index < static_cast<int>(browser_action_views_.size())); BrowserActionViews::iterator iter = browser_action_views_.begin(); @@ -1006,47 +978,25 @@ void BrowserActionsContainer::Animate(gfx::Tween::Type tween_type, } } -bool BrowserActionsContainer::ShouldDisplayBrowserAction( - const Extension* extension) const { - // Only display incognito-enabled extensions while in incognito mode. - return !profile_->IsOffTheRecord() || - extensions::util::IsIncognitoEnabled(extension->id(), profile_); -} - size_t BrowserActionsContainer::GetIconCount() const { if (!model_) return 0u; - const extensions::ExtensionList& extensions = model_->toolbar_items(); - // Find the absolute value for the model's visible count. int model_visible_size = model_->GetVisibleIconCount(); - size_t absolute_model_visible_size = - model_visible_size == -1 ? extensions.size() : model_visible_size; - - // Find the number of icons which could be displayed. - size_t displayable_icon_count = 0u; - size_t main_displayed = 0u; - for (size_t i = 0; i < extensions.size(); ++i) { - // Should there be an icon for this extension at all? - if (ShouldDisplayBrowserAction(extensions[i].get())) { - ++displayable_icon_count; - // Should we display it on the main bar? If this is an incognito window, - // icons have the same overflow status they do in a regular window. - main_displayed += i < absolute_model_visible_size ? 1u : 0u; - } - } - - // If this is an existing (initialized) container from an incognito profile, - // we can't trust the model (because the incognito bars don't adjust model - // settings). Instead, we go off what we currently have displayed. - if (initialized_ && profile_->IsOffTheRecord()) { - main_displayed = in_overflow_mode() ? - main_container_->VisibleBrowserActionsAfterAnimation() : - VisibleBrowserActionsAfterAnimation(); + size_t absolute_model_visible_size = model_visible_size == -1 ? + model_->toolbar_items().size() : model_visible_size; + + // Good time for some sanity checks: We should never try to display more + // icons than we have, and we should always have a view per item in the model. + // (The only exception is if this is in initialization.) + if (initialized_) { + DCHECK_LE(absolute_model_visible_size, browser_action_views_.size()); + DCHECK_EQ(model_->toolbar_items().size(), browser_action_views_.size()); } - // The overflow displays any (displayable) icons not shown by the main bar. + // The overflow displays any icons not shown by the main bar. return in_overflow_mode() ? - displayable_icon_count - main_displayed : main_displayed; + model_->toolbar_items().size() - absolute_model_visible_size : + absolute_model_visible_size; } diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.h b/chrome/browser/ui/views/toolbar/browser_actions_container.h index 4bd522d..fb5a1e6 100644 --- a/chrome/browser/ui/views/toolbar/browser_actions_container.h +++ b/chrome/browser/ui/views/toolbar/browser_actions_container.h @@ -330,11 +330,6 @@ class BrowserActionsContainer // the target size). void Animate(gfx::Tween::Type type, size_t num_visible_icons); - // Returns true if this extension should be shown in this toolbar. This can - // return false if we are in an incognito window and the extension is disabled - // for incognito. - bool ShouldDisplayBrowserAction(const extensions::Extension* extension) const; - // Returns the number of icons that this container should draw. This differs // from the model's GetVisibleIconCount if this container is for the overflow. size_t GetIconCount() const; diff --git a/chrome/browser/ui/views/toolbar/chevron_menu_button.cc b/chrome/browser/ui/views/toolbar/chevron_menu_button.cc index e2b40df..207bcf6 100644 --- a/chrome/browser/ui/views/toolbar/chevron_menu_button.cc +++ b/chrome/browser/ui/views/toolbar/chevron_menu_button.cc @@ -321,8 +321,6 @@ int ChevronMenuButton::MenuController::OnPerformDrop( enabled_extensions().GetByID(drop_data.id()); extensions::ExtensionToolbarModel* toolbar_model = extensions::ExtensionToolbarModel::Get(profile); - if (profile->IsOffTheRecord()) - drop_index = toolbar_model->IncognitoIndexToOriginal(drop_index); toolbar_model->MoveExtensionIcon(extension, drop_index); // If the extension was moved to the overflow menu from the main bar, notify diff --git a/extensions/browser/test_extension_registry_observer.cc b/extensions/browser/test_extension_registry_observer.cc index d9a1575..4d87e94 100644 --- a/extensions/browser/test_extension_registry_observer.cc +++ b/extensions/browser/test_extension_registry_observer.cc @@ -4,36 +4,29 @@ #include "extensions/browser/test_extension_registry_observer.h" -#include "content/public/test/test_utils.h" +#include "base/run_loop.h" #include "extensions/browser/extension_registry.h" namespace extensions { class TestExtensionRegistryObserver::Waiter { public: - explicit Waiter(const std::string& extension_id) - : observed_(false), runner_(NULL) {} + explicit Waiter(const std::string& extension_id) : observed_(false) {} void Wait() { if (observed_) return; - - runner_ = new content::MessageLoopRunner(); - runner_->Run(); + run_loop_.Run(); } void OnObserved() { observed_ = true; - - if (runner_.get()) { - runner_->Quit(); - runner_ = NULL; - } + run_loop_.Quit(); } private: bool observed_; - scoped_refptr<content::MessageLoopRunner> runner_; + base::RunLoop run_loop_; DISALLOW_COPY_AND_ASSIGN(Waiter); }; |