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

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

 // Copyright 2015 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/website_settings/chosen_object_row.h"
 
 #include "chrome/browser/ui/views/website_settings/chosen_object_row_observer.h"
 #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();
   delete_button_->set_request_focus_on_press(true);
   delete_button_->SetTooltipText(
       l10n_util::GetStringUTF16(info_->ui_info.delete_tooltip_string_id));
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   delete_button_->SetImage(views::ImageButton::STATE_NORMAL,
                            rb.GetImageSkiaNamed(IDR_CLOSE_2));
   delete_button_->SetImage(views::ImageButton::STATE_HOVERED,
                            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) {
   observer_list_.AddObserver(observer);
 }
 
 ChosenObjectRow::~ChosenObjectRow() {}
 
 void ChosenObjectRow::ButtonPressed(views::Button* sender,
                                     const ui::Event& event) {
   // Change the icon to reflect the selected setting.
   const gfx::Image& image =
       WebsiteSettingsUI::GetChosenObjectIcon(*info_, true);
   icon_->SetImage(image.ToImageSkia());
 
-  RemoveChildView(delete_button_);
+  label_with_delete_->RemoveChildView(delete_button_);
   delete delete_button_;
   delete_button_ = nullptr;
 
   FOR_EACH_OBSERVER(ChosenObjectRowObserver, observer_list_,
                     OnChosenObjectDeleted(*info_));
 }