LabelButton: cache the last computed preferred size

LabelButton: cache the last computed preferred size

LabelButton's GetPreferredSize() gets called a lot, and it's pretty
expensive (creates a temporary label every time). To make it less sucky, we
should cache its last computed size and reuse that, just like Label does.

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

[noms (inactive), 2014-07-04 14:30:50]

Hiya Mike & Scott,

Here's a CL that makes LabelButton's GetPreferredSize() faster and hopefully
does not break everything :)

Please take a look !

[msw, 2014-07-08 00:24:04]

I think we're trying to avoid caching preferred sizes, because it adds an
error-prone layer of complexity (ie. see my comments here, consider subclass
behavior, and consider future LabelButton/subclass changes that might miss this
pattern). That said, the need to create a Label instance on the stack for sizing
in LabelButton::GetPreferredSize is horrendous for performance, and might make
this sort of change worthwhile.

Perhaps we could avoid that Label instance if we had another method to get the
size of a Bolded label, or instead use a heuristic (like +10%) to pad the
current label width...

I'd like to hear thoughts from Scott, Evan, or Elliot.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:83: ResetCachedSize();
I think this call might be unnecessary if we have one in
ChildPreferredSizeChanged. See my comments in StateChanged and UpdateImage.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:149:
label_->SetHorizontalAlignment(alignment);
I don't think the alignment should generally change the preferred size, but it
might be safe to clear it here.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:154:
label_->set_directionality_mode(mode);
The directionality could possibly change the preferred size (multi-line BiDi
text may need to break into more lines), so clear it here.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:171: void
LabelButton::SetStyle(ButtonStyle style) {
Setting the style will definitely change the preferred size, (generally by
changing the border, thus yielding new insets), maybe clearing the size on
SetBorder would be sufficient, but I'm not sure offhand.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:195: if (!button_size_valid_) {
nit: reverse condition and early return instead.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:298: void
LabelButton::SetBorder(scoped_ptr<Border> border) {
SetBorder may change the insets and thus the preferred size.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:383: void LabelButton::UpdateImage() {
Updating the image may change its preferred size, and should trigger
ChildPreferredSizeChanged here, which should clear the preferred size, as I also
describe in my StateChanged comment.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:387: void
LabelButton::UpdateThemedBorder() {
Looks like clearing on SetBorder would be sufficient for this.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:410: UpdateImage();
Updating the image may change the preferred size (if there's a difference in the
image preferred sizes). We explicitly handle that case with a Layout call below,
but ImageView::SetImage calls its own PreferredSizeChanged, which should call
ChildPreferredSizeChanged here, and that should probably perform or trigger
[Invalidate]Layout instead... This isn't entirely in the scope of this CL, but
you'll at least want to clear the preferred size in ChildPreferredSizeChanged.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:419: void
LabelButton::ChildPreferredSizeChanged(View* child) {
This should definitely clear the cached preferred size.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:468: button_size_valid_ = false;
nit: avoid the flag and use empty as an invalid size; then also DCHECK that the
actual preferred size is non-empty in GetPreferredSize.

[Evan Stade, 2014-07-08 18:23:09]

For the record, I agree with all of msw's review nits. I also agree caching size
calculations is error prone.

regarding:
"the need to create a Label instance on the stack for sizing
in LabelButton::GetPreferredSize is horrendous for performance"

Has this been measured? Why is Label creation so slow? Is it somehow worse on
the stack than the heap? You could always just keep a spare Label handy if
creating one is really a perf bottleneck. I'd be less surprised to learn that
text measurement itself is what's slow.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:107: ResetCachedSize();
do shadows really affect the size of the button? that would surprise me

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:410: UpdateImage();
On 2014/07/08 00:24:03, msw wrote:
> Updating the image may change the preferred size (if there's a difference in
the
> image preferred sizes). We explicitly handle that case with a Layout call
below,
> but ImageView::SetImage calls its own PreferredSizeChanged, which should call
> ChildPreferredSizeChanged here, and that should probably perform or trigger
> [Invalidate]Layout instead... This isn't entirely in the scope of this CL, but
> you'll at least want to clear the preferred size in ChildPreferredSizeChanged.

Having gone down this rabbit hole int the past, I've found that when you
automatically call Layout in places like ChildPreferredSizeChanged you can
easily wind up doing an excessive number of layouts. This is why Views relies on
explicit, strategically placed Layout calls.

[sky, 2014-07-09 20:34:23]

I agree with caching being tricky too. You don't mention why you are doing
this. How much is caching the size saving us?

  -Scott


On Fri, Jul 4, 2014 at 7:30 AM, <noms@chromium.org> wrote:

> Reviewers: msw, sky (OOO),
>
> Message:
> Hiya Mike & Scott,
>
> Here's a CL that makes LabelButton's GetPreferredSize() faster and
> hopefully
> does not break everything :)
>
> Please take a look !
>
> Description:
> LabelButton: cache the last computed preferred size
>
> LabelButton's GetPreferredSize() gets called a lot, and it's pretty
> expensive (creates a temporary label every time). To make it less sucky, we
> should cache its last computed size and reuse that, just like Label does.
>
> BUG=373906
>
> Please review this at https://codereview.chromium.org/371633002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+63, -40 lines):
>   M ui/views/controls/button/label_button.h
>   M ui/views/controls/button/label_button.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[noms (inactive), 2014-07-09 20:52:09]

This CL fixes the Win 8 slowness in crbug.com/373906. The underlying problem is
probably deeper in
the win rendering code (the calls to ScriptShape are occasionally _much_
slower), but since I don't
know how to fix that, making GetPreferredSize() faster is the next best thing.

The reason I went with the caching of the size is because Label already did the
same thing, and by
always new-ing a new Label on the stack, we're not even taking advantage of
that.


On 2014/07/09 20:34:23, sky wrote:
> I agree with caching being tricky too. You don't mention why you are doing
> this. How much is caching the size saving us?
> 
>   -Scott
> 
> 
> On Fri, Jul 4, 2014 at 7:30 AM, <mailto:noms@chromium.org> wrote:
> 
> > Reviewers: msw, sky (OOO),
> >
> > Message:
> > Hiya Mike & Scott,
> >
> > Here's a CL that makes LabelButton's GetPreferredSize() faster and
> > hopefully
> > does not break everything :)
> >
> > Please take a look !
> >
> > Description:
> > LabelButton: cache the last computed preferred size
> >
> > LabelButton's GetPreferredSize() gets called a lot, and it's pretty
> > expensive (creates a temporary label every time). To make it less sucky, we
> > should cache its last computed size and reuse that, just like Label does.
> >
> > BUG=373906
> >
> > Please review this at https://codereview.chromium.org/371633002/
> >
> > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+63, -40 lines):
> >   M ui/views/controls/button/label_button.h
> >   M ui/views/controls/button/label_button.cc
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2014-07-09 21:33:54]

On 2014/07/09 20:52:09, Monica Dinculescu wrote:
> This CL fixes the Win 8 slowness in crbug.com/373906. The underlying problem
is
> probably deeper in
> the win rendering code (the calls to ScriptShape are occasionally _much_
> slower), but since I don't
> know how to fix that, making GetPreferredSize() faster is the next best thing.
> 
> The reason I went with the caching of the size is because Label already did
the
> same thing, and by
> always new-ing a new Label on the stack, we're not even taking advantage of
> that.

then can we just reuse the same label every time?

> 
> 
> On 2014/07/09 20:34:23, sky wrote:
> > I agree with caching being tricky too. You don't mention why you are doing
> > this. How much is caching the size saving us?
> > 
> >   -Scott
> > 
> > 
> > On Fri, Jul 4, 2014 at 7:30 AM, <mailto:noms@chromium.org> wrote:
> > 
> > > Reviewers: msw, sky (OOO),
> > >
> > > Message:
> > > Hiya Mike & Scott,
> > >
> > > Here's a CL that makes LabelButton's GetPreferredSize() faster and
> > > hopefully
> > > does not break everything :)
> > >
> > > Please take a look !
> > >
> > > Description:
> > > LabelButton: cache the last computed preferred size
> > >
> > > LabelButton's GetPreferredSize() gets called a lot, and it's pretty
> > > expensive (creates a temporary label every time). To make it less sucky,
we
> > > should cache its last computed size and reuse that, just like Label does.
> > >
> > > BUG=373906
> > >
> > > Please review this at https://codereview.chromium.org/371633002/
> > >
> > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
> > >
> > > Affected files (+63, -40 lines):
> > >   M ui/views/controls/button/label_button.h
> > >   M ui/views/controls/button/label_button.cc
> > >
> > >
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[noms (inactive), 2014-07-14 19:55:48]

Hmm, I could try reusing the same label, and do something smart where we don't
always reset the cached-label's text in LabelButton's GetPreferredSize() (so
that we can take advantage of Label's caching)

Does that sound like the way to go?

[noms (inactive), 2014-07-14 20:01:20]

Actually, I take that back. If we want to use the same Label and take advantage
of its caching fast-ness, then we still need to appropriately reset it in random
LabelButton places, so I'm not sure that's any different than the
ResetCachedSize() approach :(


On 2014/07/14 19:55:48, Monica Dinculescu wrote:
> Hmm, I could try reusing the same label, and do something smart where we don't
> always reset the cached-label's text in LabelButton's GetPreferredSize() (so
> that we can take advantage of Label's caching)
> 
> Does that sound like the way to go?

[Evan Stade, 2014-07-14 21:16:08]

On 2014/07/14 20:01:20, Monica Dinculescu wrote:
> Actually, I take that back. If we want to use the same Label and take
advantage
> of its caching fast-ness, then we still need to appropriately reset it in
random
> LabelButton places,

Such as? All the places that currently have a ResetCachedSize call also have a
call into label_ (except SetImage, which shouldn't affect the label's size). I
would expect all those calls into label_ to automatically reset Label's internal
size cache.

> so I'm not sure that's any different than the
> ResetCachedSize() approach :(
> 
> 
> On 2014/07/14 19:55:48, Monica Dinculescu wrote:
> > Hmm, I could try reusing the same label, and do something smart where we
don't
> > always reset the cached-label's text in LabelButton's GetPreferredSize() (so
> > that we can take advantage of Label's caching)
> > 
> > Does that sound like the way to go?

[noms (inactive), 2014-07-15 14:02:25]

Even if we don't use the temporary stack label in LabelButton's
GetPreferredSize(), and just use the label_ member, because we still set its
FontList on each call, this resets Label's cached size, and we're not taking
advantage of that. This means the code would be exactly like the status quo, but
without constructing the Label on the stack (and constructing the label isn't
the slow part, the label recomputing everything all the time is)

I will wait until Mike's Label fixes patch lands today and see if that is fast
enough, but otherwise I still don't see how we can get around the Win 8
performance issues without computing this size so often :(

On 2014/07/14 21:16:08, Evan Stade wrote:
> On 2014/07/14 20:01:20, Monica Dinculescu wrote:
> > Actually, I take that back. If we want to use the same Label and take
> advantage
> > of its caching fast-ness, then we still need to appropriately reset it in
> random
> > LabelButton places,
> 
> Such as? All the places that currently have a ResetCachedSize call also have a
> call into label_ (except SetImage, which shouldn't affect the label's size). I
> would expect all those calls into label_ to automatically reset Label's
internal
> size cache.
> 
> > so I'm not sure that's any different than the
> > ResetCachedSize() approach :(
> > 
> > 
> > On 2014/07/14 19:55:48, Monica Dinculescu wrote:
> > > Hmm, I could try reusing the same label, and do something smart where we
> don't
> > > always reset the cached-label's text in LabelButton's GetPreferredSize()
(so
> > > that we can take advantage of Label's caching)
> > > 
> > > Does that sound like the way to go?

[Evan Stade, 2014-07-15 19:10:07]

On 2014/07/15 14:02:25, Monica Dinculescu wrote:
> Even if we don't use the temporary stack label in LabelButton's
> GetPreferredSize(), and just use the label_ member, because we still set its
> FontList on each call

I doubt we actually /need/ to call SetFontList each time.

https://codereview.chromium.org/371633002/diff/60001/ui/views/controls/button...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/60001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:210: // Resize multi-line labels given
the current limited available width.
by the way, I don't think the code should be resizing the label in
GetPreferredSize() --- that should be part of Layout. Also, calls to width()
here are suspect because the preferred size shouldn't depend on the current
size.

[msw, 2014-07-15 22:51:43]

Unfortunately, my WIP Label rewrite <https://codereview.chromium.org/23228004>
alone may not have the dramatic performance improvement that I hoped it would
(but hopefully it'll take us in the right direction and address Evan's latest
comment here).

https://codereview.chromium.org/371633002/diff/60001/ui/views/controls/button...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/60001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:210: // Resize multi-line labels given
the current limited available width.
On 2014/07/15 19:10:06, Evan Stade wrote:
> by the way, I don't think the code should be resizing the label in
> GetPreferredSize() --- that should be part of Layout. Also, calls to width()
> here are suspect because the preferred size shouldn't depend on the current
> size.

This is addressed in my WIP https://codereview.chromium.org/23228004/

[noms (inactive), 2014-07-21 15:40:44]

I've addressed the missing/extraneous ResetCachedSize() calls from the first
patch.

The tl; dr of the latest discussion is:
- getting rid of the stack Label won't fix much, because GetPreferredSize()
calls SetFontList, which resets the Label's cached size
- (msw can explain this better but,) the calls to SetFontList() are needed so
that the LabelButton doesn't change size when it becomes default, and the text
bolded.

If the general consensus is that we definitely do not want to cache the size in
LabelButton, and since I don't have any other ideas to fix the performance
issues, I can subclass LabelButton, cache the size in the subclass, and then use
it exclusively in our bubble. That would fix the performance issues, and leave
LabelButton alone.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:83: ResetCachedSize();
On 2014/07/08 00:24:03, msw wrote:
> I think this call might be unnecessary if we have one in
> ChildPreferredSizeChanged. See my comments in StateChanged and UpdateImage.

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:107: ResetCachedSize();
On 2014/07/08 18:23:08, Evan Stade wrote:
> do shadows really affect the size of the button? that would surprise me

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:149:
label_->SetHorizontalAlignment(alignment);
On 2014/07/08 00:24:03, msw wrote:
> I don't think the alignment should generally change the preferred size, but it
> might be safe to clear it here.

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:154:
label_->set_directionality_mode(mode);
On 2014/07/08 00:24:04, msw wrote:
> The directionality could possibly change the preferred size (multi-line BiDi
> text may need to break into more lines), so clear it here.

This went away after msw's CL.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:171: void
LabelButton::SetStyle(ButtonStyle style) {
On 2014/07/08 00:24:03, msw wrote:
> Setting the style will definitely change the preferred size, (generally by
> changing the border, thus yielding new insets), maybe clearing the size on
> SetBorder would be sufficient, but I'm not sure offhand.

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:195: if (!button_size_valid_) {
On 2014/07/08 00:24:03, msw wrote:
> nit: reverse condition and early return instead.

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:298: void
LabelButton::SetBorder(scoped_ptr<Border> border) {
On 2014/07/08 00:24:03, msw wrote:
> SetBorder may change the insets and thus the preferred size.

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:383: void LabelButton::UpdateImage() {
On 2014/07/08 00:24:03, msw wrote:
> Updating the image may change its preferred size, and should trigger
> ChildPreferredSizeChanged here, which should clear the preferred size, as I
also
> describe in my StateChanged comment.

Acknowledged.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:387: void
LabelButton::UpdateThemedBorder() {
On 2014/07/08 00:24:03, msw wrote:
> Looks like clearing on SetBorder would be sufficient for this.

Acknowledged.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:419: void
LabelButton::ChildPreferredSizeChanged(View* child) {
On 2014/07/08 00:24:04, msw wrote:
> This should definitely clear the cached preferred size.

Done.

https://codereview.chromium.org/371633002/diff/40001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:468: button_size_valid_ = false;
Isn't it possible that a LabelButton would legitimately have a 0 size, though?
In which case we shouldn't recompute it if we don't have to? (I may be
overthinking this)

On 2014/07/08 00:24:04, msw wrote:
> nit: avoid the flag and use empty as an invalid size; then also DCHECK that
the
> actual preferred size is non-empty in GetPreferredSize.

https://codereview.chromium.org/371633002/diff/60001/ui/views/controls/button...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/60001/ui/views/controls/button...
ui/views/controls/button/label_button.cc:210: // Resize multi-line labels given
the current limited available width.
On 2014/07/15 22:51:43, msw wrote:
> On 2014/07/15 19:10:06, Evan Stade wrote:
> > by the way, I don't think the code should be resizing the label in
> > GetPreferredSize() --- that should be part of Layout. Also, calls to width()
> > here are suspect because the preferred size shouldn't depend on the current
> > size.
> 
> This is addressed in my WIP https://codereview.chromium.org/23228004/

Done.

[msw, 2014-07-21 16:28:26]

I'm actually okay with caching the size.

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:181: ResetCachedSize();
Call this after OnNativeThemeChanged.

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:390: void LabelButton::UpdateImage() {
Contrary to my earlier comment, ResetCachedSize() here to be safe.

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:69: void set_min_size(const gfx::Size&
min_size) {
Rename these (SetMinSize, SetMaxSize) and define them in the code file.

[noms (inactive), 2014-07-21 17:18:51]

I'm working on getting the bots to run. They're not keen on applying the patch
:)

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:181: ResetCachedSize();
On 2014/07/21 16:28:26, msw wrote:
> Call this after OnNativeThemeChanged.

Done.

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:390: void LabelButton::UpdateImage() {
On 2014/07/21 16:28:26, msw wrote:
> Contrary to my earlier comment, ResetCachedSize() here to be safe.

Done.

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/371633002/diff/100001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:69: void set_min_size(const gfx::Size&
min_size) {
On 2014/07/21 16:28:26, msw wrote:
> Rename these (SetMinSize, SetMaxSize) and define them in the code file.

Done.

[noms (inactive), 2014-07-23 18:57:31]

Ping! :)

[sky, 2014-07-23 19:19:35]

I assume you're pinging msw on this and not the rest of us.

On Wed, Jul 23, 2014 at 11:57 AM,  <noms@chromium.org> wrote:
> Ping! :)
>
> https://codereview.chromium.org/371633002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2014-07-23 19:20:16]

lgtm with a nit and some thoughts for followup.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:92: ResetCachedSize();
Ideally, changing child Label view's text should trigger
ChildPreferredSizeChanged here, making this call unnecessary. It'd be great to
have unit tests for such behavior, and to weed out unnecessary explicit
ResetCachedSize calls. I won't block the CL on that condition, but I would
greatly appreciate your help with that sort of followup work.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:245: int
LabelButton::GetHeightForWidth(int w) const {
I wonder if we should similarly cache the preferred heights of a limited number
of argument widths, I bet this is used heavily by some layout managers. You
don't need to bother with that in this CL, but followup would be appreciated.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:486: void
LabelButton::ResetCachedSize() {
I wonder if this should call PreferredSizeChanged(), but you shouldn't add that
in this CL; it wasn't done before and can be investigated later if needed.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:70: void SetMaxSize(const gfx::Size&
min_size);
nit: |max_size|

[noms (inactive), 2014-07-23 19:24:14]

Scott: technically, I was pinging you too. :)
Now that Mike has reviewed/stamped this (with a nit, uploading a patch soon),
would you mind doing an owners review, please? Thanks!

[sky, 2014-07-23 22:42:37]

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:148: void ResetCachedSize();
Is it possible to override ChildPreferredSizeChanged to mark the cached state as
out of date? That way you don't need to explicitly reset this state as it's
implicitly communicated from the label.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:171: mutable gfx::Size button_size_;
Can you name this better. It's the preferred size. Maybe cached_preferred_size_.

[noms (inactive), 2014-07-24 14:20:26]

I've added a test and addressed the other comments. Please take a look.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:92: ResetCachedSize();
I removed it from here and from all the other places in which Label triggers an
update.

On 2014/07/23 19:20:16, msw wrote:
> Ideally, changing child Label view's text should trigger
> ChildPreferredSizeChanged here, making this call unnecessary. It'd be great to
> have unit tests for such behavior, and to weed out unnecessary explicit
> ResetCachedSize calls. I won't block the CL on that condition, but I would
> greatly appreciate your help with that sort of followup work.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:245: int
LabelButton::GetHeightForWidth(int w) const {
Oh, that's a neat idea. I could help with that :)

On 2014/07/23 19:20:16, msw wrote:
> I wonder if we should similarly cache the preferred heights of a limited
number
> of argument widths, I bet this is used heavily by some layout managers. You
> don't need to bother with that in this CL, but followup would be appreciated.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:486: void
LabelButton::ResetCachedSize() {
On 2014/07/23 19:20:16, msw wrote:
> I wonder if this should call PreferredSizeChanged(), but you shouldn't add
that
> in this CL; it wasn't done before and can be investigated later if needed.

Acknowledged.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:70: void SetMaxSize(const gfx::Size&
min_size);
On 2014/07/23 19:20:16, msw wrote:
> nit: |max_size|

Done.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:148: void ResetCachedSize();
Hmm, but shouldn't actions on the LabelButton also invalidate the cached size?
For example changing the image or the border? It would definitely decrease the
calls to ResetCachedSize(), though, so I've done that.

On 2014/07/23 22:42:36, sky wrote:
> Is it possible to override ChildPreferredSizeChanged to mark the cached state
as
> out of date? That way you don't need to explicitly reset this state as it's
> implicitly communicated from the label.

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:171: mutable gfx::Size button_size_;
On 2014/07/23 22:42:36, sky wrote:
> Can you name this better. It's the preferred size. Maybe
cached_preferred_size_.

Done.

[sky, 2014-07-24 15:38:05]

LGTM

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/371633002/diff/180001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:148: void ResetCachedSize();
On 2014/07/24 14:20:26, Monica Dinculescu wrote:
> Hmm, but shouldn't actions on the LabelButton also invalidate the cached size?
> For example changing the image or the border? It would definitely decrease the
> calls to ResetCachedSize(), though, so I've done that.
> 
> On 2014/07/23 22:42:36, sky wrote:
> > Is it possible to override ChildPreferredSizeChanged to mark the cached
state
> as
> > out of date? That way you don't need to explicitly reset this state as it's
> > implicitly communicated from the label.
> 

Yes, sorry, you may need a mix of the two. But it seems ideal to leverage
ChildPreferredSizeChanged as much as possible.

[msw, 2014-07-24 17:56:38]

LGTM! More tests would be nice, but not strictly necessary.

[noms (inactive), 2014-07-24 18:04:42]

Mike: I'll talk to you offline about the other tests, and do them in a separate
CL, if needed.

YAY! Thanks everyone! Our 3 Win8 users will be delighted to see this change land
:)

[noms (inactive), 2014-07-24 18:04:48]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-07-24 18:08:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/371633002/220001

[commit-bot: I haz the power, 2014-07-24 18:53:40]

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

[commit-bot: I haz the power, 2014-07-24 19:28:21]

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

[commit-bot: I haz the power, 2014-07-24 19:28:22]

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

[noms (inactive), 2014-07-24 19:36:11]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-07-24 19:38:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/371633002/220001

[commit-bot: I haz the power, 2014-07-24 20:16:06]

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

[commit-bot: I haz the power, 2014-07-24 20:50:08]

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

[commit-bot: I haz the power, 2014-07-24 20:50:10]

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

[noms (inactive), 2014-07-25 13:22:25]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-07-25 13:23:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/371633002/220001

[commit-bot: I haz the power, 2014-07-25 14:53:03]

Change committed as 285587