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

chrome/browser/ui/views/toolbar/toolbar_view.cc

Index: chrome/browser/ui/views/toolbar/toolbar_view.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.cc b/chrome/browser/ui/views/toolbar/toolbar_view.cc
index a0f5021c9d8c88c28ec9546c20d7299784dbd346..2263be873795c7faa5e0f8866b971c54a6d397b1 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_view.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_view.cc
@@ -163,18 +163,16 @@ ToolbarView::~ToolbarView() {
 }
 
 void ToolbarView::Init() {
-  back_ = new ButtonDropDown(this, new BackForwardMenuModel(
+  back_ = new ToolbarButton(this, new BackForwardMenuModel(
       browser_, BackForwardMenuModel::BACKWARD_MENU));
   back_->set_triggerable_event_flags(
       ui::EF_LEFT_MOUSE_BUTTON | ui::EF_MIDDLE_MOUSE_BUTTON);
   back_->set_tag(IDC_BACK);
-  back_->SetImageAlignment(views::ImageButton::ALIGN_RIGHT,
-                           views::ImageButton::ALIGN_TOP);

[Peter Kasting, 2013/11/19 02:28:50]

This alignment statement was necessary in maximized mode, where we extend the
back button width to the screen edge.  Make sure your change does not break the
button positioning in this case.

[Greg Billock, 2013/11/20 00:59:03]

I'll check on that.

OK, it looks like what's happening is we're extending to the border, but the
back button is showing up slightly wider than the other buttons. I think that's
an error -- we always want the button to cap to the current image width.

[Peter Kasting, 2013/11/20 01:10:20]

Yeah, we'll need some way of drawing the border narrower than the button
actually is.  Ugh, I hope that's not too hard.

[Greg Billock, 2013/11/20 01:30:46]

We ought to be able to sense that and just add a touch of maximization padding
at the beginning of the toolbar in toolbar_view, right? Or is the whole point to
make the button go to the edge for greater fitt's law glory?

[Peter Kasting, 2013/11/20 01:42:20]

Yes, we're intentionally extending the button hit region beyond the graphical
border of the button to exploit Fitts' Law.

[Greg Billock, 2013/11/20 23:13:23]

OK, the latest patch addresses this. I ended up needing very minimal changes to
the LabelButton class, which I believe are semantically compatible with the
existing API.

   back_->SetTooltipText(l10n_util::GetStringUTF16(IDS_TOOLTIP_BACK));
   back_->SetAccessibleName(l10n_util::GetStringUTF16(IDS_ACCNAME_BACK));
   back_->set_id(VIEW_ID_BACK_BUTTON);
 
-  forward_ = new ButtonDropDown(this, new BackForwardMenuModel(
+  forward_ = new ToolbarButton(this, new BackForwardMenuModel(
       browser_, BackForwardMenuModel::FORWARD_MENU));
   forward_->set_triggerable_event_flags(
       ui::EF_LEFT_MOUSE_BUTTON | ui::EF_MIDDLE_MOUSE_BUTTON);
@@ -703,31 +701,19 @@ void ToolbarView::LoadImages() {
   ui::ThemeProvider* tp = GetThemeProvider();
 
   back_->SetImage(views::CustomButton::STATE_NORMAL,

[Peter Kasting, 2013/11/19 02:28:50]

Nit: Qualify with Button:: instead of CustomButton:: (5 places)

[Greg Billock, 2013/11/20 00:59:03]

Done.

-      tp->GetImageSkiaNamed(IDR_BACK));
-  back_->SetImage(views::CustomButton::STATE_HOVERED,
-      tp->GetImageSkiaNamed(IDR_BACK_H));
-  back_->SetImage(views::CustomButton::STATE_PRESSED,
-      tp->GetImageSkiaNamed(IDR_BACK_P));
+      *(tp->GetImageSkiaNamed(IDR_BACK)));

[Peter Kasting, 2013/11/19 02:28:50]

Nit: All lines of args should be aligned (5 places)

Did you intend to provide new, borderless center images for back/forward/home in
this CL?

[Greg Billock, 2013/11/20 00:59:03]

Done.
No, I was just going to use the existing ones. I do think we should change them
to borderless, though, but we can do it in-place and delete the unneeded ones.

[Peter Kasting, 2013/11/20 01:10:20]

OK, I was mostly worried that we'd get two borders on buttons or something if we
drew a border + a center image that also had a border.  I guess they'll overlay.
 That might make slight changes for alpha-blending if we alpha-blend to support
different toolbar colors, but that probably doesn't matter if this is
transitionary.

That does remind me though: you should check your changes with some themes that
set very different toolbar colors.  I bet you have to add these new border
images to the set of images that have to be tinted for theme changes.  Search
the codebase for other references to these image IDs.

[Greg Billock, 2013/11/20 01:30:46]

This change stops using all the images with borders, so there's no danger there
-- we are definitely drawing transparencies on top of each other. That's OK for
now, but I think having smaller center images is easier to reason about, as well
as saving a few bytes.

WRT themes: These images are pretty much just transparency masks, so they ought
to look awsumm with any background color, or even a gradient or many patterns.
I'll try a few and make sure it isn't nuts, but I'm hopeful. :-)

The themes specify a solid for the background there, so I'm pretty sure we're
OK, except that for very dark colors I'm not sure how it'll work out. Need to
check, but I'm not sure our current system reacts well to that, since they're
transparency masks as well.

[Greg Billock, 2013/11/20 23:13:23]

Still need to deal with this. (Chrome store crashing v8 at ToT making it hard to
check.)

   back_->SetImage(views::CustomButton::STATE_DISABLED,
-      tp->GetImageSkiaNamed(IDR_BACK_D));
+      *(tp->GetImageSkiaNamed(IDR_BACK_D)));
 
   forward_->SetImage(views::CustomButton::STATE_NORMAL,
-      tp->GetImageSkiaNamed(IDR_FORWARD));
-  forward_->SetImage(views::CustomButton::STATE_HOVERED,
-      tp->GetImageSkiaNamed(IDR_FORWARD_H));
-  forward_->SetImage(views::CustomButton::STATE_PRESSED,
-      tp->GetImageSkiaNamed(IDR_FORWARD_P));
+      *(tp->GetImageSkiaNamed(IDR_FORWARD)));
   forward_->SetImage(views::CustomButton::STATE_DISABLED,
-      tp->GetImageSkiaNamed(IDR_FORWARD_D));
+      *(tp->GetImageSkiaNamed(IDR_FORWARD_D)));
 
   reload_->LoadImages();
 
   home_->SetImage(views::CustomButton::STATE_NORMAL,
-      tp->GetImageSkiaNamed(IDR_HOME));
-  home_->SetImage(views::CustomButton::STATE_HOVERED,
-      tp->GetImageSkiaNamed(IDR_HOME_H));
-  home_->SetImage(views::CustomButton::STATE_PRESSED,
-      tp->GetImageSkiaNamed(IDR_HOME_P));
+      *(tp->GetImageSkiaNamed(IDR_HOME)));
 }
 
 void ToolbarView::ShowCriticalNotification() {