Make a platform-independent ToolbarActionViewController

chrome/browser/ui/extensions/extension_action_view_controller.h

Index: chrome/browser/ui/extensions/extension_action_view_controller.h
diff --git a/chrome/browser/ui/extensions/extension_action_view_controller.h b/chrome/browser/ui/extensions/extension_action_view_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..2a9c5953f0e9e971d88d9050507d7f2aa4bb7339
--- /dev/null
+++ b/chrome/browser/ui/extensions/extension_action_view_controller.h
@@ -0,0 +1,130 @@
+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_UI_EXTENSIONS_EXTENSION_ACTION_VIEW_CONTROLLER_H_
+#define CHROME_BROWSER_UI_EXTENSIONS_EXTENSION_ACTION_VIEW_CONTROLLER_H_
+
+#include "chrome/browser/extensions/extension_action_icon_factory.h"
+#include "chrome/browser/extensions/extension_context_menu_model.h"
+#include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
+#include "ui/gfx/image/image.h"
+
+class Browser;
+class ExtensionAction;
+class GURL;
+
+namespace extensions {
+class Command;
+class Extension;
+}
+
+class ExtensionActionViewController

[sky, 2014/10/23 20:24:30]

Add a description.

[Devlin, 2014/10/23 20:50:32]

Done.

+    : public ToolbarActionViewController,
+      public ExtensionActionIconFactory::Observer,
+      public ExtensionContextMenuModel::PopupDelegate {
+ public:
+  ExtensionActionViewController(const extensions::Extension* extension,
+                                Browser* browser,
+                                ExtensionAction* extension_action);
+  ~ExtensionActionViewController() override;
+
+  // ToolbarActionViewController:
+  const std::string& GetId() const override;
+  void SetDelegate(ToolbarActionViewDelegate* delegate) override;
+  gfx::Image GetIcon(content::WebContents* web_contents) override;
+  gfx::ImageSkia GetIconWithBadge() override;
+  base::string16 GetActionName() const override;
+  base::string16 GetAccessibleName(content::WebContents* web_contents) const
+      override;
+  base::string16 GetTooltip(content::WebContents* web_contents) const override;
+  bool IsEnabled(content::WebContents* web_contents) const override;
+  bool HasPopup(content::WebContents* web_contents) const override;
+  void HidePopup() override;
+  bool CanDrag() const override;
+  bool ExecuteAction(bool by_user) override;
+  void PaintExtra(gfx::Canvas* canvas,
+                  const gfx::Rect& bounds,
+                  content::WebContents* web_contents) const override;
+
+  // ExtensionContextMenuModel::PopupDelegate:
+  void InspectPopup() override;
+
+  const extensions::Extension* extension() const { return extension_; }
+  Browser* browser() { return browser_; }
+  ExtensionAction* extension_action() { return extension_action_; }
+  const ExtensionAction* extension_action() const { return extension_action_; }
+
+ protected:
+  enum PopupShowAction {
+    SHOW_POPUP,
+    SHOW_POPUP_AND_INSPECT
+  };
+
+  ToolbarActionViewDelegate* delegate() const { return delegate_; }
+
+  // Populates |command| with the command associated with |extension|, if one
+  // exists. Returns true if |command| was populated.
+  bool GetExtensionCommand(extensions::Command* command);
+
+  // Returns true if there is currently a popup for this extension action.
+  virtual bool IsShowingPopup() const = 0;

[sky, 2014/10/23 20:24:30]

Is there a reason you're going with subclassing instead of moving this to a
delegate that is owned by ExtensionActionViewController? A delegate seems a bit
cleaner for the case you have here.

[Devlin, 2014/10/23 20:50:32]

Two reasons:
- I thought a delegate might get confusing, since we already have a
ToolbarActionViewDelegate.  We can, of course, have a "PopupDelegate" and a
"ViewDelegate", but I wasn't sure that would be cleaner.
- ExtensionActionViewDelegate can't (directly) implement the methods
GetPopupView(), IsMenuRunning(), and RegisterCommand() (since these are handled
differently on cocoa/views).  These, too, could be ported to the delegate, with
the implementation in ExtensionActionViewController simply being a forward to
those, but it seemed a little messier that way.

Would you prefer the delegate solution?  (Has the advantage of giving us a
concrete controller, which is quite nice.)

+
+  // Closes the active popup (whether it was this action's popup or not).
+  virtual void CloseActivePopup() = 0;
+
+  // Closes this action's popup. This will only be called if the popup is
+  // showing.
+  virtual void ClosePopupImpl() = 0;
+
+  // See ShowPopupWithUrl() for documentation.
+  virtual bool ShowPopupWithUrlImpl(PopupShowAction show_action,
+                                    const GURL& popup_url,
+                                    bool grant_tab_permissions) = 0;
+
+  // Called once the delegate is set, in order to do any extra initialization.
+  virtual void OnDelegateSet() {}

[sky, 2014/10/23 20:24:30]

nit: don't inline virtual methods.

[Devlin, 2014/10/23 20:50:32]

Thought the rule was "don't inline non-empty virtual methods" (which is what
clang warns on).  But it appears the style guide is ambiguous on it, so OWNERs
rules. :)

Done.

+
+ private:
+  // ExtensionActionIconFactory::Observer:
+  void OnIconUpdated() override;
+
+  // Executes the extension action with |show_action|. If
+  // |grant_tab_permissions| is true, this will grant the extension active tab
+  // permissions. Only do this if this was done through a user action (and not
+  // e.g. an API). Returns true if a popup is shown.
+  bool ExecuteAction(PopupShowAction show_action,
+                     bool grant_tab_permissions);
+
+  // Shows the popup for the extension action, given the associated |popup_url|.
+  // |grant_tab_permissions| is true if active tab permissions should be given
+  // to the extension; this is only true if the popup is opened through a user
+  // action.
+  // Returns true if a popup is successfully shown.
+  bool ShowPopupWithUrl(PopupShowAction show_action,
+                        const GURL& popup_url,
+                        bool grant_tab_permissions);
+
+  // The extension associated with the action we're displaying.
+  const extensions::Extension* extension_;
+
+  // The corresponding browser.
+  Browser* browser_;
+
+  // The browser action this view represents. The ExtensionAction is not owned
+  // by this class.
+  ExtensionAction* extension_action_;
+
+  // Our delegate.
+  ToolbarActionViewDelegate* delegate_;
+
+  // The object that will be used to get the browser action icon for us.
+  // It may load the icon asynchronously (in which case the initial icon
+  // returned by the factory will be transparent), so we have to observe it for
+  // updates to the icon.
+  ExtensionActionIconFactory icon_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionActionViewController);
+};
+
+#endif  // CHROME_BROWSER_UI_EXTENSIONS_EXTENSION_ACTION_VIEW_CONTROLLER_H_