Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904443006:
Views/Harmony Remove references to ui/views/layout/layout_constants.h (Closed)
DescriptionViews/Harmony Remove references to ui/views/layout/layout_constants.h
Replace references to constants in ui/views/layout/layout_constants.h with their
equivalents using ChromeLayoutProvider.
BUG=691897
TBR=sky
Review-Url: https://codereview.chromium.org/2904443006
Cr-Commit-Position: refs/heads/master@{#475229}
Committed: https://chromium.googlesource.com/chromium/src/+/cf4f838225c132dee06cc21dd6a576347734589b
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN #Patch Set 3 : Remove layout_constants.h from import_lock_dialog_view.cc #Patch Set 4 : Remove layout_constants.h from import_lock_dialog_view.cc #Patch Set 5 : Fix compile failures #Patch Set 6 : Fix TranslateBubbleViewTest tests by setting up the ChromeLayoutProvider as the default provider #
Total comments: 40
Patch Set 7 : Address round 1 of review comments #Patch Set 8 : Remove includes of layout_constants.h from ui/views #
Total comments: 19
Patch Set 9 : Address next round of comments #
Total comments: 2
Patch Set 10 : Address next round of review comments #Patch Set 11 : Add layout_constants.h back to dialog_example.cc as it refers to kUnrelatedControlVerticalSpacing. … #Messages
Total messages: 53 (35 generated)
ananta@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2904443006/diff/1/chrome/browser/ui/views/tra... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/1/chrome/browser/ui/views/tra... chrome/browser/ui/views/translate/translate_bubble_view.cc:618: layout->AddPaddingRow( This possibly can be ripped out. There is a ton of code in this file which does special things like displaying the close button when that flag is enabled, etc. Perhaps for a later patchset
The CQ bit was checked by ananta@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_compile_dbg_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 ananta@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_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 ananta@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.
Note: I'm behind on codereviews but will try and get to this today (Thursday May 25), after I get some sleep.
https://codereview.chromium.org/2904443006/diff/1/chrome/browser/ui/views/tra... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/1/chrome/browser/ui/views/tra... chrome/browser/ui/views/translate/translate_bubble_view.cc:618: layout->AddPaddingRow( On 2017/05/24 06:32:09, ananta wrote: > This possibly can be ripped out. There is a ton of code in this file which does > special things like displaying the close button when that flag is enabled, etc. > Perhaps for a later patchset I think it can't be ripped out. I think this is the new translate UI that they're trying to switch to, but isn't on by default yet. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:193: layout->AddPaddingRow(0, vertical_spacing); Why does this padding row get added at all? It seems like this should just be removed. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:454: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); I think here you should get INSETS_DIALOG, and the use the left() and right() insets from it in the padding columns below. (And insets.width() instead of "2 * button_margin".) https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:483: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN); The use of this value looks really strange. It's a bottom margin, but it's being used in horizontal contexts below rather than vertical ones. And I can't figure out at a glance what kind of layout this is trying to do. The "label padding" contains a button width? What? So I don't know how to tell you to write this without digging deeper. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1109: provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON_ROW).bottom(); Does this dialog even have a title or bottom buttons? Maybe what you really want is just INSETS_BUBBLE_CONTENTS around all four edges. Note that both your way and mine are behavior changes from the old code, because the old code did button margins around everything, and that doesn't match anything in Chrome. Also note that if you change to using the panel insets here then probably almost everything else in the whole file would switch from dialog to panel insets. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1110: const int button_margin = provider->GetDistanceMetric( Nit: This seems like a misnomer, since it's really the content side margin, and it's not button-related in any particular way. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1113: provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL); Nit: Try to init variables just above first use if you can https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1125: CreateSingleColumnLayout(view, kFixedMenuWidth - 2 * button_margin); Doesn't this result in a double margin at the sides? We set side margins in the border, but then add them again in this column layout. Or am I confused? https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1267: provider->GetDistanceMetric(DISTANCE_RELATED_CONTROL_VERTICAL_SMALL)); Nit: I can't figure out your rule for when you use a temp and when you inline. This is repeated below but not pulled out... https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1317: const int vertical_spacing = ...while this is not repeated, but it _is_ pulled out. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1640: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN), This can't be right; the old code and your code are both using a vertical margin value for a horizontal border argument. What is this actually trying to do? https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1674: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN) - Nit: Again, prefer using the width of the dialog contents or button insets instead of (2 * horizontal margin). https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1702: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN) - This uses a vertical margin value in a horizontal context. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1717: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); Same comments as earlier about using insets left/right/width instead of using the horizontal margin directly. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1725: views::CreateEmptyBorder(0, button_margin, button_margin, button_margin)); This might be wanting to use the button insets directly? https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1841: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); Again, same comment about using insets. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:556: 0, provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL)); Should this be DISTANCE_RELATED_LABEL_HORIZONTAL? https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:618: layout->AddPaddingRow( The two arms of the conditional are the same. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:686: 0, provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL)); Again, should this be DISTANCE_RELATED_LABEL_HORIZONTAL?
https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:193: layout->AddPaddingRow(0, vertical_spacing); On 2017/05/26 00:50:24, Peter Kasting wrote: > Why does this padding row get added at all? It seems like this should just be > removed. Done. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:454: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); On 2017/05/26 00:50:24, Peter Kasting wrote: > I think here you should get INSETS_DIALOG, and the use the left() and right() > insets from it in the padding columns below. (And insets.width() instead of "2 > * button_margin".) Done. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:483: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN); On 2017/05/26 00:50:24, Peter Kasting wrote: > The use of this value looks really strange. It's a bottom margin, but it's > being used in horizontal contexts below rather than vertical ones. And I can't > figure out at a glance what kind of layout this is trying to do. The "label > padding" contains a button width? What? > > So I don't know how to tell you to write this without digging deeper. I changed this to use DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN which is what kButtonHEdgeMarginNew returns. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1109: provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON_ROW).bottom(); On 2017/05/26 00:50:24, Peter Kasting wrote: > Does this dialog even have a title or bottom buttons? Maybe what you really > want is just INSETS_BUBBLE_CONTENTS around all four edges. > > Note that both your way and mine are behavior changes from the old code, because > the old code did button margins around everything, and that doesn't match > anything in Chrome. Also note that if you change to using the panel insets here > then probably almost everything else in the whole file would switch from dialog > to panel insets. The values for INSETS_BUBBLE_CONTENTS are 13 each while the kButtonVEdgeMarginNew and kButtonHEdgeMarginNew are defined as 20. I was worried about breaking something. I changed the names to border_top, border_bottom, border_margin_left_right to make it more meaningful and retain current behavior. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1110: const int button_margin = provider->GetDistanceMetric( On 2017/05/26 00:50:24, Peter Kasting wrote: > Nit: This seems like a misnomer, since it's really the content side margin, and > it's not button-related in any particular way. Renamed to border_margin_left_right https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1113: provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL); On 2017/05/26 00:50:24, Peter Kasting wrote: > Nit: Try to init variables just above first use if you can Done. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1125: CreateSingleColumnLayout(view, kFixedMenuWidth - 2 * button_margin); On 2017/05/26 00:50:24, Peter Kasting wrote: > Doesn't this result in a double margin at the sides? We set side margins in the > border, but then add them again in this column layout. > > Or am I confused? I don't know actually. The comments for the CreateSingleColumnLayout function states that it adds a single column which ensures that child views automatically fill out to the width of the bubble. Added a TODO there to revisit. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1267: provider->GetDistanceMetric(DISTANCE_RELATED_CONTROL_VERTICAL_SMALL)); On 2017/05/26 00:50:24, Peter Kasting wrote: > Nit: I can't figure out your rule for when you use a temp and when you inline. > This is repeated below but not pulled out... sorry i missed it. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1317: const int vertical_spacing = On 2017/05/26 00:50:24, Peter Kasting wrote: > ...while this is not repeated, but it _is_ pulled out. Thanks. Removed this. Will ensure that in future changes, I define the constants just before they are used. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1640: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN), On 2017/05/26 00:50:24, Peter Kasting wrote: > This can't be right; the old code and your code are both using a vertical margin > value for a horizontal border argument. What is this actually trying to do? Looks like a border around a link. The padding at the top appears related to the border?. I changed it to use DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1674: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN) - On 2017/05/26 00:50:24, Peter Kasting wrote: > Nit: Again, prefer using the width of the dialog contents or button insets > instead of (2 * horizontal margin). Changed to use dialog_insets.width()? https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1702: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN) - On 2017/05/26 00:50:24, Peter Kasting wrote: > This uses a vertical margin value in a horizontal context. Thanks. Changed to use DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1717: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); On 2017/05/26 00:50:24, Peter Kasting wrote: > Same comments as earlier about using insets left/right/width instead of using > the horizontal margin directly. Done. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1725: views::CreateEmptyBorder(0, button_margin, button_margin, button_margin)); On 2017/05/26 00:50:24, Peter Kasting wrote: > This might be wanting to use the button insets directly? Done. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1841: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); On 2017/05/26 00:50:24, Peter Kasting wrote: > Again, same comment about using insets. Done. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1849: int label_width = kFixedSwitchUserViewWidth - 2 * button_margin; Changed this to dialog_insets.width() https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:556: 0, provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL)); On 2017/05/26 00:50:24, Peter Kasting wrote: > Should this be DISTANCE_RELATED_LABEL_HORIZONTAL? Yeah. seems like it. done https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:618: layout->AddPaddingRow( On 2017/05/26 00:50:24, Peter Kasting wrote: > The two arms of the conditional are the same. We don't have a replacement for kPanelSubVerticalSpacing yet. I picked the one closes to 24 :( which is DISTANCE_UNRELATED_CONTROL_VERTICAL https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:686: 0, provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL)); On 2017/05/26 00:50:25, Peter Kasting wrote: > Again, should this be DISTANCE_RELATED_LABEL_HORIZONTAL? Yes. Sorry i was just doing a find and replace without much thinking :(
The CQ bit was checked by ananta@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 patchset includes layout_constants.h include removal from ui/views. The only file which includes it now is layout_provider.h
The CQ bit was checked by ananta@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:483: views::DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN); On 2017/05/26 03:23:24, ananta wrote: > On 2017/05/26 00:50:24, Peter Kasting wrote: > > The use of this value looks really strange. It's a bottom margin, but it's > > being used in horizontal contexts below rather than vertical ones. And I > can't > > figure out at a glance what kind of layout this is trying to do. The "label > > padding" contains a button width? What? > > > > So I don't know how to tell you to write this without digging deeper. > > I changed this to use DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN which is what > kButtonHEdgeMarginNew returns. OK. I still wish I understood what this was doing. https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:618: layout->AddPaddingRow( On 2017/05/26 03:23:25, ananta wrote: > On 2017/05/26 00:50:24, Peter Kasting wrote: > > The two arms of the conditional are the same. > > We don't have a replacement for kPanelSubVerticalSpacing yet. I picked the one > closes to 24 :( which is DISTANCE_UNRELATED_CONTROL_VERTICAL My point was more, there's no reason to keep the conditional. We should just collapse this into a single unconditional statement. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:454: provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS); Before we land this, I'd like to see what the visual effect on the profile switcher is, since this is a behavior change; I want to make sure we're not going to make things look way worse. (I'm confident that _some_ behavior change is desirable, just not what the right change is.) My suspicion is we really are going to want the bubble contents and not the dialog contents, everywhere in this file. But either way I'll want to see the difference. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1117: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); Don't use a single "left/right" constant; use the dialog contents left/right/width below. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1167: Nit: Don't add a blank line https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1278: DISTANCE_RELATED_CONTROL_VERTICAL_SMALL); Nit: You pulled this out into a temp but didn't use the temp later on in this function where you could have. I'd also consider naming this "small_vertical_spacing" or something since this function also uses the non-small value, and the two are sort of confusable. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1656: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN), Nit: Looks like this is really the dialog contents inset's left() value? https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1719: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN) - ...And this is the insets' right() value. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1733: const int button_margin = provider->GetDistanceMetric( Nit: You shouldn't be retrieving this value; replace the 2 * button_margin below with the width() of the insets. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1740: view, kFixedAccountRemovalViewWidth - 2 * button_margin); Note that this is another place that appears to create a double margin. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1746: button_insets.top(), button_insets.left(), This is a behavior change; the old code passed 0 for the top inset. I think if you used DIALOG_BUTTON_ROW instead of DIALOG_CONTENTS, you'd get what you wanted here; but I'm not sure whether that, or using DIALOG_CONTENTS but passing 0 for the top arg here is more semantically correct, because I don't know exactly where in the control this lays out. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1872: columns->AddPaddingColumn(0, dialog_insets.left()); This should be right().
https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/2904443006/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:618: layout->AddPaddingRow( On 2017/05/26 16:19:40, Peter Kasting wrote: > On 2017/05/26 03:23:25, ananta wrote: > > On 2017/05/26 00:50:24, Peter Kasting wrote: > > > The two arms of the conditional are the same. > > > > We don't have a replacement for kPanelSubVerticalSpacing yet. I picked the one > > closes to 24 :( which is DISTANCE_UNRELATED_CONTROL_VERTICAL > > My point was more, there's no reason to keep the conditional. We should just > collapse this into a single unconditional statement. Thanks. done https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:454: provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS); On 2017/05/26 16:19:40, Peter Kasting wrote: > Before we land this, I'd like to see what the visual effect on the profile > switcher is, since this is a behavior change; I want to make sure we're not > going to make things look way worse. (I'm confident that _some_ behavior change > is desirable, just not what the right change is.) > > My suspicion is we really are going to want the bubble contents and not the > dialog contents, everywhere in this file. But either way I'll want to see the > difference. Noted https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1117: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN); On 2017/05/26 16:19:41, Peter Kasting wrote: > Don't use a single "left/right" constant; use the dialog contents > left/right/width below. Done. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1167: On 2017/05/26 16:19:40, Peter Kasting wrote: > Nit: Don't add a blank line Done. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1278: DISTANCE_RELATED_CONTROL_VERTICAL_SMALL); On 2017/05/26 16:19:41, Peter Kasting wrote: > Nit: You pulled this out into a temp but didn't use the temp later on in this > function where you could have. > > I'd also consider naming this "small_vertical_spacing" or something since this > function also uses the non-small value, and the two are sort of confusable. Done. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1656: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN), On 2017/05/26 16:19:41, Peter Kasting wrote: > Nit: Looks like this is really the dialog contents inset's left() value? Done. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1719: views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN) - On 2017/05/26 16:19:44, Peter Kasting wrote: > ...And this is the insets' right() value. Done. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1740: view, kFixedAccountRemovalViewWidth - 2 * button_margin); On 2017/05/26 16:19:42, Peter Kasting wrote: > Note that this is another place that appears to create a double margin. Noted. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1746: button_insets.top(), button_insets.left(), On 2017/05/26 16:19:41, Peter Kasting wrote: > This is a behavior change; the old code passed 0 for the top inset. > > I think if you used DIALOG_BUTTON_ROW instead of DIALOG_CONTENTS, you'd get what > you wanted here; but I'm not sure whether that, or using DIALOG_CONTENTS but > passing 0 for the top arg here is more semantically correct, because I don't > know exactly where in the control this lays out. Thanks for noticing this. Done. Set the top to 0. https://codereview.chromium.org/2904443006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1872: columns->AddPaddingColumn(0, dialog_insets.left()); On 2017/05/26 16:19:44, Peter Kasting wrote: > This should be right(). Done.
Displaying the profile UI via browser_tests is a touch more involved as these are not dialogs. Additionally there is a lot of UI here which appears to be enabled only if the --enable-account-consistency switch is passed in. A cursory examination of the profile UI on my local build appears to be ok. If it ok with you we can land this patch and then iterate/rewrite the profile views to be more testable.
LGTM https://codereview.chromium.org/2904443006/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1115: view->SetBorder(views::CreateEmptyBorder( Simpler: view->SetBorder(views::CreateEmptyBorder(dialog_insets));
The CQ bit was checked by ananta@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/2904443006/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2904443006/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1115: view->SetBorder(views::CreateEmptyBorder( On 2017/05/27 00:00:58, Peter Kasting wrote: > Simpler: > > view->SetBorder(views::CreateEmptyBorder(dialog_insets)); Thanks done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by ananta@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/2904443006/#ps180001 (title: "Address next round of 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Views/Harmony Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 ========== to ========== Views/Harmony Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 ==========
ananta@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== Views/Harmony Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 ========== to ========== Views/Harmony Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 TBR=sky ==========
+sky for ui/views owners. TBR'ing as these are boilerplate changes to fetch metrics using the layout provider instead of constants. Will address comments if any in a followup
The CQ bit was checked by ananta@chromium.org
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 ananta@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/2904443006/#ps200001 (title: "Add layout_constants.h back to dialog_example.cc as it refers to kUnrelatedControlVerticalSpacing. …")
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": 200001, "attempt_start_ts": 1495851071690860,
"parent_rev": "dd2fa336d116912de565aa11f9b916341be3d579", "commit_rev":
"cf4f838225c132dee06cc21dd6a576347734589b"}
Message was sent while issue was closed.
Description was changed from ========== Views/Harmony Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 TBR=sky ========== to ========== Views/Harmony Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 TBR=sky Review-Url: https://codereview.chromium.org/2904443006 Cr-Commit-Position: refs/heads/master@{#475229} Committed: https://chromium.googlesource.com/chromium/src/+/cf4f838225c132dee06cc21dd6a5... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/cf4f838225c132dee06cc21dd6a5...
Message was sent while issue was closed.
LGTM |
