Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753243002:
Views/Harmony: Replace layout constants in chrome/browser/ui/views/extensions. (Closed)
DescriptionViews/Harmony: Replace layout constants in chrome/browser/ui/views/extensions.
Replace all direct references to constants in ui/views/layout/layout_constants.h
under chrome/browser/ui/views/extensions/* with LayoutDelegate metrics.
BUG=691897
Review-Url: https://codereview.chromium.org/2753243002
Cr-Commit-Position: refs/heads/master@{#459707}
Committed: https://chromium.googlesource.com/chromium/src/+/8a8f67e659f819a459a6076e8fcc634e755dabf6
Patch Set 1 #Patch Set 2 : Clean up includes. #
Total comments: 23
Patch Set 3 : Review comments. #
Total comments: 6
Patch Set 4 : More review comments. #
Total comments: 12
Patch Set 5 : Review comments. #Patch Set 6 : Rebase. #
Total comments: 6
Patch Set 7 : Review comments. #Messages
Total messages: 51 (38 generated)
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: linux_chromium_asan_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 patricialor@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.
The CQ bit was checked by patricialor@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.
Patchset #2 (id:20001) has been deleted
patricialor@chromium.org changed reviewers: + tapted@chromium.org
PTAL? Thank you!
https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc:81: button_margin, button_margin); The children here are not buttons :/, but I suppose in both pre- and pos- Harmony the goal is to align with the button row of the dialog client view (it should probably be PANEL_CONTENT_MARGIN :/..) There's a mock at http://crbug.com/652510 but it's out of date :/ But the third argument should always have been kUnrelatedControlLargeHorizontalSpacing Anyway I think the right things to use here are: DIALOG_BUTTON_MARGIN, PANEL_CONTENT_MARGIN, UNRELATED_CONTROL_HORIZONTAL_SPACING With a comment // Align the contents with the dialog buttons. Using PANEL_CONTENT_MARGIN for the vertical margin will squish the dialog a bit under non-Harmony, which will look fine. But using it for the horizontal margin will misalign things (I think). Ideally we would use PANEL_CONTENT_MARGIN for vertical and horizontal. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:250: const int related_control_vert_spacing = layout_delegate->GetMetric( maybe just `vertical_padding` ? (same in AddPermissions()) https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:280: layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN); Similar thing here... this should maybe be PANEL_CONTENT_MARGIN under Harmony, but I think that will break things pre-Harmony. This is fine. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:423: const int button_margin = I think sensible names are button_margin->horizontal_margin content_margin->bottom_margin. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:441: column_set->AddPaddingColumn(0, content_margin); PANEL_CONTENT_MARGIN probably doesn't belong in the middle of a row :/ Should this be kUnrelatedControlHorizontalSpacing ? (that's 12px instead of 13px) Does this padding column line up to something, or is it arbitrary? https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:612: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING)); HORIZONTAL doesn't look right here https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:678: (parent_bulleted ? 2 : 1)); This should be pulled out (with its comment) to a helper function - in an anonymous namespace https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:10: #include "chrome/browser/ui/views/harmony/layout_delegate.h" is this needed here? (maybe move to the .cc?) https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:115: const int related_control_vert_spacing = layout_delegate->GetMetric( vertical_padding?
The CQ bit was checked by patricialor@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...
PTAL? Thanks! https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc:81: button_margin, button_margin); On 2017/03/20 03:41:52, tapted wrote: > The children here are not buttons :/, but I suppose in both pre- and pos- > Harmony the goal is to align with the button row of the dialog client view (it > should probably be PANEL_CONTENT_MARGIN :/..) > > There's a mock at http://crbug.com/652510 but it's out of date :/ > > But the third argument should always have been > kUnrelatedControlLargeHorizontalSpacing > > > Anyway I think the right things to use here are: > > DIALOG_BUTTON_MARGIN, PANEL_CONTENT_MARGIN, UNRELATED_CONTROL_HORIZONTAL_SPACING > > With a comment > > // Align the contents with the dialog buttons. > > > Using PANEL_CONTENT_MARGIN for the vertical margin will squish the dialog a bit > under non-Harmony, which will look fine. But using it for the horizontal margin > will misalign things (I think). Ideally we would use PANEL_CONTENT_MARGIN for > vertical and horizontal. Done. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:250: const int related_control_vert_spacing = layout_delegate->GetMetric( On 2017/03/20 03:41:52, tapted wrote: > maybe just `vertical_padding` ? (same in AddPermissions()) Done. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:280: layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN); On 2017/03/20 03:41:52, tapted wrote: > Similar thing here... this should maybe be PANEL_CONTENT_MARGIN under Harmony, > but I think that will break things pre-Harmony. This is fine. Should we be keeping track of these somehow? e.g. a TODO + a SecondaryMDUI check? (so that we just delete the check and the else when we are done?) https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:423: const int button_margin = On 2017/03/20 03:41:52, tapted wrote: > I think sensible names are button_margin->horizontal_margin > content_margin->bottom_margin. Done. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:441: column_set->AddPaddingColumn(0, content_margin); On 2017/03/20 03:41:52, tapted wrote: > PANEL_CONTENT_MARGIN probably doesn't belong in the middle of a row :/ > > Should this be kUnrelatedControlHorizontalSpacing ? (that's 12px instead of > 13px) > > Does this padding column line up to something, or is it arbitrary? I think it's arbitrary - this is the only place in this dialog that has more than one column, so I think it's just spacing for the icon. I've changed this to kUnrelatedControlHorizontalSpacing like you said, it still looks fine :) Thanks! I was confused by the code around it as well, so I updated the comment above - hope that's OK. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:612: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING)); On 2017/03/20 03:41:52, tapted wrote: > HORIZONTAL doesn't look right here Oops, fixed! https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:678: (parent_bulleted ? 2 : 1)); On 2017/03/20 03:41:52, tapted wrote: > This should be pulled out (with its comment) to a helper function - in an > anonymous namespace Done. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:10: #include "chrome/browser/ui/views/harmony/layout_delegate.h" On 2017/03/20 03:41:52, tapted wrote: > is this needed here? (maybe move to the .cc?) Thanks, not sure how that got through :O https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:115: const int related_control_vert_spacing = layout_delegate->GetMetric( On 2017/03/20 03:41:52, tapted wrote: > vertical_padding? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with extension_install_dialog_view.cc updated https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:280: layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN); On 2017/03/21 06:24:30, Patti Lor wrote: > On 2017/03/20 03:41:52, tapted wrote: > > Similar thing here... this should maybe be PANEL_CONTENT_MARGIN under Harmony, > > but I think that will break things pre-Harmony. This is fine. > > Should we be keeping track of these somehow? e.g. a TODO + a SecondaryMDUI > check? (so that we just delete the check and the else when we are done?) Yeah.. I think we can eventually just remove DIALOG_BUTTON_MARGIN, since it should always align with PANEL_CONTENT_MARGIN. (Then no need to track, since the compiler will get them all) https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:441: column_set->AddPaddingColumn(0, content_margin); On 2017/03/21 06:24:29, Patti Lor wrote: > On 2017/03/20 03:41:52, tapted wrote: > > PANEL_CONTENT_MARGIN probably doesn't belong in the middle of a row :/ > > > > Should this be kUnrelatedControlHorizontalSpacing ? (that's 12px instead of > > 13px) > > > > Does this padding column line up to something, or is it arbitrary? > > I think it's arbitrary - this is the only place in this dialog that has more > than one column, so I think it's just spacing for the icon. I've changed this to > kUnrelatedControlHorizontalSpacing like you said, it still looks fine :) Thanks! > > I was confused by the code around it as well, so I updated the comment above - > hope that's OK. looks great! https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:612: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING)); On 2017/03/21 06:24:30, Patti Lor wrote: > On 2017/03/20 03:41:52, tapted wrote: > > HORIZONTAL doesn't look right here > > Oops, fixed! ping? https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:86: int GetLeftPaddingForBulletedItems(const bool parent_bulleted) { nit: no need for `const` https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:607: int padding = GetLeftPaddingForBulletedItems(parent_bulleted); nit: inline below (`padding` temporary no required) https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:622: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING)); For this, I suggest constexpr int kPaddingAboveDetailsLink = 4; There is RELATED_CONTROL_VERTICAL_SPACING. But - that is 8px rather than 4px. - this isn't a control -- it's just text
The CQ bit was checked by patricialor@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...
patricialor@chromium.org changed reviewers: + pkasting@chromium.org
Hi pkasting, PTAL. https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:280: layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN); On 2017/03/22 03:05:59, tapted wrote: > On 2017/03/21 06:24:30, Patti Lor wrote: > > On 2017/03/20 03:41:52, tapted wrote: > > > Similar thing here... this should maybe be PANEL_CONTENT_MARGIN under > Harmony, > > > but I think that will break things pre-Harmony. This is fine. > > > > Should we be keeping track of these somehow? e.g. a TODO + a SecondaryMDUI > > check? (so that we just delete the check and the else when we are done?) > > Yeah.. I think we can eventually just remove DIALOG_BUTTON_MARGIN, since it > should always align with PANEL_CONTENT_MARGIN. (Then no need to track, since the > compiler will get them all) O, ok, that's easy then :) https://codereview.chromium.org/2753243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:612: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING)); On 2017/03/22 03:05:59, tapted wrote: > On 2017/03/21 06:24:30, Patti Lor wrote: > > On 2017/03/20 03:41:52, tapted wrote: > > > HORIZONTAL doesn't look right here > > > > Oops, fixed! > > ping? Eek, sorry. Thanks for catching it, it should be actually fixed now (as per your new comment) https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:86: int GetLeftPaddingForBulletedItems(const bool parent_bulleted) { On 2017/03/22 03:05:59, tapted wrote: > nit: no need for `const` Done. https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:607: int padding = GetLeftPaddingForBulletedItems(parent_bulleted); On 2017/03/22 03:05:59, tapted wrote: > nit: inline below (`padding` temporary no required) Done - replaced in 2 places. https://codereview.chromium.org/2753243002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:622: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING)); On 2017/03/22 03:05:59, tapted wrote: > For this, I suggest > > constexpr int kPaddingAboveDetailsLink = 4; > > There is RELATED_CONTROL_VERTICAL_SPACING. But > - that is 8px rather than 4px. > - this isn't a control -- it's just text Done. I also updated all the constants in the anon namespace above to use constexpr.
Description was changed from ========== Views/Harmony: Replace layout constants in chrome/browser/ui/views/extensions. Replace all direct references to constants in layout_constants.cc under chrome/browser/ui/views/extensions/* with LayoutDelegate metrics. BUG=691897 ========== to ========== Views/Harmony: Replace layout constants in chrome/browser/ui/views/extensions. Replace all direct references to constants in ui/views/layout/layout_constants.h under chrome/browser/ui/views/extensions/* with LayoutDelegate metrics. BUG=691897 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:91: LayoutDelegate::Metric::RELATED_BUTTON_HORIZONTAL_SPACING) * Seems like this should be RELATED_CONTROL_HORIZONTAL_SPACING. That's more semantically correct and what the old code looked to have been doing. I'm not 100% convinced that's actually the right value either, but I don't have a better one (existing, or as a new concept) to suggest. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:611: GetLeftPaddingForBulletedItems(parent_bulleted)); Nit: Pull the left padding out into a temp instead of duplicating this call https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:620: layout_->StartRowWithPadding(0, 0, 0, kPaddingAboveDetailsLink); Why add this as a custom value for this file instead of introducing an actual RELATED_CONTROL_VERTICAL_SPACING_SMALL? Seems like we should have that (and in Harmony it should almost certainly be the same value as the non-SMALL variant). https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:130: views::BoxLayout::kVertical, 0, 0, vertical_padding)); This will change the behavior for pre-Harmony, which may be correct, but I wanted to be sure it was an intentional change, and not an unintentional one (or one driven by not having the right constant exposed in LayoutDelegate). https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:159: int spacing = iter + 1 == entries.end() ? vertical_padding : 0; Same comment. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/media_gallery_checkbox_view.cc (right): https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_gallery_checkbox_view.cc:60: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING), Same comment.
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@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.
Thanks, PTAL. I also wrote a question on the attached bug for this CL. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:91: LayoutDelegate::Metric::RELATED_BUTTON_HORIZONTAL_SPACING) * On 2017/03/23 04:21:34, Peter Kasting wrote: > Seems like this should be RELATED_CONTROL_HORIZONTAL_SPACING. That's more > semantically correct and what the old code looked to have been doing. > > I'm not 100% convinced that's actually the right value either, but I don't have > a better one (existing, or as a new concept) to suggest. Yeah - Looks like I used the wrong when converting to a function here, thanks for catching it. Fixed. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:611: GetLeftPaddingForBulletedItems(parent_bulleted)); On 2017/03/23 04:21:33, Peter Kasting wrote: > Nit: Pull the left padding out into a temp instead of duplicating this call Done. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:620: layout_->StartRowWithPadding(0, 0, 0, kPaddingAboveDetailsLink); On 2017/03/23 04:21:34, Peter Kasting wrote: > Why add this as a custom value for this file instead of introducing an actual > RELATED_CONTROL_VERTICAL_SPACING_SMALL? Seems like we should have that (and in > Harmony it should almost certainly be the same value as the non-SMALL variant). Oh - I'd assumed that we were removing stuff from layout_constants.h which didn't correspond to any value in the LayoutDelegate metrics. I've added it in this CL (along with the horizontal small variant), but I'll clarify on the bug what should be matching up with what (and if we are missing any other layout_constants.h constants that should have their own metric). https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:130: views::BoxLayout::kVertical, 0, 0, vertical_padding)); On 2017/03/23 04:21:34, Peter Kasting wrote: > This will change the behavior for pre-Harmony, which may be correct, but I > wanted to be sure it was an intentional change, and not an unintentional one (or > one driven by not having the right constant exposed in LayoutDelegate). Yep - so similar to the previous comment I made, I assumed we were getting rid of the constants not exposed in LayoutDelegate. I've replaced it with the pre-Harmony value now, will discuss further on bug about what I should be doing in future changes. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:159: int spacing = iter + 1 == entries.end() ? vertical_padding : 0; On 2017/03/23 04:21:34, Peter Kasting wrote: > Same comment. Fixed to previous value. https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/media_gallery_checkbox_view.cc (right): https://codereview.chromium.org/2753243002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/media_gallery_checkbox_view.cc:60: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING), On 2017/03/23 04:21:34, Peter Kasting wrote: > Same comment. Fixed to previous value.
LGTM https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc (right): https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc:84: LayoutDelegate::Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING)); Changing kButtonHEdgeMarginNew to UNRELATED_CONTROL_HORIZONTAL_SPACING changes the behavior in pre-Harmony. Seems like maybe you want UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE, which will match the old behavior in pre-Harmony and behave the same as this code in Harmony? (I think this was maybe what Trent was going for and he just typo'd.) https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:89: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING) * It seems like SUBSECTION_INDENT might be theoretically more appropriate for this, but in Harmony that may look really bad. I suspect we'll have to rewrite this layout to work in a better way, which is beyond the scope of this patch, so maybe leaving it like this is OK. :/ https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:44: // related. Nit: "Smaller horizontal spacing between related controls." and similar below. (I'm copying the style of the _LARGE comments below for this wording.)
Thanks for being so thorough! :) https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc (right): https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc:84: LayoutDelegate::Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING)); On 2017/03/24 07:00:51, Peter Kasting wrote: > Changing kButtonHEdgeMarginNew to UNRELATED_CONTROL_HORIZONTAL_SPACING changes > the behavior in pre-Harmony. Seems like maybe you want > UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE, which will match the old behavior in > pre-Harmony and behave the same as this code in Harmony? > > (I think this was maybe what Trent was going for and he just typo'd.) Done. https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:89: LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING) * On 2017/03/24 07:00:51, Peter Kasting wrote: > It seems like SUBSECTION_INDENT might be theoretically more appropriate for > this, but in Harmony that may look really bad. > > I suspect we'll have to rewrite this layout to work in a better way, which is > beyond the scope of this patch, so maybe leaving it like this is OK. :/ Acknowledged. https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2753243002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:44: // related. On 2017/03/24 07:00:51, Peter Kasting wrote: > Nit: "Smaller horizontal spacing between related controls." > > and similar below. (I'm copying the style of the _LARGE comments below for this > wording.) Done.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2753243002/#ps140001 (title: "Review comments.")
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
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 patricialor@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": 140001, "attempt_start_ts": 1490590687519020,
"parent_rev": "da7f7baf81fd53e84850d1f7fbff478cc9b123b7", "commit_rev":
"8a8f67e659f819a459a6076e8fcc634e755dabf6"}
Message was sent while issue was closed.
Description was changed from ========== Views/Harmony: Replace layout constants in chrome/browser/ui/views/extensions. Replace all direct references to constants in ui/views/layout/layout_constants.h under chrome/browser/ui/views/extensions/* with LayoutDelegate metrics. BUG=691897 ========== to ========== Views/Harmony: Replace layout constants in chrome/browser/ui/views/extensions. Replace all direct references to constants in ui/views/layout/layout_constants.h under chrome/browser/ui/views/extensions/* with LayoutDelegate metrics. BUG=691897 Review-Url: https://codereview.chromium.org/2753243002 Cr-Commit-Position: refs/heads/master@{#459707} Committed: https://chromium.googlesource.com/chromium/src/+/8a8f67e659f819a459a6076e8fcc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/8a8f67e659f819a459a6076e8fcc... |
