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;
+constexpr int kBulletWidth = 20;
 
 // Size of extension icon in top left of dialog.
-const int kIconSize = 64;
+constexpr int kIconSize = 64;
+
+// The padding to put just above the link to view more details.
+constexpr int kPaddingAboveDetailsLink = 4;
 
 // The maximum height of the scroll view before it will show a scrollbar.
-const int kScrollViewMaxHeight = 250;
+constexpr int kScrollViewMaxHeight = 250;
 
 // Width of the left column of the dialog when the extension requests
 // permissions.
-const int kPermissionsLeftColumnWidth = 250;
+constexpr int kPermissionsLeftColumnWidth = 250;
 
 // Width of the left column of the dialog when the extension requests no
 // permissions.
-const int kNoPermissionsLeftColumnWidth = 200;
+constexpr int kNoPermissionsLeftColumnWidth = 200;
 
 // Width of the left column for external install prompts. The text is long in
 // this case, so make it wider than normal.
-const int kExternalInstallLeftColumnWidth = 350;
+constexpr 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(bool parent_bulleted) {
+  return LayoutDelegate::Get()->GetMetric(
+             LayoutDelegate::Metric::RELATED_BUTTON_HORIZONTAL_SPACING) *

[Peter Kasting, 2017/03/23 04:21:34]

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.

[Patti Lor, 2017/03/24 06:37:12]

Yeah - Looks like I used the wrong when converting to a function here, thanks
for catching it. Fixed.

+         (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();
   image_view->SetImage(*skia_image);
   parent->AddChildView(image_view);
 }
 
 // Creates a string for displaying |message| to the user. If it has to look
 // like a entry in a bullet point list, one is added.
@@ skipping 149 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 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.
   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);
+  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, vertical_padding);
       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, vertical_padding);
 
     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, vertical_padding);
 
     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 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 =
       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, vertical_padding);
     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();
-  // 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_);
 
   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));
+      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 =
       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 122 matching lines @@
 }
 
 // ExpandableContainerView::DetailsView ----------------------------------------
 
 ExpandableContainerView::DetailsView::DetailsView(int horizontal_space,
                                                   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);
-  column_set->AddPaddingColumn(0, padding);
-  column_set->AddColumn(views::GridLayout::LEADING,
-                        views::GridLayout::LEADING,
-                        0,
-                        views::GridLayout::FIXED,
-                        horizontal_space - padding,
-                        0);
+  column_set->AddPaddingColumn(0,
+                               GetLeftPaddingForBulletedItems(parent_bulleted));

[Peter Kasting, 2017/03/23 04:21:33]

Nit: Pull the left padding out into a temp instead of duplicating this call

[Patti Lor, 2017/03/24 06:37:12]

Done.

+  column_set->AddColumn(
+      views::GridLayout::LEADING, views::GridLayout::LEADING, 0,
+      views::GridLayout::FIXED,
+      horizontal_space - GetLeftPaddingForBulletedItems(parent_bulleted), 0);
 }
 
 void ExpandableContainerView::DetailsView::AddDetail(
     const base::string16& detail) {
-  layout_->StartRowWithPadding(0, 0,
-                               0, views::kRelatedControlSmallVerticalSpacing);
+  layout_->StartRowWithPadding(0, 0, 0, kPaddingAboveDetailsLink);

[Peter Kasting, 2017/03/23 04:21:34]

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

[Patti Lor, 2017/03/24 06:37:12]

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

   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 28 matching lines @@
 
   details_view_ = new DetailsView(horizontal_space, parent_bulleted);
 
   layout->StartRow(0, column_set_id);
   layout->AddView(details_view_);
 
   for (size_t i = 0; i < details.size(); ++i)
     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();
   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));
+  // 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,
                         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);
 }