Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2722503005:
Add bottom border to StyledLabel. (Closed)
DescriptionAdd bottom border to StyledLabel.
On CL https://codereview.chromium.org/2072513003 it was changed calculation of height of StyledLabel, so the |total_height| is equal to the bottom point of lowest sublabel. This doesn't take into consideration a bottom border. Screenshots are on the bug. This CL fixes this.
BUG=697034
Review-Url: https://codereview.chromium.org/2722503005
Cr-Commit-Position: refs/heads/master@{#455495}
Committed: https://chromium.googlesource.com/chromium/src/+/2581d91a5a2adfdae460bd25a43b44cac6f80ded
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : Rebase, test added #
Total comments: 6
Patch Set 4 : Addressed comments #Patch Set 5 : Tiny fix #
Total comments: 4
Patch Set 6 : Addressed reviewer comments #
Total comments: 2
Patch Set 7 : Spacing fixed #Messages
Total messages: 27 (16 generated)
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add bottom border to StyledLabel. BUG=697034 ========== to ========== Add bottom border to StyledLabel. On CL https://codereview.chromium.org/2072513003 it was changed calculation of height of StyledLabel, so the |total_height| is equal to the bottom point of lowest sublabel. And now it doesn't take into consideration bottom border. Screenshots are on the bug. This CL fixes this. BUG=697034 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add bottom border to StyledLabel. On CL https://codereview.chromium.org/2072513003 it was changed calculation of height of StyledLabel, so the |total_height| is equal to the bottom point of lowest sublabel. And now it doesn't take into consideration bottom border. Screenshots are on the bug. This CL fixes this. BUG=697034 ========== to ========== Add bottom border to StyledLabel. On CL https://codereview.chromium.org/2072513003 it was changed calculation of height of StyledLabel, so the |total_height| is equal to the bottom point of lowest sublabel. This doesn't take into consideration a bottom border. Screenshots are on the bug. This CL fixes this. BUG=697034 ==========
dvadym@chromium.org changed reviewers: + msw@chromium.org
msw@, could you please review? Please advice if you think it's not the best solution. Regards, Vadym
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msw@chromium.org changed reviewers: + estade@chromium.org
Adding Evan, he should review this first.
ping
https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:356: focus_border_insets += -label->View::GetInsets(); nit: why is this += - and not -= https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:360: GetInsets().left() + x - focus_border_insets.left(), GetInsets() seems non-trivial, can we pull it into a local variable instead of calling it repeatedly? https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:365: total_height = std::max(total_height, label->bounds().bottom()); I think it's a little bit more clear if we add it here instead since it's near related logic. Does this work: total_height = std::max(total_height, label->bounds().bottom() + GetInsets().bottom() - focus_border_insets.bottom())
Thanks for comments Evan! I've addressed them. PTAL https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:356: focus_border_insets += -label->View::GetInsets(); On 2017/03/06 14:28:30, Evan Stade wrote: > nit: why is this += - and not -= Done. https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:360: GetInsets().left() + x - focus_border_insets.left(), On 2017/03/06 14:28:31, Evan Stade wrote: > GetInsets() seems non-trivial, can we pull it into a local variable instead of > calling it repeatedly? Done. https://codereview.chromium.org/2722503005/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:365: total_height = std::max(total_height, label->bounds().bottom()); On 2017/03/06 14:28:31, Evan Stade wrote: > I think it's a little bit more clear if we add it here instead since it's near > related logic. Does this work: > > total_height = std::max(total_height, label->bounds().bottom() + > GetInsets().bottom() - focus_border_insets.bottom()) Done.
lgtm https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... ui/views/controls/styled_label.cc:358: gfx::Insets insets = GetInsets(); nit: const https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... ui/views/controls/styled_label_unittest.cc:571: EXPECT_EQ(label_preferred_size.height() + 5 /* bottom border */, nit: can you also set other sides of the border and test those are taken into account?
Thanks for review Evan! https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... ui/views/controls/styled_label.cc:358: gfx::Insets insets = GetInsets(); On 2017/03/06 22:01:29, Evan Stade wrote: > nit: const Done. https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/2722503005/diff/80001/ui/views/controls/style... ui/views/controls/styled_label_unittest.cc:571: EXPECT_EQ(label_preferred_size.height() + 5 /* bottom border */, On 2017/03/06 22:01:29, Evan Stade wrote: > nit: can you also set other sides of the border and test those are taken into > account? Done.
lgtm with an optional nit https://codereview.chromium.org/2722503005/diff/100001/ui/views/controls/styl... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/2722503005/diff/100001/ui/views/controls/styl... ui/views/controls/styled_label_unittest.cc:571: 6 /* bottom */, 20 /*right*/)); optional nit: use consistent spacing inside these comments here and below.
https://codereview.chromium.org/2722503005/diff/100001/ui/views/controls/styl... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/2722503005/diff/100001/ui/views/controls/styl... ui/views/controls/styled_label_unittest.cc:571: 6 /* bottom */, 20 /*right*/)); On 2017/03/08 16:34:25, msw wrote: > optional nit: use consistent spacing inside these comments here and below. Done.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2722503005/#ps120001 (title: "Spacing fixed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488992914058240,
"parent_rev": "8fe541f3c9154b760838d2fe4b470c58c1a9fe7b", "commit_rev":
"2581d91a5a2adfdae460bd25a43b44cac6f80ded"}
Message was sent while issue was closed.
Description was changed from ========== Add bottom border to StyledLabel. On CL https://codereview.chromium.org/2072513003 it was changed calculation of height of StyledLabel, so the |total_height| is equal to the bottom point of lowest sublabel. This doesn't take into consideration a bottom border. Screenshots are on the bug. This CL fixes this. BUG=697034 ========== to ========== Add bottom border to StyledLabel. On CL https://codereview.chromium.org/2072513003 it was changed calculation of height of StyledLabel, so the |total_height| is equal to the bottom point of lowest sublabel. This doesn't take into consideration a bottom border. Screenshots are on the bug. This CL fixes this. BUG=697034 Review-Url: https://codereview.chromium.org/2722503005 Cr-Commit-Position: refs/heads/master@{#455495} Committed: https://chromium.googlesource.com/chromium/src/+/2581d91a5a2adfdae460bd25a43b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2581d91a5a2adfdae460bd25a43b... |
