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

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/extensions/extension_install_dialog_view.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <string>
 #include <utility>
@@ skipping 31 matching lines @@
 #include "ui/native_theme/common_theme.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/scroll_view.h"
 #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;
 using content::Referrer;
 using extensions::ExperienceSamplingEvent;
 
 namespace {
 
 // Width of the bullet column in BulletedView.
 const int kBulletWidth = 20;
@@ skipping 177 matching lines @@
     rating->AddChildView(rating_count);
 
     layout->StartRow(0, column_set_id);
     views::Label* user_count =
         new views::Label(prompt_->GetUserCount(), small_font_list);
     user_count->SetAutoColorReadabilityEnabled(false);
     user_count->SetEnabledColor(SK_ColorGRAY);
     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.
   CustomScrollableView* scrollable = new CustomScrollableView();
   views::GridLayout* scroll_layout = new views::GridLayout(scrollable);
   scrollable->SetLayoutManager(scroll_layout);
 
   views::ColumnSet* scrollable_column_set =
       scroll_layout->AddColumnSet(column_set_id);
 
   scrollable_column_set->AddColumn(
       views::GridLayout::LEADING, views::GridLayout::LEADING,
       0,  // no resizing
       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();
   scroll_view_->set_hide_horizontal_scrollbar(true);
   scroll_view_->SetContents(scrollable);
   layout->AddView(scroll_view_, 4, 1);
 
   if (prompt_->ShouldShowPermissions()) {
     bool has_permissions =
         prompt_->GetPermissionCount(
             ExtensionInstallPrompt::PermissionsType::ALL_PERMISSIONS) > 0;
     if (has_permissions) {
       AddPermissions(
           scroll_layout, rb, column_set_id, content_width,
           ExtensionInstallPrompt::PermissionsType::REGULAR_PERMISSIONS);
       AddPermissions(
           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));
       permission_label->SetMultiLine(true);
       permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
       permission_label->SizeToFit(content_width);
       scroll_layout->AddView(permission_label);
     }
   }
 
   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 =
         new views::Label(prompt_->GetRetainedFilesHeading());
     retained_files_header->SetMultiLine(true);
     retained_files_header->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     retained_files_header->SizeToFit(content_width);
     scroll_layout->AddView(retained_files_header);
 
     scroll_layout->StartRow(0, column_set_id);
     PermissionDetails details;
     for (size_t i = 0; i < prompt_->GetRetainedFileCount(); ++i) {
       details.push_back(prompt_->GetRetainedFile(i));
     }
     ExpandableContainerView* issue_advice_view =
         new ExpandableContainerView(details, content_width, false);
     scroll_layout->AddView(issue_advice_view);
   }
 
   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 =
         new views::Label(prompt_->GetRetainedDevicesHeading());
     retained_devices_header->SetMultiLine(true);
     retained_devices_header->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     retained_devices_header->SizeToFit(content_width);
     scroll_layout->AddView(retained_devices_header);
 
     scroll_layout->StartRow(0, column_set_id);
     PermissionDetails details;
     for (size_t i = 0; i < prompt_->GetRetainedDeviceCount(); ++i) {
       details.push_back(prompt_->GetRetainedDeviceMessageString(i));
     }
     ExpandableContainerView* issue_advice_view =
         new ExpandableContainerView(details, content_width, false);
     scroll_layout->AddView(issue_advice_view);
   }
 
   DCHECK_GE(prompt_->type(), 0);
   UMA_HISTOGRAM_ENUMERATION("Extensions.InstallPrompt.Type",
                             prompt_->type(),
                             ExtensionInstallPrompt::NUM_PROMPT_TYPES);
 
   scroll_view_->ClipHeightTo(
       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(
       ExtensionInstallPrompt::PromptTypeToString(prompt_->type()));
   sampling_event_ = ExperienceSamplingEvent::Create(event_name);
 }
 
 bool ExtensionInstallDialogView::AddPermissions(
     views::GridLayout* layout,
     ui::ResourceBundle& rb,
     int column_set_id,
     int left_column_width,
     ExtensionInstallPrompt::PermissionsType perm_type) {
   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 =
       new views::Label(prompt_->GetPermissionsHeading(perm_type));
   permissions_header->SetMultiLine(true);
   permissions_header->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   permissions_header->SizeToFit(left_column_width);
   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));
 
     permission_label->SetMultiLine(true);
     permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     permission_label->SizeToFit(left_column_width - kBulletWidth);
     layout->AddView(new BulletedView(permission_label));
 
     // If we have more details to provide, show them in collapsed form.
     if (!prompt_->GetPermissionsDetails(i, perm_type).empty()) {
       layout->StartRow(0, column_set_id);
       PermissionDetails details;
       details.push_back(PrepareForDisplay(
           prompt_->GetPermissionsDetails(i, perm_type), false));
       ExpandableContainerView* details_container =
           new ExpandableContainerView(details, left_column_width, true);
       layout->AddView(details_container);
     }
   }
   return true;
 }
 
 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_);
 
   views::ColumnSet* column_set = layout->AddColumnSet(column_set_id);
   column_set->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING,
                         0,  // no resizing
                         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 =
       new views::Label(prompt_->GetDialogTitle(),
                        ui::ResourceBundle::GetSharedInstance().GetFontList(
                            ui::ResourceBundle::MediumFont));
   title->SetMultiLine(true);
   title->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   title->SizeToFit(left_column_width);
 
@@ skipping 127 matching lines @@
                                                   bool parent_bulleted)
     : layout_(new views::GridLayout(this)),
       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);
+      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,
                         0,
                         views::GridLayout::FIXED,
                         horizontal_space - padding,
                         0);
 }
 
 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);
   detail_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   layout_->AddView(detail_label);
 }
 
 gfx::Size ExpandableContainerView::DetailsView::GetPreferredSize() const {
   gfx::Size size = views::View::GetPreferredSize();
   return gfx::Size(size.width(), size.height() * state_);
@@ skipping 43 matching lines @@
   link->SetText(l10n_util::GetStringUTF16(IDS_EXTENSIONS_SHOW_DETAILS));
   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));
+      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,
                         0,
                         views::GridLayout::FIXED,
                         link_col_width,
                         link_col_width);
   // The Up/Down arrow column.
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::TRAILING,
@@ skipping 62 matching lines @@
   gfx::ImageSkia icon = gfx::CreateVectorIcon(
       expanded ? kCaretUpIcon : kCaretDownIcon, gfx::kChromeIconGrey);
   arrow_toggle_->SetImage(views::Button::STATE_NORMAL, &icon);
 }
 
 // static
 ExtensionInstallPrompt::ShowDialogCallback
 ExtensionInstallPrompt::GetViewsShowDialogCallback() {
   return base::Bind(&ShowExtensionInstallDialogImpl);
 }