Material Page Info (Views, 3/3): Update site settings section.

chrome/browser/ui/views/website_settings/chosen_object_row.cc

Index: chrome/browser/ui/views/website_settings/chosen_object_row.cc
diff --git a/chrome/browser/ui/views/website_settings/chosen_object_row.cc b/chrome/browser/ui/views/website_settings/chosen_object_row.cc
index 0a4661a685729d4b025046e938fd24a3786685a4..f345fa7458454e1157c6c7b6472c30621a34ffb2 100644
--- a/chrome/browser/ui/views/website_settings/chosen_object_row.cc
+++ b/chrome/browser/ui/views/website_settings/chosen_object_row.cc
@@ -8,42 +8,42 @@
 #include "chrome/browser/ui/views/website_settings/website_settings_popup_view.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/font_list.h"
 #include "ui/resources/grit/ui_resources.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/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
 
 ChosenObjectRow::ChosenObjectRow(
-    std::unique_ptr<WebsiteSettingsUI::ChosenObjectInfo> info)
+    std::unique_ptr<WebsiteSettingsUI::ChosenObjectInfo> info,
+    views::GridLayout* layout)
     : info_(std::move(info)) {
-  views::GridLayout* layout = new views::GridLayout(this);
-  SetLayoutManager(layout);
-  const int column_set_id = 0;
-  views::ColumnSet* column_set = layout->AddColumnSet(column_set_id);
-  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                        views::GridLayout::FIXED, kPermissionIconColumnWidth,
-                        0);
-  column_set->AddPaddingColumn(0, kPermissionIconMarginLeft);
-  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                        views::GridLayout::USE_PREF, 0, 0);
-  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                        views::GridLayout::USE_PREF, 0, 0);
-
-  layout->StartRow(1, column_set_id);
   // Create the permission icon.
   icon_ = new views::ImageView();
   const gfx::Image& image =
       WebsiteSettingsUI::GetChosenObjectIcon(*info_, false);
   icon_->SetImage(image.ToImageSkia());
-  layout->AddView(icon_, 1, 1, views::GridLayout::CENTER,
-                  views::GridLayout::CENTER);
+  layout->AddView(icon_);
+
+  label_with_delete_ = new views::View();

[msw, 2016/09/26 20:47:55]

Why create a view to contain the label and the delete button? Can't they be
added directly to the grid layout? Can you post a screenshot of the delete
button or point to any on the bug? (I can't find one)

[lgarron, 2016/09/28 21:11:45]

They could be added directly, but then I'd need to use a different column set,
and redundantly implement the icon layout for it.

I just realized I forgot to upload a recent screenshot with a chosen object. See
https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c92

[msw, 2016/09/30 00:37:04]

It'd be nice to avoid unnecessary containers, but I suppose it's okay as-is.

[lgarron, 2016/09/30 05:05:55]

Acknowledged.

+  views::BoxLayout* box_layout =
+      new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 5);

[msw, 2016/09/26 20:47:55]

optional nit: use a views layout constant (kRelatedButtonHSpacing is 6), or a
function/file-local constant, instead of the literal '5'.

[lgarron, 2016/09/28 21:11:45]

Thanks for the tip; I've changed to views::kRelatedButtonHSpacing.

+  box_layout->set_cross_axis_alignment(
+      views::BoxLayout::CROSS_AXIS_ALIGNMENT_END);
+  label_with_delete_->SetLayoutManager(box_layout);
+
   // Create the label that displays the permission type.
-  views::Label* label = new views::Label(l10n_util::GetStringFUTF16(
-      info_->ui_info.label_string_id,
-      WebsiteSettingsUI::ChosenObjectToUIString(*info_)));
-  layout->AddView(label, 1, 1, views::GridLayout::LEADING,
-                  views::GridLayout::CENTER);
+  const gfx::FontList& font_list =
+      ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(1);

[msw, 2016/09/26 20:47:55]

I find it odd to just toss around 1px taller fonts in random places. It's
unclear how this relates to the title and other elements in the bubble, and
having these size delta literals scattered throughout the directory makes it
difficult to reason about the overall layout and font size
consistency/relations. Since we are phasing out ResourceBundle::FontStyle and
related constants, I wonder if we should have a centralized place to put font
size deltas for Views UI or website settings. WDYT? Maybe ping tapted?

[lgarron, 2016/09/28 21:11:45]

Brought it up with Max (designer) and Emily (PM) and you CCed on the design doc.

+  views::Label* label =
+      new views::Label(l10n_util::GetStringFUTF16(
+                           info_->ui_info.label_string_id,
+                           WebsiteSettingsUI::ChosenObjectToUIString(*info_)),
+                       font_list);
+  label_with_delete_->AddChildView(label);
+
   // Create the delete button.
   delete_button_ = new views::ImageButton(this);
   delete_button_->SetFocusForPlatform();
@@ -57,8 +57,8 @@ ChosenObjectRow::ChosenObjectRow(
                            rb.GetImageSkiaNamed(IDR_CLOSE_2_H));
   delete_button_->SetImage(views::ImageButton::STATE_PRESSED,
                            rb.GetImageSkiaNamed(IDR_CLOSE_2_P));
-  layout->AddView(delete_button_, 1, 1, views::GridLayout::LEADING,
-                  views::GridLayout::CENTER);
+  label_with_delete_->AddChildView(delete_button_);
+  layout->AddView(label_with_delete_);
 }
 
 void ChosenObjectRow::AddObserver(ChosenObjectRowObserver* observer) {
@@ -74,7 +74,7 @@ void ChosenObjectRow::ButtonPressed(views::Button* sender,
       WebsiteSettingsUI::GetChosenObjectIcon(*info_, true);
   icon_->SetImage(image.ToImageSkia());
 
-  RemoveChildView(delete_button_);
+  label_with_delete_->RemoveChildView(delete_button_);
   delete delete_button_;
   delete_button_ = nullptr;