views: Move MenuButton from TextButton to LabelButton.

views: Move MenuButton from TextButton to LabelButton.

This also converts the TextButtons in the BookmarkBarView to LabelButtons, since they need to have the same base class as MenuButton does. This also standardizes on fade eliding instead of fade eliding sometimes and ellipsis eliding in other cases.

This patch has the effect that everything on the bookmark bar also renders with GTK+ borders in GTK theme mode on Linux.

BUG=155363
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277878

[Elliot Glaysher, 2014-06-07 00:06:36]

https://codereview.chromium.org/298813002/diff/100001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (left):

https://codereview.chromium.org/298813002/diff/100001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:160: // This logic should be kept in
sync with the LabelButtonBorder constructor.
In retrospect, me thinking this would ever happen was hopelessly naive.

https://codereview.chromium.org/298813002/diff/100001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.cc (right):

https://codereview.chromium.org/298813002/diff/100001/ui/views/button_drag_ut...
ui/views/button_drag_utils.cc:21: static const int kLinkDragImageMaxWidth = 150;
This was made to match BookmarkBarView::kMaxButtonWidth.

[sky, 2014-06-09 15:42:00]

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1113: const gfx::Point&
press_pt,
I think you're missing the offset (press_pt) used here.

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:204: //
SetShowMultipleIconStates(delegate_->NeedToShowMultipleIconStates());
?

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:240:
SetTooltipText(name);
Why the change here?

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc:14:
preferred_size_(ResourceBundle::GetSharedInstance().
I say nuke the member and look up every time you need it.

[msw, 2014-06-09 17:18:40]

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:510: // This allows
the button to shrink if the new text is smaller.
You can "Call set_min_size(gfx::Size()) to clear the monotonically increasing
size." to allow the LabelButton to shrink too (otherwise it won't shrink for
smaller text).

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1410:
button->ClearMaxTextSize();
ditto: call set_min_size(gfx::Size())

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/translate_infobar_base.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/translate_infobar_base.cc:51:
button->ClearMaxTextSize();
ditt: call set_min_size(gfx::Size())

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:183: ClearMaxTextSize();
ditto: call set_min_size(gfx::Size())

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:799:
chevron_->SetHoverIcon(*tp->GetImageSkiaNamed(
nit: why are the hover and pressed state images no longer set?

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:60: // A version of
MenuButton with almost empty insets to fit properly on the
q: does this look okay when the border is drawn (gtk and non-gtk style?)

https://codereview.chromium.org/298813002/diff/110001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.cc (right):

https://codereview.chromium.org/298813002/diff/110001/ui/views/button_drag_ut...
ui/views/button_drag_utils.cc:21: static const int kLinkDragImageMaxWidth = 150;
Why are you changing this?

[Elliot Glaysher, 2014-06-11 00:27:28]

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:510: // This allows
the button to shrink if the new text is smaller.
On 2014/06/09 17:18:39, msw wrote:
> You can "Call set_min_size(gfx::Size()) to clear the monotonically increasing
> size." to allow the LabelButton to shrink too (otherwise it won't shrink for
> smaller text).

Used set_min_size() at all places where we used to have a ClearMaxTextSize().

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1113: const gfx::Point&
press_pt,
On 2014/06/09 15:42:00, sky wrote:
> I think you're missing the offset (press_pt) used here.

Added optional offset parameter to SetURLAndDragImage and threaded press_pt
through.

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:204: //
SetShowMultipleIconStates(delegate_->NeedToShowMultipleIconStates());
On 2014/06/09 15:42:00, sky wrote:
> ?

Stray mark.

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:240:
SetTooltipText(name);
On 2014/06/09 15:42:00, sky wrote:
> Why the change here?

The delegate was always the default delegate. There may have been another
implementation sometime in the past, but this always returns true. Same at the
top of the method.

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (left):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:799:
chevron_->SetHoverIcon(*tp->GetImageSkiaNamed(
On 2014/06/09 17:18:40, msw wrote:
> nit: why are the hover and pressed state images no longer set?

Because I deleted the "chevron_->SetBorder(views::Border::NullBorder());" line
above.

The chevron now has the nice fade-in that you get for free when using
LabelButtonBorder instead of just flipping images. It also themes itself
correctly when in GTK mode.

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:60: // A version of
MenuButton with almost empty insets to fit properly on the
On 2014/06/09 17:18:40, msw wrote:
> q: does this look okay when the border is drawn (gtk and non-gtk style?)

Yes. (It doesn't draw correctly without clearing the insets.)

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc (right):

https://codereview.chromium.org/298813002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc:14:
preferred_size_(ResourceBundle::GetSharedInstance().
On 2014/06/09 15:42:00, sky wrote:
> I say nuke the member and look up every time you need it.

Done.

https://codereview.chromium.org/298813002/diff/110001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.cc (right):

https://codereview.chromium.org/298813002/diff/110001/ui/views/button_drag_ut...
ui/views/button_drag_utils.cc:21: static const int kLinkDragImageMaxWidth = 150;
On 2014/06/09 17:18:40, msw wrote:
> Why are you changing this?

As mentioned above, this is being set to BookmarkBarView::kMaxButtonWidth.
Otherwise, we create a drag image larger than the button we're dragging from.

[msw, 2014-06-11 00:55:46]

lgtm with nits; thanks!

https://codereview.chromium.org/298813002/diff/150001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/298813002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:167:
SetElideBehavior(gfx::FADE_TAIL);
This changed the appearance of elided bookmark buttons/folders. Windows always
used ellipsis eliding, and Linux would do that for RTL text. I support this
change, but it's worth mentioning in the CL description (and posting pics?).

https://codereview.chromium.org/298813002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1788: text_color);
nit: fits on line above.

https://codereview.chromium.org/298813002/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1791: text_color);
nit: |color| would make this call and the one below each fit on one line too.

https://codereview.chromium.org/298813002/diff/150001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.cc (right):

https://codereview.chromium.org/298813002/diff/150001/ui/views/button_drag_ut...
ui/views/button_drag_utils.cc:66: press_point, data);
nit: fits on line above.

[sky, 2014-06-11 15:46:09]

LGTM

[Elliot Glaysher, 2014-06-11 18:13:10]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-06-11 18:16:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/298813002/170001

[commit-bot: I haz the power, 2014-06-11 20:52:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 20:56:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:56:44]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Elliot Glaysher, 2014-06-12 18:07:22]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-06-12 18:10:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/298813002/170001

[commit-bot: I haz the power, 2014-06-12 21:14:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-12 21:21:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-12 21:21:57]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[Elliot Glaysher, 2014-06-13 18:21:23]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-06-13 18:23:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/298813002/190001

[commit-bot: I haz the power, 2014-06-13 22:29:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-14 00:57:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-14 00:57:16]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Elliot Glaysher, 2014-06-16 19:56:47]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-06-16 19:57:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/298813002/210001

[commit-bot: I haz the power, 2014-06-17 04:41:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 04:41:42]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Elliot Glaysher, 2014-06-17 17:29:58]

I am disabling
TabDragging/DetachToBrowserTabDragControllerTest.DragDirectlyToSecondWindow/0.
It is already disabled on all other platforms due to flake, and I can't repro
the failure locally.

[Elliot Glaysher, 2014-06-17 17:42:27]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:44:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/298813002/230001

[commit-bot: I haz the power, 2014-06-17 22:11:29]

Change committed as 277878

[jackhou1, 2014-06-18 07:21:48]

A revert of this CL has been created in
https://codereview.chromium.org/340003002/ by jackhou@chromium.org.

The reason for reverting is: This is causes ExtensionApiTest.Bookmarks to fail
on Linux:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29....

[jackhou1, 2014-06-18 07:26:46]

Sorry to revert such a big CL. I did test reverting it in my local repo from
ToT, and it fixed the test.