Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616573002:
Adjust spacing in InternalPageInfoPopupView (Closed)
|
Created:
3 years, 11 months ago by Evan Stade Modified:
3 years, 11 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust spacing in InternalPageInfoPopupView
(the page info bubble for chrome:// pages)
BUG=585076
Review-Url: https://codereview.chromium.org/2616573002
Cr-Commit-Position: refs/heads/master@{#442055}
Committed: https://chromium.googlesource.com/chromium/src/+/ab578c9b8495ab800c8909ab4a3af8b99ca6ca6a
Patch Set 1 #
Total comments: 4
Patch Set 2 : constexpr #Messages
Total messages: 14 (9 generated)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
estade@chromium.org changed reviewers: + pkasting@chromium.org
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.
LGTM https://codereview.chromium.org/2616573002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2616573002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:177: const int kSpacing = 12; Nit: constexpr https://codereview.chromium.org/2616573002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:386: kSpacing + 8 + frame->bubble_border()->GetBorderThickness()); Is it possible to compute this without hardcoding "8"? For example, keep a pointer to the icon view around and use it to get the width/2. This would also let you get the x coordinate from the icon so you wouldn't have to ensure the spacing amount/calculation stays in sync here, which would mean you wouldn't have to hoist the spacing constant. (You could do this without an icon pointer by just assuming "first child is the icon", but that seems fragile.) This would obviate the need for the comment above.
https://codereview.chromium.org/2616573002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2616573002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:177: const int kSpacing = 12; On 2017/01/06 02:27:11, Peter Kasting (backlogged) wrote: > Nit: constexpr Done. https://codereview.chromium.org/2616573002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:386: kSpacing + 8 + frame->bubble_border()->GetBorderThickness()); On 2017/01/06 02:27:11, Peter Kasting (backlogged) wrote: > Is it possible to compute this without hardcoding "8"? For example, keep a > pointer to the icon view around and use it to get the width/2. This would also > let you get the x coordinate from the icon so you wouldn't have to ensure the > spacing amount/calculation stays in sync here, which would mean you wouldn't > have to hoist the spacing constant. > > (You could do this without an icon pointer by just assuming "first child is the > icon", but that seems fragile.) > > This would obviate the need for the comment above. I don't think the contents will have been laid out by this point, so the spacing constant would still need to be here. We could still get rid of the hardcoded 8 but it's not at zero cost, and it doesn't seem to me super likely that will ever save us from a bug.
The CQ bit was checked by estade@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/2616573002/#ps20001 (title: "constexpr")
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": 20001, "attempt_start_ts": 1483734026798520,
"parent_rev": "e0615e1b33112564698760629bef4b56d80572cd", "commit_rev":
"ab578c9b8495ab800c8909ab4a3af8b99ca6ca6a"}
Message was sent while issue was closed.
Description was changed from ========== Adjust spacing in InternalPageInfoPopupView (the page info bubble for chrome:// pages) BUG=585076 ========== to ========== Adjust spacing in InternalPageInfoPopupView (the page info bubble for chrome:// pages) BUG=585076 Review-Url: https://codereview.chromium.org/2616573002 Cr-Commit-Position: refs/heads/master@{#442055} Committed: https://chromium.googlesource.com/chromium/src/+/ab578c9b8495ab800c8909ab4a3a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ab578c9b8495ab800c8909ab4a3a... |
