Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
734923003:
Fixed StyledLabel size caching (Closed)
DescriptionFixed StyledLabel size caching
StyledLabel size caching was not really working due to width check
did not take control insets into account. So any call to
GetHeightForWidth or Layout caused full layout recalculation.
When view with StyledLabel is created CalculateAndDoLayout that
does all layouting job is called for five or six times with same
arguments. And each call caused StyledLabel reset and controls removal
R=sky@chromium.org
Committed: https://crrev.com/1736bcf4e918f13c78ade1d446c7222c720d4d6a
Cr-Commit-Position: refs/heads/master@{#308770}
Committed: https://crrev.com/89945ce39a4f2efda1800ae53f4b357723586580
Cr-Commit-Position: refs/heads/master@{#309172}
Patch Set 1 #
Total comments: 5
Patch Set 2 : modified StyledLabel caching according to review comments #
Total comments: 4
Patch Set 3 : code review changes #
Total comments: 5
Patch Set 4 : code review changes #
Total comments: 11
Patch Set 5 : codereview changes #Patch Set 6 : codereview changes #
Total comments: 4
Patch Set 7 : codereview changes #Patch Set 8 : fix compilation on linux #Patch Set 9 : fix memory error #
Messages
Total messages: 35 (5 generated)
edjomin@yandex-team.ru changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... ui/views/controls/styled_label.cc:145: calculated_size_ = gfx::Size(); // force layout recalculation This comment is a bit wrong. It's not a layout calculation you need, but recreating the children, right? https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... ui/views/controls/styled_label.cc:177: return const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true).height(); I think we should continue caching here, but make Layout() do a layout if the size changes.
https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... ui/views/controls/styled_label.cc:145: calculated_size_ = gfx::Size(); // force layout recalculation On 2014/11/18 18:30:03, sky wrote: > This comment is a bit wrong. It's not a layout calculation you need, but > recreating the children, right? Yes, you are right. I reset cached size here to force next call to Layout() recreate all controls. I'll change the comment https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... ui/views/controls/styled_label.cc:177: return const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true).height(); On 2014/11/18 18:30:03, sky wrote: > I think we should continue caching here, but make Layout() do a layout if the > size changes. This was my first idea but it doesn't work this way. If I cache size here next call to Layout will not create controls because size remains the same. If I force controls recreation in Layout() caching becomes useless. I thought this solution would be better than adding additional member variables. Which solution is preferable? Let me describe situation in which I encountered this issue. I have view with GridLayout that contains StyledLabel. First of all I get one or two calls to GetHeightForWidth issued by GridLayout when it calculates row sizes. Then I get series of 4 calls to Layout from different sources. Size is not changing but each call to Layout removes all child controls and creates them again. So view appears with noticeable delay.
https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/1/ui/views/controls/styled_lab... ui/views/controls/styled_label.cc:177: return const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true).height(); On 2014/11/19 17:04:00, edjomin wrote: > On 2014/11/18 18:30:03, sky wrote: > > I think we should continue caching here, but make Layout() do a layout if the > > size changes. > > This was my first idea but it doesn't work this way. If I cache size here next > call to Layout will not create controls because size remains the same. If I > force controls recreation in Layout() caching becomes useless. I thought this > solution would be better than adding additional member variables. Which solution > is preferable? > > Let me describe situation in which I encountered this issue. > I have view with GridLayout that contains StyledLabel. First of all I get one or > two calls to GetHeightForWidth issued by GridLayout when it calculates row > sizes. Then I get series of 4 calls to Layout from different sources. Size is > not changing but each call to Layout removes all child controls and creates them > again. So view appears with noticeable delay. Layout and sizing for this class is expensive, so it's likely worth optimizing both. I get what you're saying. I'm suggesting you cache both. The last width Layout() was done at, and calculated_size_ for the last preferred size that was calculated.
Please take another look
https://codereview.chromium.org/734923003/diff/20001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/20001/ui/views/controls/styled... ui/views/controls/styled_label.cc:144: displayed_on_background_color_ = color; Early out if color == displayed_on_background_color_. https://codereview.chromium.org/734923003/diff/20001/ui/views/controls/styled... ui/views/controls/styled_label.cc:147: // when background is changed all controls should be recreated Can't we instead reset the colors on the child labels? The color doesn't effect the sizing, right?
Cahnged code according to your comments, please take a look https://codereview.chromium.org/734923003/diff/20001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/20001/ui/views/controls/styled... ui/views/controls/styled_label.cc:144: displayed_on_background_color_ = color; On 2014/12/10 17:02:54, sky wrote: > Early out if color == displayed_on_background_color_. Done. https://codereview.chromium.org/734923003/diff/20001/ui/views/controls/styled... ui/views/controls/styled_label.cc:147: // when background is changed all controls should be recreated On 2014/12/10 17:02:54, sky wrote: > Can't we instead reset the colors on the child labels? The color doesn't effect > the sizing, right? Done.
https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: static_cast<Label*>(child_at(i))-> DCHECK the class name of the child is Label::kViewClassName. https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... ui/views/controls/styled_label.cc:181: // do not store calculated_size_ for dry_run call. Update your comment, and make sure you capitalize first letters and end in periods. See other comments in Chrome for what I mean.
Please take another look https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: static_cast<Label*>(child_at(i))-> On 2014/12/11 15:48:54, sky wrote: > DCHECK the class name of the child is Label::kViewClassName. Done. https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... ui/views/controls/styled_label.cc:181: // do not store calculated_size_ for dry_run call. On 2014/12/11 15:48:53, sky wrote: > Update your comment, and make sure you capitalize first letters and end in > periods. See other comments in Chrome for what I mean. Removed this comment because it's useless now, sorry. https://codereview.chromium.org/734923003/diff/40001/ui/views/controls/styled... ui/views/controls/styled_label.cc:181: // do not store calculated_size_ for dry_run call. On 2014/12/11 15:48:53, sky wrote: > Update your comment, and make sure you capitalize first letters and end in > periods. See other comments in Chrome for what I mean. Done.
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: DCHECK(!strcmp(child_at(i)->GetClassName(), Label::kViewClassName) || Use == for comparison. https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.h:150: int layout_built_for_width_; This needs a description (and a newline). Also, the name is rather confusing. How about width_at_last_layout_? https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label_unittest.cc:434: // we should be able to calculate height without any problem Remember to capitalize sentences and end with proper punctuation.
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: DCHECK(!strcmp(child_at(i)->GetClassName(), Label::kViewClassName) || On 2014/12/12 16:01:10, sky wrote: > Use == for comparison. Can we? GetClassName() returns const char* not std::string.
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: DCHECK(!strcmp(child_at(i)->GetClassName(), Label::kViewClassName) || On 2014/12/12 16:05:07, tfarina wrote: > On 2014/12/12 16:01:10, sky wrote: > > Use == for comparison. > Can we? GetClassName() returns const char* not std::string. Yes. The expectation is GetClassName() returns a constant value that never changes. See other examples.
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: DCHECK(!strcmp(child_at(i)->GetClassName(), Label::kViewClassName) || On 2014/12/12 16:21:17, sky wrote: > On 2014/12/12 16:05:07, tfarina wrote: > > On 2014/12/12 16:01:10, sky wrote: > > > Use == for comparison. > > Can we? GetClassName() returns const char* not std::string. > > Yes. The expectation is GetClassName() returns a constant value that never > changes. See other examples. My comment was more: In C when comparing strings (pointer to chars) you use strcmp. I doubt comparing with == will yield to correct results. Isn't that case in C++?
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: DCHECK(!strcmp(child_at(i)->GetClassName(), Label::kViewClassName) || On 2014/12/12 16:50:48, tfarina wrote: > On 2014/12/12 16:21:17, sky wrote: > > On 2014/12/12 16:05:07, tfarina wrote: > > > On 2014/12/12 16:01:10, sky wrote: > > > > Use == for comparison. > > > Can we? GetClassName() returns const char* not std::string. > > > > Yes. The expectation is GetClassName() returns a constant value that never > > changes. See other examples. > My comment was more: In C when comparing strings (pointer to chars) you use > strcmp. I doubt comparing with == will yield to correct results. Isn't that case > in C++? > == wouldn't work correctly for string comparison. But since GetClassName() is expected to return a constant value, comparing the two pointers (instead of the two strings) would be sufficient.
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.cc:151: DCHECK(!strcmp(child_at(i)->GetClassName(), Label::kViewClassName) || On 2014/12/12 16:01:10, sky wrote: > Use == for comparison. Done.
https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.h:150: int layout_built_for_width_; On 2014/12/12 16:01:10, sky wrote: > This needs a description (and a newline). Also, the name is rather confusing. > How about width_at_last_layout_? You seemed to have missed this comment.
Please take another look https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.h:150: int layout_built_for_width_; On 2014/12/15 17:03:07, sky wrote: > On 2014/12/12 16:01:10, sky wrote: > > This needs a description (and a newline). Also, the name is rather confusing. > > How about width_at_last_layout_? > > You seemed to have missed this comment. Yes, I've missed it, sorry https://codereview.chromium.org/734923003/diff/60001/ui/views/controls/styled... ui/views/controls/styled_label.h:150: int layout_built_for_width_; On 2014/12/12 16:01:10, sky wrote: > This needs a description (and a newline). Also, the name is rather confusing. > How about width_at_last_layout_? Done.
https://codereview.chromium.org/734923003/diff/100001/ui/views/controls/style... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/734923003/diff/100001/ui/views/controls/style... ui/views/controls/styled_label_unittest.cc:437: EXPECT_GT(precalculated_height, 0); For testing assertions you always want expected, actual. So, use EXPECT_LT(0, precalculated_height); https://codereview.chromium.org/734923003/diff/100001/ui/views/controls/style... ui/views/controls/styled_label_unittest.cc:448: EXPECT_GT(styled()->child_count(), 0); Same comment for assertions for these three.
https://codereview.chromium.org/734923003/diff/100001/ui/views/controls/style... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/734923003/diff/100001/ui/views/controls/style... ui/views/controls/styled_label_unittest.cc:437: EXPECT_GT(precalculated_height, 0); On 2014/12/16 16:24:51, sky wrote: > For testing assertions you always want expected, actual. So, use EXPECT_LT(0, > precalculated_height); Done. https://codereview.chromium.org/734923003/diff/100001/ui/views/controls/style... ui/views/controls/styled_label_unittest.cc:448: EXPECT_GT(styled()->child_count(), 0); On 2014/12/16 16:24:51, sky wrote: > Same comment for assertions for these three. Done.
LGTM
The CQ bit was checked by edjomin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734923003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by edjomin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734923003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1736bcf4e918f13c78ade1d446c7222c720d4d6a Cr-Commit-Position: refs/heads/master@{#308770}
Commit for this issue was reverted due to uninitialized memory bug. (https://codereview.chromium.org/808853006) I've fixed it. Please, take one more look
LGTM
The CQ bit was checked by edjomin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734923003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/89945ce39a4f2efda1800ae53f4b357723586580 Cr-Commit-Position: refs/heads/master@{#309172} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
