Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581493002:
PageInfo bubble: Use the non-client view's window title and close button. (Closed)
|
Created:
4 years ago by tapted Modified:
3 years, 11 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, markusheintz_, msramek+watch_chromium.org, msw+watch_chromium.org, raymes+watch_chromium.org, rouslan+bubble_chromium.org, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPageInfo bubble: Use the non-client view's window title and close button.
Currently it sets a custom close button which is different to the other
Harmony dialogs.
To keep items aligned, use the standard panel margins from
ui/views/layout_constants.h. (13px rather than 16px). This makes the
panel slightly more narrow.
BUG=640851, 674269
Review-Url: https://codereview.chromium.org/2581493002
Cr-Commit-Position: refs/heads/master@{#442779}
Committed: https://chromium.googlesource.com/chromium/src/+/37240b4fdef1ec7017f59d4cb7ced8c655e5c595
Patch Set 1 #
Total comments: 7
Patch Set 2 : respond to comments #Patch Set 3 : selfnit: content->website #
Total comments: 2
Patch Set 4 : Use standard margins #Patch Set 5 : const #
Messages
Total messages: 33 (21 generated)
Description was changed from ========== PageInfo bubble: Use the non-client view's window title and close button BUG=674269 ========== to ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. BUG=674269 ==========
tapted@chromium.org changed reviewers: + lgarron@chromium.org
Hi Lucas, please take an initial look.. there might be something I've missed. Thanks!
Description was changed from ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. BUG=674269 ========== to ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. BUG=640851, 674269 ==========
LGTM (with nits), since this does a much-needed cleanup while consciously preserving all the relevant details I can think of. (Assuming that the right of the screenshot in https://bugs.chromium.org/p/chromium/issues/detail?id=674269#c1 is how this looks.) https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (left): https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:194: const int label_column = 0; yaaaaaaaaaaaay good riddance :-D I've added 640851 to BUG=. https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:486: const gfx::FontList& WebsiteSettingsPopupView::GetTitleFontList() const { It seems we're implementing this to make the font slightly larger. Would you mind adding a comment to document that purpose, in case this gets refactored further? https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:158: // The label that displays security summary for the current page. Nit: Remove "label" from comment. (e.g. "The security summary for the current page.") https://codereview.chromium.org/2581493002/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2581493002/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate.h:83: void set_title_margins(const gfx::Insets& title_margins) { FYI: I don't know anything about this class, so if you want thoughts on whether this is the best idea you'll have to consult someone else. :-) (Looks fine to me, though.)
The CQ bit was checked by tapted@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 checked by tapted@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...
tapted@chromium.org changed reviewers: + msw@chromium.org
+msw for OWNERS - please take a look https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:486: const gfx::FontList& WebsiteSettingsPopupView::GetTitleFontList() const { On 2016/12/14 23:46:23, lgarron wrote: > It seems we're implementing this to make the font slightly larger. Would you > mind adding a comment to document that purpose, in case this gets refactored > further? Done. Added a constant, with a comment. (we actually get a smaller font than what the default bubble title uses) https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/website_settings_popup_view.h (right): https://codereview.chromium.org/2581493002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/website_settings_popup_view.h:158: // The label that displays security summary for the current page. On 2016/12/14 23:46:23, lgarron wrote: > Nit: Remove "label" from comment. (e.g. "The security summary for the current > page.") Done. https://codereview.chromium.org/2581493002/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2581493002/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate.h:83: void set_title_margins(const gfx::Insets& title_margins) { On 2016/12/14 23:46:23, lgarron wrote: > FYI: I don't know anything about this class, so if you want thoughts on whether > this is the best idea you'll have to consult someone else. :-) > (Looks fine to me, though.) Yup - sending to msw for OWNERS :) (pretty sure this is a good approach)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2581493002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2581493002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_dialog_delegate.h:83: void set_title_margins(const gfx::Insets& title_margins) { Bummer, it seems like we need this mostly so the website settings bubble can left-align its title with the rest of its content. It'd be nice to avoid custom title margins and instead use the standard title/panel margins for the content (ie. make the content's left-margin match the title's left-margin, instead of the inverse). It's a difference of 3px on top and left, afaict: kPanelHorizMargin==13 vs kSectionPaddingHorizontal==16, and kPanelVertMargin==13 vs kHeaderPaddingTop==16. wdyt?
Otherwise, this lgtm functionally.
Description was changed from ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. BUG=640851, 674269 ========== to ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. To keep items aligned, use the standard panel margins from ui/views/layout_constants.h. (13px rather than 16px). This makes the panel slightly more narrow. BUG=640851, 674269 ==========
The CQ bit was checked by tapted@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 checked by tapted@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...
lgarron: PTAL - this changes margins slightly so that the PageInfo bubble uses the layout constants for other bubbles. I think you were more involved in the UI review for this, so might know whether this tweak will upset people. WDYT? https://codereview.chromium.org/2581493002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2581493002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_dialog_delegate.h:83: void set_title_margins(const gfx::Insets& title_margins) { On 2017/01/05 01:00:00, msw (vacation jan 4 maybe 5) wrote: > Bummer, it seems like we need this mostly so the website settings bubble can > left-align its title with the rest of its content. It'd be nice to avoid custom > title margins and instead use the standard title/panel margins for the content > (ie. make the content's left-margin match the title's left-margin, instead of > the inverse). It's a difference of 3px on top and left, afaict: > kPanelHorizMargin==13 vs kSectionPaddingHorizontal==16, and kPanelVertMargin==13 > vs kHeaderPaddingTop==16. wdyt? Done. Added a screenshot at http://crbug.com/674269#c7
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice, I hope that minor padding change meets everyone's approval, LGTM.
lgarron: ping? (see quoted bit below:) On 2017/01/05 02:20:42, tapted wrote: > lgarron: PTAL - this changes margins slightly so that the PageInfo bubble uses > the layout constants for other bubbles. I think you were more involved in the UI > review for this, so might know whether this tweak will upset people. WDYT?
On 2017/01/11 at 01:41:39, tapted wrote: > lgarron: ping? (see quoted bit below:) > > On 2017/01/05 02:20:42, tapted wrote: > > lgarron: PTAL - this changes margins slightly so that the PageInfo bubble uses > > the layout constants for other bubbles. I think you were more involved in the UI > > review for this, so might know whether this tweak will upset people. WDYT? The margins don't look like they change much, so I'm all for paying down consistency debt.
On 2017/01/11 01:54:11, lgarron wrote: > On 2017/01/11 at 01:41:39, tapted wrote: > > lgarron: ping? (see quoted bit below:) > > > > On 2017/01/05 02:20:42, tapted wrote: > > > lgarron: PTAL - this changes margins slightly so that the PageInfo bubble > uses > > > the layout constants for other bubbles. I think you were more involved in > the UI > > > review for this, so might know whether this tweak will upset people. WDYT? > > The margins don't look like they change much, so I'm all for paying down > consistency debt. alrighty, let's go for it :).
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org Link to the patchset: https://codereview.chromium.org/2581493002/#ps80001 (title: "const")
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": 80001, "attempt_start_ts": 1484099741086800,
"parent_rev": "e00ffecc157dfa92f60c583cd3bfab7f102b65ae", "commit_rev":
"37240b4fdef1ec7017f59d4cb7ced8c655e5c595"}
Message was sent while issue was closed.
Description was changed from ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. To keep items aligned, use the standard panel margins from ui/views/layout_constants.h. (13px rather than 16px). This makes the panel slightly more narrow. BUG=640851, 674269 ========== to ========== PageInfo bubble: Use the non-client view's window title and close button. Currently it sets a custom close button which is different to the other Harmony dialogs. To keep items aligned, use the standard panel margins from ui/views/layout_constants.h. (13px rather than 16px). This makes the panel slightly more narrow. BUG=640851, 674269 Review-Url: https://codereview.chromium.org/2581493002 Cr-Commit-Position: refs/heads/master@{#442779} Committed: https://chromium.googlesource.com/chromium/src/+/37240b4fdef1ec7017f59d4cb7ce... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/37240b4fdef1ec7017f59d4cb7ce... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
