Issue 82483003: Add GetMinimumSize() for Borders, and make LabelButton auto-size to at least as

Issue 82483003: Add GetMinimumSize() for Borders, and make LabelButton auto-size to at least as (Closed)

Created:
7 years, 1 month ago by Peter Kasting

Modified:
7 years, 1 month ago

Reviewers:
msw, sky

CC:
chromium-reviews, msw+watch_chromium.org, sadrul, tfarina, kalyank, alicet1, ben+ash_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Add GetMinimumSize() for Borders, and make LabelButton auto-size to at least as large as its border requires. BUG=311269 TEST=none R=msw@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236912

Patch Set 3 : #

Created: 7 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+127 lines, -20 lines)			Patch
M	ash/system/tray/tray_details_view.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/avatar_label.cc	View		2 chunks	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/toolbar/wrench_menu.cc	View	1 2	2 chunks	+8 lines, -0 lines	0 comments	Download
M	ui/views/border.h	View		1 chunk	+9 lines, -1 line	0 comments	Download
M	ui/views/border.cc	View		4 chunks	+13 lines, -0 lines	0 comments	Download
M	ui/views/bubble/bubble_border.h	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ui/views/bubble/bubble_border.cc	View		2 chunks	+28 lines, -19 lines	0 comments	Download
M	ui/views/controls/button/label_button.cc	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	ui/views/controls/button/label_button_border.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/views/controls/button/label_button_border.cc	View		1 chunk	+11 lines, -0 lines	0 comments	Download
M	ui/views/controls/button/text_button.h	View		3 chunks	+4 lines, -0 lines	0 comments	Download
M	ui/views/controls/button/text_button.cc	View		2 chunks	+15 lines, -0 lines	0 comments	Download
M	ui/views/controls/focusable_border.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/views/controls/focusable_border.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	ui/views/shadow_border.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/views/shadow_border.cc	View	1 2	2 chunks	+6 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

msw

https://codereview.chromium.org/82483003/diff/220001/ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc File ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc (right): https://codereview.chromium.org/82483003/diff/220001/ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc#newcode259 ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc:259: gfx::Size MaximizeBubbleBorder::GetMinimumSize() const { Why is the minimum size ...

7 years, 1 month ago (2013-11-22 18:00:24 UTC) #2

Peter Kasting

7 years, 1 month ago (2013-11-22 22:07:55 UTC) #3

https://codereview.chromium.org/82483003/diff/220001/ash/wm/caption_buttons/m...
File ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc (right):

https://codereview.chromium.org/82483003/diff/220001/ash/wm/caption_buttons/m...
ash/wm/caption_buttons/maximize_bubble_controller_bubble.cc:259: gfx::Size
MaximizeBubbleBorder::GetMinimumSize() const {
On 2013/11/22 18:00:25, msw wrote:
> Why is the minimum size needed at all in cases like this?

I don't understand the question.  Are you saying, this patch doesn't use a
MaximizeBubbleBorder's minimum size, so don't write code for it?  I don't think
we should be providing methods that can be called on any border but are only
accurate for some borders.  That's a land mine waiting to happen.

https://codereview.chromium.org/82483003/diff/220001/chrome/browser/ui/views/...
File chrome/browser/ui/views/avatar_label.cc (right):

https://codereview.chromium.org/82483003/diff/220001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.cc:87: gfx::Size size(4, 4);
On 2013/11/22 18:00:25, msw wrote:
> nit: should 4 be declared as a file-local constant for this and Paint?

We'd declare 2 rather than 4 if we were going to declare anything; I don't mind
such a declaration in principle but I don't understand the code well enough to
give such a constant a good, accurate name.

https://codereview.chromium.org/82483003/diff/220001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/82483003/diff/220001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:146: // CENTER_BUTTON.
On 2013/11/22 18:00:25, msw wrote:
> nit: why is CENTER_BUTTON critical? What about the other types?

I actually think I meant SINGLE_BUTTON... I made the comment more generic.

> Also, it's not
> really sufficient for the entire button, but the border; it might help to
> clarify that in such a comment.

The way minimum sizes are defined on all classes (as I try to make clear in the
base class header comments) is to provide enough size for that class to draw. 
In particular, for borders, this means "enough space for the border to draw",
NOT "enough space for the button which has this border as a border to draw". 
The latter would be appropriate for a GetMinimumSize() on the button.

The views that use these borders are the classes that ultimately understand what
content, if any, is contained inside the view and how it's laid out relative to
the border, and thus know whether insets, as well as any other content, need to
be accounted for in GetMinimumSize().

https://codereview.chromium.org/82483003/diff/220001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:216: // Ugh, why don't all the
following use SkPaths with rounded corners?
On 2013/11/22 18:00:25, msw wrote:
> nit: fix it, file an issue, or make this a TODO.

Done.

https://codereview.chromium.org/82483003/diff/220001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/82483003/diff/220001/ui/views/border.cc#newco...
ui/views/border.cc:87: return gfx::Size();
On 2013/11/22 18:00:25, msw wrote:
> nit: shouldn't this match the insets? Empty means transparent, not unsized.

As noted previously, GetMinimumSize() should only be big enough for the border
to draw without error.  This does no drawing and thus needs no size.

https://codereview.chromium.org/82483003/diff/220001/ui/views/border.h
File ui/views/border.h (right):

https://codereview.chromium.org/82483003/diff/220001/ui/views/border.h#newcode74
ui/views/border.h:74: // Returns the minimum size this border requires.  Note
that this may not be
On 2013/11/22 18:00:25, msw wrote:
> Would it make sense to have the default Border::GetMinimumSize implementation
> just return the size of the insets? That'd be sufficient for
> ash/system/tray/tray_details_view.cc and in probably most other cases.

That's not the intent of GetMinimumSize() from Border's perspective.  That would
be appropriate for GetMinimumSize() on the view that has this border as a
border.  See other similar comments.

https://codereview.chromium.org/82483003/diff/220001/ui/views/controls/focusa...
File ui/views/controls/focusable_border.cc (right):

https://codereview.chromium.org/82483003/diff/220001/ui/views/controls/focusa...
ui/views/controls/focusable_border.cc:64: return gfx::Size();
On 2013/11/22 18:00:25, msw wrote:
> Shouldn't this minimum size match the insets or the 2px stroke width?

In the case where the local bounds are an empty rect, Paint() draws no border at
all (as opposed to trying to calculate rects with negative widths or something),
which means the border can draw "correctly" even at zero size.

https://codereview.chromium.org/82483003/diff/220001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/82483003/diff/220001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:37: // This isn't really right if one of the offsets
is larger than (blur_ / 2)...
On 2013/11/22 18:00:25, msw wrote:
> nit: fix it, file an issue, or make this a TODO.

Done.

Message was sent while issue was closed.

Committed patchset #3 manually as r236912.

Expand Messages | Collapse Messages