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..506480da904d07c509dd0b5ca115d3ff5918343c 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;
@@ -79,6 +78,17 @@ const int kNoPermissionsLeftColumnWidth = 200;
 // this case, so make it wider than normal.
 const int kExternalInstallLeftColumnWidth = 350;
 
+// Get the appropriate indentation for an item if its parent is using bullet
+// points. If the parent is using bullets for its items, then a padding of one
+// unit will make the child item (which has no bullet) look like a sibling of
+// its 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 GetLeftPaddingForBulletedItems(const bool parent_bulleted) {

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

nit: no need for `const`

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

Done.

+  return LayoutDelegate::Get()->GetMetric(
+             LayoutDelegate::Metric::RELATED_BUTTON_HORIZONTAL_SPACING) *
+         (parent_bulleted ? 2 : 1);
+}
+
 void AddResourceIcon(const gfx::ImageSkia* skia_image, void* data) {
   views::View* parent = static_cast<views::View*>(data);
   views::ImageView* image_view = new views::ImageView();
@@ -247,17 +257,20 @@ void ExtensionInstallDialogView::InitView() {
     layout->AddView(user_count);
   }
 
+  LayoutDelegate* layout_delegate = LayoutDelegate::Get();
+  const int vertical_padding = layout_delegate->GetMetric(
+      LayoutDelegate::Metric::RELATED_CONTROL_VERTICAL_SPACING);
   if (prompt_->ShouldShowPermissions()) {
-    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    layout->AddPaddingRow(0, vertical_padding);
     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 +287,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);
+  scrollable_column_set->AddPaddingColumn(0, button_margin);
 
   layout->StartRow(0, column_set_id);
   scroll_view_ = new views::ScrollView();
@@ -294,7 +309,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, vertical_padding);
       scroll_layout->StartRow(0, column_set_id);
       views::Label* permission_label = new views::Label(
           l10n_util::GetStringUTF16(IDS_EXTENSION_NO_SPECIAL_PERMISSIONS));
@@ -306,7 +321,7 @@ void ExtensionInstallDialogView::InitView() {
   }
 
   if (prompt_->GetRetainedFileCount()) {
-    scroll_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    scroll_layout->AddPaddingRow(0, vertical_padding);
 
     scroll_layout->StartRow(0, column_set_id);
     views::Label* retained_files_header =
@@ -327,7 +342,7 @@ void ExtensionInstallDialogView::InitView() {
   }
 
   if (prompt_->GetRetainedDeviceCount()) {
-    scroll_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    scroll_layout->AddPaddingRow(0, vertical_padding);
 
     scroll_layout->StartRow(0, column_set_id);
     views::Label* retained_devices_header =
@@ -356,9 +371,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 +389,9 @@ bool ExtensionInstallDialogView::AddPermissions(
   if (prompt_->GetPermissionCount(perm_type) == 0)
     return false;
 
-  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+  const int vertical_padding = LayoutDelegate::Get()->GetMetric(
+      LayoutDelegate::Metric::RELATED_CONTROL_VERTICAL_SPACING);
+  layout->AddPaddingRow(0, vertical_padding);
 
   layout->StartRow(0, column_set_id);
   views::Label* permissions_header =
@@ -386,7 +402,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, vertical_padding);
     layout->StartRow(0, column_set_id);
     views::Label* permission_label =
         new views::Label(prompt_->GetPermission(i, perm_type));
@@ -414,14 +430,20 @@ views::GridLayout* ExtensionInstallDialogView::CreateLayout(
     int left_column_width,
     int column_set_id) {
   container_ = new views::View();
-  // 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).
+  LayoutDelegate* layout_delegate = LayoutDelegate::Get();
+  const int horizontal_margin =
+      layout_delegate->GetMetric(LayoutDelegate::Metric::DIALOG_BUTTON_MARGIN);
+  const int bottom_margin =
+      layout_delegate->GetMetric(LayoutDelegate::Metric::PANEL_CONTENT_MARGIN);
+
+  // This is views::GridLayout::CreatePanel(), but without a top or right
+  // margin. The empty dialog title will then become the top margin, and a
+  // padding column will be manually added to handle a right margin. This is
+  // done so that the extension icon can be shown on the right of the dialog
+  // title, but on the same y-axis, and the scroll view used to contain other
+  // content can have its scrollbar aligned with the right edge of the dialog.
   views::GridLayout* layout = new views::GridLayout(container_);
-  layout->SetInsets(0, views::kButtonHEdgeMarginNew,
-                    LayoutDelegate::Get()->GetMetric(
-                        LayoutDelegate::Metric::PANEL_CONTENT_MARGIN),
-                    0);
+  layout->SetInsets(0, horizontal_margin, bottom_margin, 0);
   container_->SetLayoutManager(layout);
   AddChildView(container_);
 
@@ -432,14 +454,14 @@ views::GridLayout* ExtensionInstallDialogView::CreateLayout(
                         0,  // no fixed width
                         left_column_width);
   column_set->AddPaddingColumn(
-      0, LayoutDelegate::Get()->GetMetric(
-             LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
+      0, layout_delegate->GetMetric(
+             LayoutDelegate::Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING));
   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, horizontal_margin);
 
   layout->StartRow(0, column_set_id);
   views::Label* title =
@@ -582,12 +604,7 @@ ExpandableContainerView::DetailsView::DetailsView(int horizontal_space,
       state_(0) {
   SetLayoutManager(layout_);
   views::ColumnSet* column_set = layout_->AddColumnSet(0);
-  // If the parent is using bullets for its items, then a padding of one unit
-  // will make the child item (which has no bullet) look like a sibling of its
-  // 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);
+  int padding = GetLeftPaddingForBulletedItems(parent_bulleted);

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

nit: inline below (`padding` temporary no required)

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

Done - replaced in 2 places.

   column_set->AddPaddingColumn(0, padding);
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,
@@ -599,8 +616,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/22 03:05:59]

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

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

Done. I also updated all the constants in the anon namespace above to use
constexpr.

   views::Label* detail_label =
       new views::Label(PrepareForDisplay(detail, false));
   detail_label->SetMultiLine(true);
@@ -649,8 +668,7 @@ ExpandableContainerView::ExpandableContainerView(
     details_view_->AddDetail(details[i]);
 
   // Make sure the link width column is as wide as needed for both Show and
-  // Hide details, so that the arrow doesn't shift horizontally when we
-  // toggle.
+  // Hide details, so that the arrow doesn't shift horizontally when we toggle.
   views::Link* link = new views::Link(
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_HIDE_DETAILS));
   int link_col_width = link->GetPreferredSize().width();
@@ -658,13 +676,9 @@ ExpandableContainerView::ExpandableContainerView(
   link_col_width = std::max(link_col_width, link->GetPreferredSize().width());
 
   column_set = layout->AddColumnSet(++column_set_id);
-  // Padding to the left of the More Details column. If the parent is using
-  // bullets for its items, then a padding of one unit will make the child
-  // item (which has no bullet) look like a sibling of its 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.
-  column_set->AddPaddingColumn(
-      0, views::kRelatedControlHorizontalSpacing * (parent_bulleted ? 2 : 1));
+  // Padding to the left of the More Details column.
+  column_set->AddPaddingColumn(0,
+                               GetLeftPaddingForBulletedItems(parent_bulleted));
   // The More Details column.
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,