Cache DrawStringParams in views::Label

Cache DrawStringParams in views::Label

BUG=443777
R=msw

Committed: https://crrev.com/74893f0bfcc2e7f4ed4245bc5a05b74e5d925549
Cr-Commit-Position: refs/heads/master@{#310795}

[ckocagil, 2015-01-07 18:51:23]

ckocagil@chromium.org changed reviewers:
+ msw@chromium.org

[ckocagil, 2015-01-07 18:52:36]

msw: Let's cache these params for M-41. We can finish land your RenderText in
Label CL for M-42.

[msw, 2015-01-07 20:35:57]

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:117: cached_draw_params_.reset();
Maybe just ResetLayoutCache(); here.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:410: const DrawStringParams* paint_params =
CalculateDrawStringParams();
nit: |draw_params| or just |params|?

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:547: DrawStringParams* paint_params = new
DrawStringParams();
Can you just do cached_draw_params_.reset(new DrawStringParams());?
Otherwise consider |draw_params| or just |params|?

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:550: elide_behavior_ == gfx::FADE_TAIL;
nit: retain relative indent.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:554: else {
nit: else on line above.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.h#ne...
ui/views/controls/label.h:190: // Resets |cached_heights_|,
|cached_heights_cursor_|, |cached_paint_params_|
Do you mean |cached_draw_params_|?

[ckocagil, 2015-01-07 22:55:08]

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:117: cached_draw_params_.reset();
On 2015/01/07 20:35:56, msw wrote:
> Maybe just ResetLayoutCache(); here.

Done.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:410: const DrawStringParams* paint_params =
CalculateDrawStringParams();
On 2015/01/07 20:35:56, msw wrote:
> nit: |draw_params| or just |params|?

Done, it's |params| now.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:547: DrawStringParams* paint_params = new
DrawStringParams();
On 2015/01/07 20:35:56, msw wrote:
> Can you just do cached_draw_params_.reset(new DrawStringParams());?
> Otherwise consider |draw_params| or just |params|?

It's |params| now. |cached_draw_params_| is longer to type, and I'd prefer it to
stay a const pointer.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:550: elide_behavior_ == gfx::FADE_TAIL;
On 2015/01/07 20:35:56, msw wrote:
> nit: retain relative indent.

Done.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.cc#n...
ui/views/controls/label.cc:554: else {
On 2015/01/07 20:35:56, msw wrote:
> nit: else on line above.

Done.

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/843503003/diff/1/ui/views/controls/label.h#ne...
ui/views/controls/label.h:190: // Resets |cached_heights_|,
|cached_heights_cursor_|, |cached_paint_params_|
On 2015/01/07 20:35:56, msw wrote:
> Do you mean |cached_draw_params_|?

Yes, done. (leftovers from a rename...)

[msw, 2015-01-07 23:34:01]

lgtm

[ckocagil, 2015-01-08 00:13:06]

ckocagil@chromium.org changed reviewers:
+ sky@chromium.org

[ckocagil, 2015-01-08 00:13:41]

+sky for OWNER'S review: PTAL

[sky, 2015-01-08 03:48:49]

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label....
ui/views/controls/label.h:157: DrawStringParams() : flags(0) {}
style guide says members after functions, even for structs.

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label....
ui/views/controls/label.h:185: const DrawStringParams*
CalculateDrawStringParams() const;
Document ownership here.

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label....
ui/views/controls/label.h:231: mutable scoped_ptr<const DrawStringParams>
cached_draw_params_;
Is there a reason to recreate this rather than make it a member? Reset could
empty the text.

[ckocagil, 2015-01-08 14:16:32]

Comments addressed and unit tests fixed. PTAL.

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label....
ui/views/controls/label.h:157: DrawStringParams() : flags(0) {}
On 2015/01/08 03:48:49, sky wrote:
> style guide says members after functions, even for structs.

Done.

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label....
ui/views/controls/label.h:185: const DrawStringParams*
CalculateDrawStringParams() const;
On 2015/01/08 03:48:49, sky wrote:
> Document ownership here.

Done.

https://codereview.chromium.org/843503003/diff/20001/ui/views/controls/label....
ui/views/controls/label.h:231: mutable scoped_ptr<const DrawStringParams>
cached_draw_params_;
On 2015/01/08 03:48:49, sky wrote:
> Is there a reason to recreate this rather than make it a member? Reset could
> empty the text.

Done, it's a member now.

[sky, 2015-01-08 22:54:57]

https://codereview.chromium.org/843503003/diff/40001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/843503003/diff/40001/ui/views/controls/label....
ui/views/controls/label.cc:294: const int flags = ComputeDrawStringFlags();
It seems like there is still a number of places calling ComputeDrawStringFlags.
Shouldn't they all you the cache now?

[ckocagil, 2015-01-09 15:05:14]

https://codereview.chromium.org/843503003/diff/40001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/843503003/diff/40001/ui/views/controls/label....
ui/views/controls/label.cc:294: const int flags = ComputeDrawStringFlags();
On 2015/01/08 22:54:57, sky wrote:
> It seems like there is still a number of places calling
ComputeDrawStringFlags.
> Shouldn't they all you the cache now?

There are 2 calls to ComputeDrawStringFlags() other than the one in
CalculateDrawStringParams():

The call in GetHeightForWidth() is needed because the flags returned by
CalculateDrawStringParams() have a different value which changes the behavior
(and the tests fail).

Simply replacing the call in GetTextSize() with ComputeDrawStringFlags() causes
an infinite recursion. Breaking it would require passing an argument, some
complex state juggling, or separating the caches of |flags| and |text|. I don't
think we want these.

[sky, 2015-01-09 17:07:09]

LGTM

https://codereview.chromium.org/843503003/diff/40001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/843503003/diff/40001/ui/views/controls/label....
ui/views/controls/label.cc:294: const int flags = ComputeDrawStringFlags();
On 2015/01/09 15:05:14, ckocagil wrote:
> On 2015/01/08 22:54:57, sky wrote:
> > It seems like there is still a number of places calling
> ComputeDrawStringFlags.
> > Shouldn't they all you the cache now?
> 
> There are 2 calls to ComputeDrawStringFlags() other than the one in
> CalculateDrawStringParams():
> 
> The call in GetHeightForWidth() is needed because the flags returned by
> CalculateDrawStringParams() have a different value which changes the behavior
> (and the tests fail).
> 
> Simply replacing the call in GetTextSize() with ComputeDrawStringFlags()
causes
> an infinite recursion. Breaking it would require passing an argument, some
> complex state juggling, or separating the caches of |flags| and |text|. I
don't
> think we want these.

Fair enough. Please add comments to these places as to why they can't use the
cache then.

[ckocagil, 2015-01-09 17:19:05]

On 2015/01/09 17:07:09, sky wrote:
> Fair enough. Please add comments to these places as to why they can't use the
> cache then.

Done, landing

[ckocagil, 2015-01-09 17:20:05]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2015-01-09 17:20:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/843503003/60001

[commit-bot: I haz the power, 2015-01-09 18:06:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-09 18:07:10]

Patchset 4 (id:??) landed as
https://crrev.com/74893f0bfcc2e7f4ed4245bc5a05b74e5d925549
Cr-Commit-Position: refs/heads/master@{#310795}