Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936533004:
Provide vertical padding to the RenderTexts used in suggestions. (Closed)
|
Created:
3 years, 6 months ago by Justin Donnelly Modified:
3 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, jdonnelly+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionProvide vertical padding to the RenderTexts used in suggestions.
Where possible, RenderText uses this additional space to vertically
center the cap height of the font instead of centering the entire font.
All vertical padding was taken away in http://crrev.com/467038 which
caused a vertical alignment regression. This CL restores some padding in
a way that still works with the goal of the earlier change (to have
different spacing between multi-line suggestions than between
suggestions).
BUG=723984
Review-Url: https://codereview.chromium.org/2936533004
Cr-Commit-Position: refs/heads/master@{#480047}
Committed: https://chromium.googlesource.com/chromium/src/+/c7b5ef1e2bd9deb076af700de566211e4cfa7628
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixes for answer image positioning and row height in the presence of an answer. #
Total comments: 2
Patch Set 3 : Respond to comment. #Messages
Total messages: 24 (17 generated)
The CQ bit was checked by jdonnelly@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jdonnelly@chromium.org changed reviewers: + pkasting@chromium.org
LGTM https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:54: static const int kVerticalMargin = 1; So why a margin of 1 and padding of 4 instead of a margin of 0 and padding of 6? https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:59: static const int kVerticalPadding = 4; Nit: kAdditionalTextLineHeight? Should this just be set as part of the line height for the RenderText used to draw this, so it doesn't have to be added in manually?
The CQ bit was checked by jdonnelly@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...
Thanks for the lgtm but I discovered and fixed a couple minor issues. Can you take another look when you get a chance? https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:54: static const int kVerticalMargin = 1; On 2017/06/13 23:42:50, Peter Kasting wrote: > So why a margin of 1 and padding of 4 instead of a margin of 0 and padding of 6? Because that won't work in the multi-part case (answers and/or vertical layout). It would introduce more padding between the two parts than we want. (That's what caused this bug in the first place, was me changing the layout so that we could have a different amount of space between the two parts than we have between suggestions.) https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:59: static const int kVerticalPadding = 4; On 2017/06/13 23:42:50, Peter Kasting wrote: > Nit: kAdditionalTextLineHeight? > > Should this just be set as part of the line height for the RenderText used to > draw this, so it doesn't have to be added in manually? That would add additional, undesired space between lines in multi-line answers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:54: static const int kVerticalMargin = 1; On 2017/06/15 18:56:53, Justin Donnelly wrote: > On 2017/06/13 23:42:50, Peter Kasting wrote: > > So why a margin of 1 and padding of 4 instead of a margin of 0 and padding of > 6? > > Because that won't work in the multi-part case (answers and/or vertical layout). > It would introduce more padding between the two parts than we want. (That's what > caused this bug in the first place, was me changing the layout so that we could > have a different amount of space between the two parts than we have between > suggestions.) OK, but using 1 and 4 is sufficient for your needs? https://codereview.chromium.org/2936533004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2936533004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:376: int answer_icon_size = GetAnswerFont().GetHeight(); Nit: Is it worth commenting here why using GetAnswerHeight() is wrong?
The CQ bit was checked by jdonnelly@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...
https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2936533004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:54: static const int kVerticalMargin = 1; On 2017/06/15 22:43:34, Peter Kasting wrote: > On 2017/06/15 18:56:53, Justin Donnelly wrote: > > On 2017/06/13 23:42:50, Peter Kasting wrote: > > > So why a margin of 1 and padding of 4 instead of a margin of 0 and padding > of > > 6? > > > > Because that won't work in the multi-part case (answers and/or vertical > layout). > > It would introduce more padding between the two parts than we want. (That's > what > > caused this bug in the first place, was me changing the layout so that we > could > > have a different amount of space between the two parts than we have between > > suggestions.) > > OK, but using 1 and 4 is sufficient for your needs? Yes. I've tested this extensively on Linux and Windows. (And I've discovered a couple other Windows issues as a result. I guess I really need to be testing on Windows more.) https://codereview.chromium.org/2936533004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2936533004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:376: int answer_icon_size = GetAnswerFont().GetHeight(); On 2017/06/15 22:43:34, Peter Kasting wrote: > Nit: Is it worth commenting here why using GetAnswerHeight() is wrong? Done.
The CQ bit was unchecked by jdonnelly@chromium.org
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2936533004/#ps40001 (title: "Respond to comment.")
The CQ bit was checked by jdonnelly@chromium.org
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": 40001, "attempt_start_ts": 1497622498563630,
"parent_rev": "b623375e9b4f289c9c102e5dce72be5a5b1396f6", "commit_rev":
"c7b5ef1e2bd9deb076af700de566211e4cfa7628"}
Message was sent while issue was closed.
Description was changed from ========== Provide vertical padding to the RenderTexts used in suggestions. Where possible, RenderText uses this additional space to vertically center the cap height of the font instead of centering the entire font. All vertical padding was taken away in http://crrev.com/467038 which caused a vertical alignment regression. This CL restores some padding in a way that still works with the goal of the earlier change (to have different spacing between multi-line suggestions than between suggestions). BUG=723984 ========== to ========== Provide vertical padding to the RenderTexts used in suggestions. Where possible, RenderText uses this additional space to vertically center the cap height of the font instead of centering the entire font. All vertical padding was taken away in http://crrev.com/467038 which caused a vertical alignment regression. This CL restores some padding in a way that still works with the goal of the earlier change (to have different spacing between multi-line suggestions than between suggestions). BUG=723984 Review-Url: https://codereview.chromium.org/2936533004 Cr-Commit-Position: refs/heads/master@{#480047} Committed: https://chromium.googlesource.com/chromium/src/+/c7b5ef1e2bd9deb076af700de566... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c7b5ef1e2bd9deb076af700de566... |
