Views/Harmony: Replace layout constants in chrome/browser/ui/views/extensions.

chrome/browser/ui/views/extensions/extension_install_dialog_view.cc

Index: chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
diff --git a/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc b/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
index 4ccf6d36576cb67b477d04dec83f4388c0a3df68..c9dab0c7cb2ae2c883421ae76974f5dd925b1cb0 100644
--- a/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
+++ b/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
@@ -49,7 +49,6 @@
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
-#include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
 using content::OpenURLParams;
@@ -247,17 +246,20 @@ void ExtensionInstallDialogView::InitView() {
     layout->AddView(user_count);
   }
 
+  LayoutDelegate* layout_delegate = LayoutDelegate::Get();
+  const int related_control_vert_spacing = layout_delegate->GetMetric(

[tapted, 2017/03/20 03:41:52]

maybe just `vertical_padding` ? (same in AddPermissions())

[Patti Lor, 2017/03/21 06:24:30]

Done.

+      LayoutDelegate::Metric::RELATED_CONTROL_VERTICAL_SPACING);
   if (prompt_->ShouldShowPermissions()) {
-    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    layout->AddPaddingRow(0, related_control_vert_spacing);
     layout->StartRow(0, column_set_id);
     layout->AddView(new views::Separator(), 3, 1, views::GridLayout::FILL,
                     views::GridLayout::FILL);
   }
 
-  const int content_width = left_column_width +
-                            LayoutDelegate::Get()->GetMetric(
-                                LayoutDelegate::Metric::PANEL_CONTENT_MARGIN) +
-                            kIconSize;
+  const int content_width =
+      left_column_width +
+      layout_delegate->GetMetric(LayoutDelegate::Metric::PANEL_CONTENT_MARGIN) +
+      kIconSize;
 
   // Create the scrollable view which will contain the permissions and retained
   // files/devices. It will span the full content width.
@@ -274,7 +276,9 @@ void ExtensionInstallDialogView::InitView() {
       views::GridLayout::USE_PREF, content_width, content_width);
 
   // Pad to the very right of the dialog, so the scrollbar will be on the edge.
-  scrollable_column_set->AddPaddingColumn(0, views::kButtonHEdgeMarginNew);
+  const int button_margin =
+      layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN);

[tapted, 2017/03/20 03:41:52]

Similar thing here... this should maybe be PANEL_CONTENT_MARGIN under Harmony,
but I think that will break things pre-Harmony. This is fine.

[Patti Lor, 2017/03/21 06:24:30]

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?)

[tapted, 2017/03/22 03:05:59]

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)

[Patti Lor, 2017/03/22 05:57:40]

O, ok, that's easy then :)

+  scrollable_column_set->AddPaddingColumn(0, button_margin);
 
   layout->StartRow(0, column_set_id);
   scroll_view_ = new views::ScrollView();
@@ -294,7 +298,7 @@ void ExtensionInstallDialogView::InitView() {
           scroll_layout, rb, column_set_id, content_width,
           ExtensionInstallPrompt::PermissionsType::WITHHELD_PERMISSIONS);
     } else {
-      scroll_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+      scroll_layout->AddPaddingRow(0, related_control_vert_spacing);
       scroll_layout->StartRow(0, column_set_id);
       views::Label* permission_label = new views::Label(
           l10n_util::GetStringUTF16(IDS_EXTENSION_NO_SPECIAL_PERMISSIONS));
@@ -306,7 +310,7 @@ void ExtensionInstallDialogView::InitView() {
   }
 
   if (prompt_->GetRetainedFileCount()) {
-    scroll_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    scroll_layout->AddPaddingRow(0, related_control_vert_spacing);
 
     scroll_layout->StartRow(0, column_set_id);
     views::Label* retained_files_header =
@@ -327,7 +331,7 @@ void ExtensionInstallDialogView::InitView() {
   }
 
   if (prompt_->GetRetainedDeviceCount()) {
-    scroll_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    scroll_layout->AddPaddingRow(0, related_control_vert_spacing);
 
     scroll_layout->StartRow(0, column_set_id);
     views::Label* retained_devices_header =
@@ -356,9 +360,8 @@ void ExtensionInstallDialogView::InitView() {
       0,
       std::min(kScrollViewMaxHeight, scrollable->GetPreferredSize().height()));
 
-  dialog_size_ = gfx::Size(
-      content_width + 2 * views::kButtonHEdgeMarginNew,
-      container_->GetPreferredSize().height());
+  dialog_size_ = gfx::Size(content_width + 2 * button_margin,
+                           container_->GetPreferredSize().height());
 
   std::string event_name = ExperienceSamplingEvent::kExtensionInstallDialog;
   event_name.append(
@@ -375,7 +378,9 @@ bool ExtensionInstallDialogView::AddPermissions(
   if (prompt_->GetPermissionCount(perm_type) == 0)
     return false;
 
-  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+  const int related_control_vert_spacing = LayoutDelegate::Get()->GetMetric(
+      LayoutDelegate::Metric::RELATED_CONTROL_VERTICAL_SPACING);
+  layout->AddPaddingRow(0, related_control_vert_spacing);
 
   layout->StartRow(0, column_set_id);
   views::Label* permissions_header =
@@ -386,7 +391,7 @@ bool ExtensionInstallDialogView::AddPermissions(
   layout->AddView(permissions_header);
 
   for (size_t i = 0; i < prompt_->GetPermissionCount(perm_type); ++i) {
-    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    layout->AddPaddingRow(0, related_control_vert_spacing);
     layout->StartRow(0, column_set_id);
     views::Label* permission_label =
         new views::Label(prompt_->GetPermission(i, perm_type));
@@ -414,14 +419,16 @@ views::GridLayout* ExtensionInstallDialogView::CreateLayout(
     int left_column_width,
     int column_set_id) {
   container_ = new views::View();
+  LayoutDelegate* layout_delegate = LayoutDelegate::Get();
+  const int button_margin =

[tapted, 2017/03/20 03:41:52]

I think sensible names are button_margin->horizontal_margin
content_margin->bottom_margin.

[Patti Lor, 2017/03/21 06:24:30]

Done.

+      layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN);
+  const int content_margin =
+      layout_delegate->GetMetric(LayoutDelegate::Metric::PANEL_CONTENT_MARGIN);
   // This is basically views::GridLayout::CreatePanel, but without a top or
   // right margin (we effectively get a top margin anyway from the empty dialog
   // title, and we add an explicit padding column as a right margin below).
   views::GridLayout* layout = new views::GridLayout(container_);
-  layout->SetInsets(0, views::kButtonHEdgeMarginNew,
-                    LayoutDelegate::Get()->GetMetric(
-                        LayoutDelegate::Metric::PANEL_CONTENT_MARGIN),
-                    0);
+  layout->SetInsets(0, button_margin, content_margin, 0);
   container_->SetLayoutManager(layout);
   AddChildView(container_);
 
@@ -431,15 +438,13 @@ views::GridLayout* ExtensionInstallDialogView::CreateLayout(
                         views::GridLayout::USE_PREF,
                         0,  // no fixed width
                         left_column_width);
-  column_set->AddPaddingColumn(
-      0, LayoutDelegate::Get()->GetMetric(
-             LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
+  column_set->AddPaddingColumn(0, content_margin);

[tapted, 2017/03/20 03:41:52]

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?

[Patti Lor, 2017/03/21 06:24:29]

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.

[tapted, 2017/03/22 03:05:59]

looks great!

   column_set->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
                         0,  // no resizing
                         views::GridLayout::USE_PREF,
                         0,  // no fixed width
                         kIconSize);
-  column_set->AddPaddingColumn(0, views::kButtonHEdgeMarginNew);
+  column_set->AddPaddingColumn(0, button_margin);
 
   layout->StartRow(0, column_set_id);
   views::Label* title =
@@ -587,7 +592,9 @@ ExpandableContainerView::DetailsView::DetailsView(int horizontal_space,
   // parent. Therefore increase the indentation by one more unit to show that it
   // is in fact a child item (with no missing bullet) and not a sibling.
   int padding =
-      views::kRelatedControlHorizontalSpacing * (parent_bulleted ? 2 : 1);
+      LayoutDelegate::Get()->GetMetric(
+          LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING) *
+      (parent_bulleted ? 2 : 1);
   column_set->AddPaddingColumn(0, padding);
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,
@@ -599,8 +606,10 @@ ExpandableContainerView::DetailsView::DetailsView(int horizontal_space,
 
 void ExpandableContainerView::DetailsView::AddDetail(
     const base::string16& detail) {
-  layout_->StartRowWithPadding(0, 0,
-                               0, views::kRelatedControlSmallVerticalSpacing);
+  layout_->StartRowWithPadding(
+      0, 0, 0,
+      LayoutDelegate::Get()->GetMetric(
+          LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING));

[tapted, 2017/03/20 03:41:52]

HORIZONTAL doesn't look right here

[Patti Lor, 2017/03/21 06:24:30]

Oops, fixed!

[tapted, 2017/03/22 03:05:59]

ping?

[Patti Lor, 2017/03/22 05:57:40]

Eek, sorry. Thanks for catching it, it should be actually fixed now (as per your
new comment)

   views::Label* detail_label =
       new views::Label(PrepareForDisplay(detail, false));
   detail_label->SetMultiLine(true);
@@ -664,7 +673,9 @@ ExpandableContainerView::ExpandableContainerView(
   // increase the indentation by one more unit to show that it is in fact a
   // child item (with no missing bullet) and not a sibling.
   column_set->AddPaddingColumn(
-      0, views::kRelatedControlHorizontalSpacing * (parent_bulleted ? 2 : 1));
+      0, LayoutDelegate::Get()->GetMetric(
+             LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING) *
+             (parent_bulleted ? 2 : 1));

[tapted, 2017/03/20 03:41:52]

This should be pulled out (with its comment) to a helper function - in an
anonymous namespace

[Patti Lor, 2017/03/21 06:24:30]

Done.

   // The More Details column.
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,