Add infrastructure for Chrome Actions

chrome/browser/ui/views/toolbar/toolbar_action_view_controller.h

+// 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_VIEWS_TOOLBAR_TOOLBAR_ACTION_VIEW_CONTROLLER_H_
+#define CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_ACTION_VIEW_CONTROLLER_H_
+
+#include "base/strings/string16.h"
+#include "ui/gfx/image/image.h"
+
+namespace gfx {
+class Canvas;
+class Rect;
+}
+
+class ToolbarActionViewDelegate;
+
+// The basic controller class for actions that are shown on the toolbar,
+// extension actions (like browser actions) and chrome actions (like
+// chromecast).
+class ToolbarActionViewController {

[sky, 2014/10/15 23:52:30]

I'm surprised this class doesn't know about the number of actions? How is one
suppose valid ids?

[Devlin, 2014/10/16 16:46:39]

There's one controller per action per view.

+ public:
+  virtual ~ToolbarActionViewController() {}
+
+  // Sets the view delegate, which can handle most of the front-end logic.
+  virtual void SetDelegate(ToolbarActionViewDelegate* delegate) = 0;
+
+  // Returns the icon to use for the given |tab_id|.
+  virtual gfx::Image GetIcon(int tab_id) = 0;

[sky, 2014/10/15 23:52:30]

It's not clear at all what |tab_id| is, not why the user of this class should
need to supply it. Seems like there is a ToolbarActionViewController per action,
which implies the id should be specific to the implementation and not need to be
supplied per action. It could very well be I'm missing something.

[Devlin, 2014/10/16 16:46:39]

There is currently one ToolbarActionViewController per action per view.
BrowserActionView (poorly named, now) owns a ToolbarActionViewController, which
corresponds to either a single chrome action or a single extension action.

We need to have a tab id, because some actions have different content depending
on the tab (particularly things like enabled state, but also potentially
anything else).  Right now, it's supplied by the caller because it's slightly
less work (since the caller will frequently call multiple of these methods at
once, like when updating state, the tab id can be determined once and reused,
instead of determined in each of these methods), and because I thought it was
slightly cleaner to have the controller provide state for a given input state,
and not (inherently) need to know about things like the current browser or web
contents.  It also opens to the door to allowing us to have one controller per
action, and re-using these actions across multiple instances of the same view
(e.g., multiple windows).

That was the rationale, anyway. :)  If you would prefer to invert the logic, so
that the controller determines the tab ids as needed, I'm happy to switch it. :)

[sky, 2014/10/16 18:13:39]

I'm ok with what you have. What isn't all clear though is what 'tab_id' is. I
had to look through your patch (right now) to determine this. You need to
document that here and in other places you use it. I also tend to think passing
around the WebContents is preferable (I wouldn't have been confused then).

[Devlin, 2014/10/16 22:05:27]

Ah, I see.  I didn't realize that tab ids weren't as commonly used throughout
the codebase.  Replaced with web contents for clarity.

+
+  // Returns the accessible name to use for the given |tab_id|.
+  virtual base::string16 GetAccessibleName(int tab_id) const = 0;
+
+  // Returns the tooltip to use for the given |tab_id|.
+  virtual base::string16 GetTooltip(int tab_id) const = 0;
+
+  // Returns true if the action should be enabled on the given |tab_id|.
+  virtual bool IsEnabled(int tab_id) const = 0;
+
+  // Returns true if the action has a popup for the given |tab_id|.
+  virtual bool HasPopup(int tab_id) const = 0;
+
+  // Hides the current popup, if one is visible.
+  virtual void HidePopup() = 0;
+
+  // Returns true if a menu is currently running for the action.
+  virtual bool IsMenuRunning() const = 0;
+
+  // Executes the default action (typically showing the popup), and attributes
+  // the action to a user (thus, only use this for actions that *were* done by
+  // the user).
+  virtual void ExecuteActionByUser() = 0;
+
+  // Paints any extra parts of the image (e.g., a badge).
+  virtual void PaintExtra(gfx::Canvas* canvas,
+                          const gfx::Rect& bounds,
+                          int tab_id) const {
+  }
+
+  // Registers an accelerator. Called when the view is added to the hierarchy.
+  // Unregistering any commands is the responsibility of the controller.
+  virtual void RegisterCommand() {
+  }
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_ACTION_VIEW_CONTROLLER_H_