[Toolbar] Base toolbar button class with background images for button states

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

+// Copyright (c) 2013 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_SITE_CHIP_VIEW_H_
+#define CHROME_BROWSER_UI_VIEWS_TOOLBAR_SITE_CHIP_VIEW_H_
+
+#include "base/metrics/field_trial.h"
+#include "chrome/browser/ui/toolbar/toolbar_model.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_button.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/label_button.h"
+#include "ui/views/view.h"
+
+class LocationIconView;
+class ToolbarView;
+
+namespace content {
+class WebContents;
+}
+
+namespace views {
+class Label;
+}
+
+class SiteChipView : public ToolbarButton {

[Peter Kasting, 2013/11/15 03:28:58]

Is the site chip going to want a dropdown?  If not, should we attempt to
separate that functionality from the base toolbar button class, or make the site
chip not inherit from ToolbarButton?

(Originally I had only envisioned reusing the graphics to start with, and not
the code from the other toolbar buttons; I'm not opposed to code reuse if it
makes sense, but I want to make sure we really want the site chip to use all
that code.)

[Greg Billock, 2013/11/15 19:36:50]

The problem is the diamond inheritance -- we have the ButtonDropDown stuff, and
the (much simpler) border stuff. I think there's a good case for using
~ButtonDropDown as a superclass -- I wouldn't be surprised to see the site chip
end up growing a menu if we keep it as a toolbar button. But in the meantime,
it's probably a good plan to just aim for a toolbar button superclass aside from
site chip needs, as the hope is it makes the buttons more uniform as well as
making the experiment easy.

[Peter Kasting, 2013/11/15 22:11:56]

Could we make the toolbar button base class include painting the border and
supporting clicks, and then add dropdown support via composition with a class
that adds the menu and the like?

Or perhaps have a simple inheritance hierarchy like:

               ToolbarButton (provides border and some basic behaviors)
                 /      \
ToolbarDropdownButton  SiteChip
         |
[All other toolbar buttons]

I definitely don't want diamond inheritance.  It may be that you're right and
including the dropdown functionality in the base class -- and then simply not
enabling it in the site chip -- is the best route.  I leave it to your judgment.

[Greg Billock, 2013/11/19 00:07:59]

The border stuff is pretty simple, so I think that'd be easier mix-in. My
intuition is being guided by a kind of "what if this had already been done" --
if there was a ToolbarButton, it'd be pretty similar to what this CL has, I
think, since all existing buttons have menus. So in that world, we'd make the
site chip view based on ToolbarButton with perhaps a small chnage to make it so
the superclass wouldn't show the menu.

I still think it is <50% that we end up with a toolbar button for the site chip.
(I think it much more likely we end up with something in the location bar.) So
I'm inclined to not lean too heavily on the site chip for the class design.

+ public:
+  explicit SiteChipView(ToolbarView* toolbar_view);
+  virtual ~SiteChipView();
+
+  void Init();
+
+  void Update(content::WebContents* tab);
+
+  void OnChanged();
+
+  views::ImageView* GetLocationIconView();
+  const views::ImageView* GetLocationIconView() const;
+
+  // views::ButtonListener
+  void ButtonPressed(views::Button* sender, const ui::Event& event) OVERRIDE;
+
+  // views::View
+  virtual gfx::Size GetPreferredSize() OVERRIDE;
+  virtual void Layout() OVERRIDE;
+
+ private:
+  ToolbarView* toolbar_view_;
+  views::Label* host_label_;
+  LocationIconView* location_icon_view_;
+
+  DISALLOW_COPY_AND_ASSIGN(SiteChipView);
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_SITE_CHIP_VIEW_H_