diff options
| author | rdevlin.cronin <rdevlin.cronin@chromium.org> | 2014-08-28 14:06:55 -0700 |
|---|---|---|
| committer | Commit bot <commit-bot@chromium.org> | 2014-08-28 21:08:21 +0000 |
| commit | 9a1800e4df575c7f7035cfbfddb84dcf879d217c (patch) | |
| tree | f2b1e94131faedfed1a17749c975e1cdc5d19697 /chrome/browser/extensions | |
| parent | db0482c7dae49d6d35530d867deb995c7ae97fb3 (diff) | |
| download | chromium_src-9a1800e4df575c7f7035cfbfddb84dcf879d217c.zip chromium_src-9a1800e4df575c7f7035cfbfddb84dcf879d217c.tar.gz chromium_src-9a1800e4df575c7f7035cfbfddb84dcf879d217c.tar.bz2 | |
Make a ShowExtensionActionPopup function
More-or-less consolidate calls to show an ExtensionAction's
popup into ExtensionActionAPI. This has the happy effect of
reducing the amount of piping we have to do in the UI code
(cutting a few steps out of the Browser -> BrowserWindow ->
Toolbar -> BrowserActionsContainer -> BrowserActionView chain),
and, more importantly, lets page action command executions
happen in the BrowserActionsContainer when the redesign is
enabled.
Hopefully, this means that we can stop showing page actions
in the location bar with the switch enabled - but that's
another patch.
BUG=397259
TBR=sky@chromium.org (minor changes to test_browser_window)
Review URL: https://codereview.chromium.org/489183005
Cr-Commit-Position: refs/heads/master@{#292462}
Diffstat (limited to 'chrome/browser/extensions')
7 files changed, 99 insertions, 36 deletions
diff --git a/chrome/browser/extensions/api/extension_action/extension_action_api.cc b/chrome/browser/extensions/api/extension_action/extension_action_api.cc index 291aac2..cf7db41 100644 --- a/chrome/browser/extensions/api/extension_action/extension_action_api.cc +++ b/chrome/browser/extensions/api/extension_action/extension_action_api.cc @@ -29,6 +29,7 @@ #include "extensions/browser/image_util.h" #include "extensions/browser/notification_types.h" #include "extensions/common/error_utils.h" +#include "extensions/common/feature_switch.h" #include "ui/gfx/image/image.h" #include "ui/gfx/image/image_skia.h" @@ -48,7 +49,6 @@ const char kNoTabError[] = "No tab with id: *."; const char kOpenPopupError[] = "Failed to show popup either because there is an existing popup or another " "error occurred."; -const char kInternalError[] = "Internal error."; } // namespace @@ -171,13 +171,9 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction( int tab_id = SessionTabHelper::IdForTab(web_contents); - ExtensionActionManager* action_manager = - ExtensionActionManager::Get(static_cast<Profile*>(browser_context_)); - // TODO(devlin): Make a ExtensionActionManager::GetExtensionAction method. ExtensionAction* extension_action = - action_manager->GetBrowserAction(*extension); - if (!extension_action) - extension_action = action_manager->GetPageAction(*extension); + ExtensionActionManager::Get(browser_context_)->GetExtensionAction( + *extension); // Anything that calls this should have a page or browser action. DCHECK(extension_action); @@ -205,6 +201,29 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction( return ExtensionAction::ACTION_NONE; } +bool ExtensionActionAPI::ShowExtensionActionPopup( + const Extension* extension, + Browser* browser, + bool grant_active_tab_permissions) { + ExtensionAction* extension_action = + ExtensionActionManager::Get(browser_context_)->GetExtensionAction( + *extension); + if (!extension_action) + return false; + + if (extension_action->action_type() == ActionInfo::TYPE_PAGE && + !FeatureSwitch::extension_action_redesign()->IsEnabled()) { + // We show page actions in the location bar unless the new toolbar is + // enabled. + return browser->window()->GetLocationBar()->ShowPageActionPopup( + extension, grant_active_tab_permissions); + } else { + return ExtensionToolbarModel::Get(browser->profile())-> + ShowExtensionActionPopup( + extension, browser, grant_active_tab_permissions); + } +} + void ExtensionActionAPI::NotifyChange(ExtensionAction* extension_action, content::WebContents* web_contents, content::BrowserContext* context) { @@ -563,13 +582,19 @@ BrowserActionOpenPopupFunction::BrowserActionOpenPopupFunction() } bool BrowserActionOpenPopupFunction::RunAsync() { - ExtensionToolbarModel* model = ExtensionToolbarModel::Get(GetProfile()); - if (!model) { - error_ = kInternalError; - return false; - } - - if (!model->ShowBrowserActionPopup(extension_.get())) { + // We only allow the popup in the active window. + Browser* browser = chrome::FindLastActiveWithProfile( + GetProfile(), chrome::GetActiveDesktop()); + + // If there's no active browser, or the Toolbar isn't visible, abort. + // Otherwise, try to open a popup in the active browser. + // TODO(justinlin): Remove toolbar check when http://crbug.com/308645 is + // fixed. + if (!browser || + !browser->window()->IsActive() || + !browser->window()->IsToolbarVisible() || + !ExtensionActionAPI::Get(GetProfile())->ShowExtensionActionPopup( + extension_.get(), browser, false)) { error_ = kOpenPopupError; return false; } diff --git a/chrome/browser/extensions/api/extension_action/extension_action_api.h b/chrome/browser/extensions/api/extension_action/extension_action_api.h index 3830096..7d76b2c 100644 --- a/chrome/browser/extensions/api/extension_action/extension_action_api.h +++ b/chrome/browser/extensions/api/extension_action/extension_action_api.h @@ -83,6 +83,14 @@ class ExtensionActionAPI : public BrowserContextKeyedAPI { Browser* browser, bool grant_active_tab_permissions); + // Opens the popup for the given |extension| in the given |browser|'s window. + // If |grant_active_tab_permissions| is true, this grants the extension + // activeTab (so this should only be done if this is through a direct user + // action). + bool ShowExtensionActionPopup(const Extension* extension, + Browser* browser, + bool grant_active_tab_permissions); + // Notifies that there has been a change in the given |extension_action|. void NotifyChange(ExtensionAction* extension_action, content::WebContents* web_contents, diff --git a/chrome/browser/extensions/extension_action_manager.cc b/chrome/browser/extensions/extension_action_manager.cc index 125021a..14ab141 100644 --- a/chrome/browser/extensions/extension_action_manager.cc +++ b/chrome/browser/extensions/extension_action_manager.cc @@ -97,8 +97,7 @@ void PopulateMissingValues(const Extension& extension, if (action->default_icon()) *default_icon = *action->default_icon(); - const ExtensionIconSet& extension_icons = - extensions::IconsInfo::GetIcons(&extension); + const ExtensionIconSet& extension_icons = IconsInfo::GetIcons(&extension); std::string largest_icon = extension_icons.Get( extension_misc::EXTENSION_ICON_GIGANTOR, ExtensionIconSet::MATCH_SMALLER); @@ -193,7 +192,7 @@ ExtensionAction* ExtensionActionManager::GetSystemIndicator( // given profile. This could return NULL if the system indicator area is // unavailable on the current system. If so, return NULL to signal that // the system indicator area is unusable. - if (!extensions::SystemIndicatorManagerFactory::GetForProfile(profile_)) + if (!SystemIndicatorManagerFactory::GetForProfile(profile_)) return NULL; return GetOrCreateOrNull(&system_indicators_, extension, @@ -202,4 +201,10 @@ ExtensionAction* ExtensionActionManager::GetSystemIndicator( profile_); } +ExtensionAction* ExtensionActionManager::GetExtensionAction( + const Extension& extension) const { + ExtensionAction* action = GetBrowserAction(extension); + return action ? action : GetPageAction(extension); +} + } // namespace extensions diff --git a/chrome/browser/extensions/extension_action_manager.h b/chrome/browser/extensions/extension_action_manager.h index 8973397..0d78277 100644 --- a/chrome/browser/extensions/extension_action_manager.h +++ b/chrome/browser/extensions/extension_action_manager.h @@ -33,21 +33,26 @@ class ExtensionActionManager : public KeyedService, // shared between a profile and its incognito version. static ExtensionActionManager* Get(content::BrowserContext* browser_context); - // Retrieves the page action, or browser action for |extension|. + // Retrieves the page action, browser action, or system indicator for + // |extension|. // If the result is not NULL, it remains valid until the extension is // unloaded. - ExtensionAction* GetPageAction(const extensions::Extension& extension) const; - ExtensionAction* GetBrowserAction( - const extensions::Extension& extension) const; - ExtensionAction* GetSystemIndicator( - const extensions::Extension& extension) const; + ExtensionAction* GetPageAction(const Extension& extension) const; + ExtensionAction* GetBrowserAction(const Extension& extension) const; + ExtensionAction* GetSystemIndicator(const Extension& extension) const; + + // Returns either the PageAction or BrowserAction for |extension|, or NULL if + // none exists. Since an extension can only declare one of Browser|PageAction, + // this is okay to use anywhere you need a generic "ExtensionAction". + // Since SystemIndicators are used differently and don't follow this + // rule of mutual exclusion, they are not checked or returned. + ExtensionAction* GetExtensionAction(const Extension& extension) const; // Gets the best fit ExtensionAction for the given |extension|. This takes // into account |extension|'s browser or page actions, if any, along with its // name and any declared icons. scoped_ptr<ExtensionAction> GetBestFitAction( - const extensions::Extension& extension, - extensions::ActionInfo::Type type) const; + const Extension& extension, ActionInfo::Type type) const; private: // Implement ExtensionRegistryObserver. diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc index 35e51b2..1858603 100644 --- a/chrome/browser/extensions/extension_toolbar_model.cc +++ b/chrome/browser/extensions/extension_toolbar_model.cc @@ -569,13 +569,18 @@ void ExtensionToolbarModel::OnExtensionToolbarPrefChange() { } } -bool ExtensionToolbarModel::ShowBrowserActionPopup(const Extension* extension) { +bool ExtensionToolbarModel::ShowExtensionActionPopup( + const Extension* extension, + Browser* browser, + bool grant_active_tab) { ObserverListBase<Observer>::Iterator it(observers_); Observer* obs = NULL; + // Look for the Observer associated with the browser. + // This would be cleaner if we had an abstract class for the Toolbar UI + // (like we do for LocationBar), but sadly, we don't. while ((obs = it.GetNext()) != NULL) { - // Stop after first popup since it should only show in the active window. - if (obs->ShowExtensionActionPopup(extension)) - return true; + if (obs->GetBrowser() == browser) + return obs->ShowExtensionActionPopup(extension, grant_active_tab); } return false; } diff --git a/chrome/browser/extensions/extension_toolbar_model.h b/chrome/browser/extensions/extension_toolbar_model.h index c0d35d6..29e83b3 100644 --- a/chrome/browser/extensions/extension_toolbar_model.h +++ b/chrome/browser/extensions/extension_toolbar_model.h @@ -37,8 +37,8 @@ class ExtensionToolbarModel : public content::NotificationObserver, // A class which is informed of changes to the model; represents the view of // MVC. Also used for signaling view changes such as showing extension popups. - // TODO(devlin): Should this really be an observer? There should probably be - // only one (aka a Delegate)... + // TODO(devlin): Should this really be an observer? It acts more like a + // delegate. class Observer { public: // An extension has been added to the toolbar and should go at |index|. @@ -59,8 +59,11 @@ class ExtensionToolbarModel : public content::NotificationObserver, virtual void ToolbarExtensionUpdated(const Extension* extension) = 0; // Signal the |extension| to show the popup now in the active window. + // If |grant_active_tab| is true, then active tab permissions should be + // given to the extension (only do this if this is through a user action). // Returns true if a popup was slated to be shown. - virtual bool ShowExtensionActionPopup(const Extension* extension) = 0; + virtual bool ShowExtensionActionPopup(const Extension* extension, + bool grant_active_tab) = 0; // Signal when the container needs to be redrawn because of a size change, // and when the model has finished loading. @@ -75,6 +78,9 @@ class ExtensionToolbarModel : public content::NotificationObserver, // with the new set (and just assume the new set is different). virtual void ToolbarHighlightModeChanged(bool is_highlighting) = 0; + // Returns the browser associated with the Observer. + virtual Browser* GetBrowser() = 0; + protected: virtual ~Observer() {} }; @@ -112,9 +118,13 @@ class ExtensionToolbarModel : public content::NotificationObserver, void OnExtensionToolbarPrefChange(); - // Tells observers to display a popup without granting tab permissions and - // returns whether the popup was slated to be shown. - bool ShowBrowserActionPopup(const Extension* extension); + // Finds the Observer associated with |browser| and tells it to display a + // popup for the given |extension|. If |grant_active_tab| is true, this + // grants active tab permissions to the |extension|; only do this because of + // a direct user action. + bool ShowExtensionActionPopup(const Extension* extension, + Browser* browser, + bool grant_active_tab); // Ensures that the extensions in the |extension_ids| list are visible on the // toolbar. This might mean they need to be moved to the front (if they are in diff --git a/chrome/browser/extensions/extension_toolbar_model_unittest.cc b/chrome/browser/extensions/extension_toolbar_model_unittest.cc index e2631c2..9b90bb5 100644 --- a/chrome/browser/extensions/extension_toolbar_model_unittest.cc +++ b/chrome/browser/extensions/extension_toolbar_model_unittest.cc @@ -75,7 +75,8 @@ class ExtensionToolbarModelTestObserver virtual void ToolbarExtensionUpdated(const Extension* extension) OVERRIDE { } - virtual bool ShowExtensionActionPopup(const Extension* extension) OVERRIDE { + virtual bool ShowExtensionActionPopup(const Extension* extension, + bool grant_active_tab) OVERRIDE { return false; } @@ -87,6 +88,10 @@ class ExtensionToolbarModelTestObserver highlight_mode_count_ += is_highlighting ? 1 : -1; } + virtual Browser* GetBrowser() OVERRIDE { + return NULL; + } + ExtensionToolbarModel* model_; size_t inserted_count_; |