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

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

Index: chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
diff --git a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
index 2d848a1d49e9ac38f158263bd431d2874a77640f..07c055f979a8fc7ce9ec5ce12bebd3708db1ae9c 100644
--- a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
+++ b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
@@ -72,42 +72,47 @@ namespace {
 // showing.
 bool is_popup_showing = false;
 
-// Left icon margin.
-const int kIconMarginLeft = 6;
+/**************** General constants ****************/

[lgarron, 2016/09/23 02:07:45]

I've updated the structure and variable names in the constant declaration
namespace to match the new Mac implementation more closely.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/website_settings...

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

nit: consider a more common chrome/* comment heading pattern, like:
// General constants -----------------------------------------------------------

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

Done. Thanks for letting me know the convention; I couldn't find an example!

+
+// Popup width constraints.
+const int kMinPopupWidth = 320;
+const int kMaxPopupWidth = 1000;
+
+// Margin and padding values shared by all sections.
+const int kSectionPaddingHorizontal = 16;

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

nit: try to use a views constant, like kUnrelatedControlHorizontalSpacing (12),
kUnrelatedControlLargeHorizontalSpacing (20) or similar. Ditto for other values,
where it makes sense.

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

This is the outside padding for each section, which I guess matches
kPanelHorizMargin (13). Should I prefer matching the mocks as closely as
possible, or using those values?

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

Sadly, there's no one-size-fits-all answer to that question; it's up to you when
to follow the mocks closely and when to follow standards/conventions. I'd err on
the side of using consts and very rarely tweaking those values (to fit
MD/Harmony/hotness++) and *very* rarely use one-off magic values. If it were up
to me, we'd have a comprehensive set of constants and the designers would define
mocks in terms of those values... ie. instead of a mock saying 'put 5px between
these controls', it should say 'these controls are related, use the standard
horizontal padding for related controls between them'. /rant

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

Yeah, I wish translating specs to code didn't require so much interpretation.
:-/
I'd prefer to match the values from the specs as closely as possible for now,
though, since consensus on them seems fragile. (I did try to consolidate some
constants, though.)

Do the designers know about `ui/views/layout/layout_constants.h`?
It mentions "spec 21/4", which is still a mysterious name to me. I asked about
it once and got no answer. Do you know?

[msw, 2016/09/30 17:18:26]

That line dates back to initial commit, or perhaps some history was trashed,
perhaps it doesn't mean anything to anyone anymore. I doubt most designers
know/care about existing layout constants, native theme colors, or other such
stuff, maybe they have their own list and just inline literal values into the
spec?
https://chromium.googlesource.com/chromium/src/+blame/cee34b707ac41e83ff9d904...

+
+// Margin and padding values shared by all sections.
+const int kPopupMarginBottom = 16;
+
+/**************** Security Section (PopupHeaderView) ****************/
 
 // Margin and padding values for the |PopupHeaderView|.
 const int kHeaderMarginBottom = 10;
 const int kHeaderPaddingBottom = 16;
-const int kHeaderPaddingLeft = 18;
-const int kHeaderPaddingRightForText = kHeaderPaddingLeft;
 const int kHeaderPaddingTop = 16;
 const int kHeaderPaddingForCloseButton = 8;
 
 // Spacing between labels in the header.
 const int kHeaderLabelSpacing = 4;
 
-// The max possible width of the popup.
-const int kMaxPopupWidth = 1000;
+/**************** Site Settings Section ****************/
+
+// Spacing before and after the cookies view.
+const int kCookiesViewVerticalPadding = 6;
 
-// The margins between the popup border and the popup content.
-const int kPopupMarginBottom = 14;
+// Square size of the permission images.
+const int kPermissionImageSize = 16;

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

Does this only apply to vector icons or are image resources being scaled down?
If we are shrinking icon sizes, should we shrink any image resources?

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

All image resources are 16px (default_100_percent) or 32px (default_200_percent)
PNGs [1], although hopefully we will switch to vector [2].
Since the source images are the right size, I believe we do not need to scale.
The icons have already landed and look correct.

[1] https://codereview.chromium.org/2269513003
[2] https://crbug.com/647551

 
-// Padding values for sections on the site settings view.
-const int kSiteSettingsViewContentMinWidth = 300;
-const int kSiteSettingsViewPaddingBottom = 6;
-const int kSiteSettingsViewPaddingLeft = 18;
-const int kSiteSettingsViewPaddingRight = 18;
-const int kSiteSettingsViewPaddingTop = 4;
+// Spacing between a permission image and the text.
+const int kPermissionImageSpacing = 6;
 
-// Space between the headline and the content of a section.
-const int kSiteSettingsViewHeadlineMarginBottom = 10;
-// Spacing between rows in the "Permissions" and "Cookies and Site Data"
-// sections.
-const int kContentRowSpacing = 2;
+// Spacing between rows in the site settings section
+const int kPermissionsVerticalSpacing = 12;
 
-const int kSiteDataIconColumnWidth = 20;
+/**************** Button IDs ****************/
 
 const int BUTTON_RESET_CERTIFICATE_DECISIONS = 1337;
+const int BUTTON_SITE_SETTINGS = 1338;
 
 }  // namespace
 
@@ -192,7 +197,7 @@ PopupHeaderView::PopupHeaderView(
 
   const int label_column = 0;
   views::ColumnSet* column_set = layout->AddColumnSet(label_column);
-  column_set->AddPaddingColumn(0, kHeaderPaddingLeft);
+  column_set->AddPaddingColumn(0, kSectionPaddingHorizontal);
   column_set->AddColumn(views::GridLayout::FILL,
                         views::GridLayout::FILL,
                         1,
@@ -237,10 +242,10 @@ PopupHeaderView::PopupHeaderView(
   const int label_column_status = 1;
   views::ColumnSet* column_set_status =
       layout->AddColumnSet(label_column_status);
-  column_set_status->AddPaddingColumn(0, kHeaderPaddingLeft);
+  column_set_status->AddPaddingColumn(0, kSectionPaddingHorizontal);
   column_set_status->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL,
                                1, views::GridLayout::USE_PREF, 0, 0);
-  column_set_status->AddPaddingColumn(0, kHeaderPaddingRightForText);
+  column_set_status->AddPaddingColumn(0, kSectionPaddingHorizontal);
 
   layout->AddPaddingRow(0, kHeaderLabelSpacing);
 
@@ -454,10 +459,10 @@ WebsiteSettingsPopupView::WebsiteSettingsPopupView(
       header_(nullptr),
       separator_(nullptr),
       site_settings_view_(nullptr),
-      site_data_content_(nullptr),
+      cookies_view_(nullptr),
       cookie_dialog_link_(nullptr),
-      permissions_content_(nullptr),
-      site_settings_link_(nullptr),
+      permissions_view_(nullptr),
+      site_settings_button_(nullptr),
       weak_factory_(this) {
   set_parent_window(parent_window);
 
@@ -534,9 +539,27 @@ int WebsiteSettingsPopupView::GetDialogButtons() const {
 
 void WebsiteSettingsPopupView::ButtonPressed(views::Button* button,
                                              const ui::Event& event) {
-  if (button->id() == BUTTON_RESET_CERTIFICATE_DECISIONS)
-    presenter_->OnRevokeSSLErrorBypassButtonPressed();
-  GetWidget()->Close();
+  switch (button->id()) {
+    case BUTTON_RESET_CERTIFICATE_DECISIONS:
+      presenter_->OnRevokeSSLErrorBypassButtonPressed();
+      GetWidget()->Close();
+      break;
+    case BUTTON_SITE_SETTINGS:
+      // TODO(palmer): This opens the general Content Settings pane, which is OK
+      // for now. But on Android, it opens a page specific to a given origin
+      // that

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

Fix line wrapping.

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

Done.

+      // shows all of the settings for that origin. If/when that's available on
+      // desktop we should link to that here, too.
+      web_contents()->OpenURL(content::OpenURLParams(
+          GURL(chrome::kChromeUIContentSettingsURL), content::Referrer(),
+          WindowOpenDisposition::NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
+          false));
+      presenter_->RecordWebsiteSettingsAction(
+          WebsiteSettings::WEBSITE_SETTINGS_SITE_SETTINGS_OPENED);
+      break;
+    default:
+      NOTREACHED();
+  }
 }
 
 void WebsiteSettingsPopupView::LinkClicked(views::Link* source,
@@ -563,14 +586,11 @@ gfx::Size WebsiteSettingsPopupView::GetPreferredSize() const {
   if (site_settings_view_)
     height += site_settings_view_->GetPreferredSize().height();
 
-  int width = kSiteSettingsViewContentMinWidth;
-  if (site_data_content_)
-    width = std::max(width, site_data_content_->GetPreferredSize().width());
-  if (permissions_content_)
-    width = std::max(width, permissions_content_->GetPreferredSize().width());
+  int width = kMinPopupWidth;
   if (header_)
     width = std::max(width, header_->GetPreferredNameWidth());

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

Did this rely on the addition of padding values?
  width += kSiteSettingsViewPaddingLeft + kSiteSettingsViewPaddingRight;
If so, a long string here might not fit when the padding is applied.

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

Indeed, yes. But now that you've prompted me to look into it, there are two
issues:

This line "should" be width = std::max(width, header_->GetPreferredNameWidth())
+ 2*kSectionPaddingHorizontal;

However, the grid cell for the label actually needs to keep a distance of
kHeaderPaddingForCloseButton + (width of close button) from the right.
In addition, the text seems to use ellipses instead of wrapping when it's too
wide.

I believe the right choice is actually to:
- Set `summary_label_->SetMultiLine(true);`
- Not take into account the label for width sizing.

I'm double-checking with the designer that this is okay, but I think it's a safe
choice.

-  width += kSiteSettingsViewPaddingLeft + kSiteSettingsViewPaddingRight;
+  if (site_settings_view_)
+    width = std::max(width, site_settings_view_->GetPreferredSize().width());
   width = std::min(width, kMaxPopupWidth);
   return gfx::Size(width, height);
 }
@@ -580,50 +600,38 @@ void WebsiteSettingsPopupView::SetCookieInfo(
   // |cookie_info_list| should only ever have 2 items: first- and third-party
   // cookies.
   DCHECK_EQ(cookie_info_list.size(), 2u);
-  base::string16 first_party_label_text;
-  base::string16 third_party_label_text;
+  base::string16 label_text;
+  int total_allowed = 0;

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

nit: can you just do something like:
  int allowed = cookie_info_list[0].allowed + cookie_info_list[1].allowed;?

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

I could, but that would be inconsistent with the (identical) Mac code.
Which of this and the Mac code would you prefer to update?

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

This is fine as-is

   for (const auto& i : cookie_info_list) {

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

nit: curlies not needed; s/i/cookie/

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

Done.

-    if (i.is_first_party) {
-      first_party_label_text = l10n_util::GetPluralStringFUTF16(
-          IDS_WEBSITE_SETTINGS_FIRST_PARTY_SITE_DATA, i.allowed);

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

Remove string resources that are no longer used.

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

Done.

-    } else {
-      third_party_label_text = l10n_util::GetPluralStringFUTF16(
-          IDS_WEBSITE_SETTINGS_THIRD_PARTY_SITE_DATA, i.allowed);

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

Remove string resources that are no longer used.

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

Done.

-    }
+    total_allowed += i.allowed;
   }
+  label_text = l10n_util::GetPluralStringFUTF16(

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

nit: combine the declaration and assignment of |label_text| in one statement.

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

Done.

+      IDS_WEBSITE_SETTINGS_NUM_COOKIES, total_allowed);
 
   if (!cookie_dialog_link_) {
-    cookie_dialog_link_ = new views::Link(first_party_label_text);
+    cookie_dialog_link_ = new views::Link(label_text);
     cookie_dialog_link_->set_listener(this);
   } else {
-    cookie_dialog_link_->SetText(first_party_label_text);
+    cookie_dialog_link_->SetText(label_text);
   }
 
   views::GridLayout* layout =
-      static_cast<views::GridLayout*>(site_data_content_->GetLayoutManager());
+      static_cast<views::GridLayout*>(cookies_view_->GetLayoutManager());
   if (!layout) {
-    layout = new views::GridLayout(site_data_content_);
-    site_data_content_->SetLayoutManager(layout);
-
-    const int site_data_content_column = 0;
-    views::ColumnSet* column_set =
-        layout->AddColumnSet(site_data_content_column);
-    column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                          views::GridLayout::FIXED, kSiteDataIconColumnWidth,
-                          0);
-    column_set->AddPaddingColumn(0, kIconMarginLeft);
-    column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                          views::GridLayout::USE_PREF, 0, 0);
-    // No padding. This third column is for |third_party_label_text| (see
-    // below),
-    // and the text needs to flow naturally from the |first_party_label_text|
-    // link.
-    column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                          views::GridLayout::USE_PREF, 0, 0);
-
-    layout->AddPaddingRow(1, 5);
-
-    layout->StartRow(1, site_data_content_column);
+    layout = new views::GridLayout(cookies_view_);
+    cookies_view_->SetLayoutManager(layout);
+
+    const int cookies_view_column = 0;
+    views::ColumnSet* column_set = layout->AddColumnSet(cookies_view_column);
+    column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
+                          views::GridLayout::FIXED, kPermissionImageSize, 0);
+    column_set->AddPaddingColumn(0, kPermissionImageSpacing);
+    column_set->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL,
+                          0, views::GridLayout::USE_PREF, 0, 0);
+
+    layout->AddPaddingRow(0, kCookiesViewVerticalPadding);
+
+    layout->StartRow(1, cookies_view_column);
     WebsiteSettingsUI::PermissionInfo info;
     info.type = CONTENT_SETTINGS_TYPE_COOKIES;
     info.setting = CONTENT_SETTING_ALLOW;
@@ -633,19 +641,27 @@ void WebsiteSettingsPopupView::SetCookieInfo(
     views::ImageView* icon = new views::ImageView();
     const gfx::Image& image = WebsiteSettingsUI::GetPermissionIcon(info);
     icon->SetImage(image.ToImageSkia());
-    layout->AddView(icon, 1, 1, views::GridLayout::CENTER,
-                    views::GridLayout::CENTER);
-    layout->AddView(cookie_dialog_link_, 1, 1, views::GridLayout::CENTER,
-                    views::GridLayout::CENTER);
-    base::string16 comma = base::ASCIIToUTF16(", ");
+    layout->AddView(
+        icon, 1, 2, views::GridLayout::FILL,
+        // TODO: The vertical alignment may change to CENTER once Harmony is
+        // implemented. See https://crbug.com/512442#c48
+        views::GridLayout::LEADING);
 
-    layout->AddView(new views::Label(comma + third_party_label_text), 1, 1,
-                    views::GridLayout::LEADING, views::GridLayout::CENTER);
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    const gfx::FontList& font_list = rb.GetFontListWithDelta(1);

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

Ditto

+    views::Label* cookies_label = new views::Label(
+        l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_TITLE_SITE_DATA),
+        font_list);
+    layout->AddView(cookies_label);
+    layout->StartRow(1, cookies_view_column);
+    layout->SkipColumns(1);
+
+    layout->AddView(cookie_dialog_link_);
 
-    layout->AddPaddingRow(1, 6);
+    layout->AddPaddingRow(0, kCookiesViewVerticalPadding);
   }
 
-  layout->Layout(site_data_content_);
+  layout->Layout(cookies_view_);
   SizeToContents();
 }
 
@@ -657,70 +673,67 @@ void WebsiteSettingsPopupView::SetPermissionInfo(
   // will have already updated its state, so it's already reflected in the UI.
   // In addition, if a permission is set to the default setting, WebsiteSettings
   // removes it from |permission_info_list|, but the button should remain.
-  if (permissions_content_) {
+  if (permissions_view_) {
     base::STLDeleteContainerPointers(chosen_object_info_list.begin(),
                                      chosen_object_info_list.end());
     return;
   }
 
-  permissions_content_ = new views::View();
-  views::GridLayout* layout = new views::GridLayout(permissions_content_);
-  permissions_content_->SetLayoutManager(layout);
+  permissions_view_ = new views::View();
+  views::GridLayout* layout = new views::GridLayout(permissions_view_);
+  permissions_view_->SetLayoutManager(layout);
+  site_settings_view_->AddChildView(permissions_view_);
 
-  base::string16 headline =
-      permission_info_list.empty()
-          ? base::string16()
-          : l10n_util::GetStringUTF16(
-                IDS_WEBSITE_SETTINGS_TITLE_SITE_PERMISSIONS);
-  views::View* permissions_section =
-      CreateSection(headline, permissions_content_, nullptr);
-  site_settings_view_->AddChildView(permissions_section);
+  const int column_set_id = 0;
+  views::ColumnSet* column_set = layout->AddColumnSet(column_set_id);
+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
+                        views::GridLayout::FIXED, kPermissionImageSize, 0);
+  column_set->AddPaddingColumn(0, kPermissionImageSpacing);
+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                        views::GridLayout::USE_PREF, 0, 0);
 
-  const int content_column = 0;
-  views::ColumnSet* column_set = layout->AddColumnSet(content_column);
-  column_set->AddColumn(views::GridLayout::FILL,
-                        views::GridLayout::FILL,
-                        1,
-                        views::GridLayout::USE_PREF,
-                        0,
-                        0);
   for (const auto& permission : permission_info_list) {
-    layout->StartRow(1, content_column);
+    layout->StartRow(1, column_set_id);
     PermissionSelectorRow* selector = new PermissionSelectorRow(
         web_contents() ? web_contents()->GetVisibleURL() : GURL::EmptyGURL(),
-        permission);
+        permission, layout);
     selector->AddObserver(this);
-    layout->AddView(selector,
-                    1,
-                    1,
-                    views::GridLayout::LEADING,
-                    views::GridLayout::CENTER);
-    layout->AddPaddingRow(1, kContentRowSpacing);
+    layout->AddPaddingRow(1, kPermissionsVerticalSpacing);
   }
 
-  for (auto* object : chosen_object_info_list) {
-    layout->StartRow(1, content_column);
-    // The view takes ownership of the object info.
-    auto* object_view = new ChosenObjectRow(base::WrapUnique(object));
-    object_view->AddObserver(this);
-    layout->AddView(object_view, 1, 1, views::GridLayout::LEADING,
-                    views::GridLayout::CENTER);
-    layout->AddPaddingRow(1, kContentRowSpacing);
+  if (chosen_object_info_list.size() > 0) {

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

nit: This seems unnecessary, given the loop below; if not, use !empty.

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

Hmm, this seems to be from an old version of the code before refactoring, but I
can't find why. Removed.

+    for (auto* object : chosen_object_info_list) {
+      layout->StartRow(1, column_set_id);
+      // layout->SkipColumns(1);

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

Remove commented-out code; please do a close self-review first.

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

Done, sorry about that (I actually did several passes, I promise :-( ).

+      // The view takes ownership of the object info.
+      auto object_view = new ChosenObjectRow(base::WrapUnique(object), layout);
+      object_view->AddObserver(this);
+      // layout->AddView(object_view, 1, 2, views::GridLayout::LEADING,

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

Remove commented-out code; please do a close self-review first.

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

Done.

+      //                 views::GridLayout::CENTER);
+      layout->AddPaddingRow(1, kPermissionsVerticalSpacing);
+    }
   }
 
-  layout->Layout(permissions_content_);
+  layout->Layout(permissions_view_);
 
   // Add site settings link.
-  site_settings_link_ = new views::Link(
-      l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_SETTINGS_LINK));
-  site_settings_link_->set_listener(this);
-  views::View* link_section = new views::View();
-  const int kLinkMarginTop = 4;
-  link_section->SetLayoutManager(
-      new views::BoxLayout(views::BoxLayout::kHorizontal,
-                           kSiteSettingsViewPaddingLeft, kLinkMarginTop, 0));
-  link_section->AddChildView(site_settings_link_);
-  site_settings_view_->AddChildView(link_section);
+  views::LabelButton* site_settings_button_ =

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

Does this conflict with a member identifier? Don't use underscores post-fix for
local variables. Remove the |site_settings_button_| class member if it's unused.

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

It used to be a member, but should not be anymore. I've removed the member
variable and removed the underscore.

+      views::MdTextButton::CreateSecondaryUiButton(
+          this, l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_SETTINGS_LINK));
+  site_settings_button_->set_id(BUTTON_SITE_SETTINGS);
+  views::View* button_wrapper = new views::View();
+  views::BoxLayout* box_layout =
+      new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
+  if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {

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

nit:
box_layout->set_main_axis_alignment(ui::MaterialDesignController::IsSecondaryUiMaterial()
? views::BoxLayout::MAIN_AXIS_ALIGNMENT_END :
views::BoxLayout::MAIN_AXIS_ALIGNMENT_START);

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

Done.

+    box_layout->set_main_axis_alignment(
+        views::BoxLayout::MAIN_AXIS_ALIGNMENT_END);
+  } else {
+    box_layout->set_main_axis_alignment(
+        views::BoxLayout::MAIN_AXIS_ALIGNMENT_START);
+  }
+  button_wrapper->SetLayoutManager(box_layout);
+  button_wrapper->AddChildView(site_settings_button_);
+  site_settings_view_->AddChildView(button_wrapper);
 
   SizeToContents();
 }
@@ -752,87 +765,30 @@ void WebsiteSettingsPopupView::SetSelectedTab(TabId tab_id) {
 }
 
 views::View* WebsiteSettingsPopupView::CreateSiteSettingsView() {
-  views::View* pane = new views::View();
-  pane->SetLayoutManager(
-      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1));
-
-  // Add cookies and site data section.
-  site_data_content_ = new views::View();
-  views::View* site_data_section = CreateSection(
-      l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_TITLE_SITE_DATA),
-      site_data_content_, nullptr);
-  pane->AddChildView(site_data_section);
-
-  return pane;
+  views::View* site_settings_view = new views::View();
+  views::BoxLayout* box_layout =
+      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0);
+  site_settings_view->SetLayoutManager(box_layout);
+  box_layout->set_cross_axis_alignment(
+      views::BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH);
+
+  // Add cookies view.
+  cookies_view_ = new views::View();
+  box_layout->set_inside_border_insets(

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

nit: move above with the other |box_layout| init (set_cross_axis_alignment)

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

Done.

+      gfx::Insets(0, kSectionPaddingHorizontal));
+  site_settings_view->AddChildView(cookies_view_);
+
+  return site_settings_view;
 }
-views::View* WebsiteSettingsPopupView::CreateSection(
-    const base::string16& headline_text,
-    views::View* content,
-    views::Link* link) {
-  views::View* container = new views::View();
-  views::GridLayout* layout = new views::GridLayout(container);
-  container->SetLayoutManager(layout);
-  const int content_column = 0;
-  views::ColumnSet* column_set = layout->AddColumnSet(content_column);
-  column_set->AddPaddingColumn(0, kSiteSettingsViewPaddingLeft);
-  column_set->AddColumn(views::GridLayout::FILL,
-                        views::GridLayout::FILL,
-                        1,
-                        views::GridLayout::USE_PREF,
-                        0,
-                        0);
-
-  if (headline_text.length() > 0) {
-    layout->AddPaddingRow(1, kSiteSettingsViewPaddingTop);
-    layout->StartRow(1, content_column);
-    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    views::Label* headline = new views::Label(
-        headline_text, rb.GetFontList(ui::ResourceBundle::BoldFont));
-    layout->AddView(headline, 1, 1, views::GridLayout::LEADING,
-                    views::GridLayout::CENTER);
-  }
-
-  layout->AddPaddingRow(1, kSiteSettingsViewHeadlineMarginBottom);
-  layout->StartRow(1, content_column);
-  layout->AddView(content, 1, 1, views::GridLayout::LEADING,
-                  views::GridLayout::CENTER);
-
-  if (link) {
-    layout->AddPaddingRow(1, 4);
-    layout->StartRow(1, content_column);
-    layout->AddView(link, 1, 1, views::GridLayout::LEADING,
-                    views::GridLayout::CENTER);
-  }
-
-  layout->AddPaddingRow(1, kSiteSettingsViewPaddingBottom);
-  return container;
-}
-
 
 void WebsiteSettingsPopupView::HandleLinkClickedAsync(views::Link* source) {
   if (web_contents() == nullptr || web_contents()->IsBeingDestroyed())
     return;
-
-  if (source == cookie_dialog_link_) {
-    // Count how often the Collected Cookies dialog is opened.
-    presenter_->RecordWebsiteSettingsAction(
-        WebsiteSettings::WEBSITE_SETTINGS_COOKIES_DIALOG_OPENED);
-
-    new CollectedCookiesViews(web_contents());
-  } else if (source == site_settings_link_) {
-    // TODO(palmer): This opens the general Content Settings pane, which is OK
-    // for now. But on Android, it opens a page specific to a given origin that
-    // shows all of the settings for that origin. If/when that's available on
-    // desktop we should link to that here, too.
-    web_contents()->OpenURL(content::OpenURLParams(
-        GURL(chrome::kChromeUIContentSettingsURL), content::Referrer(),
-        WindowOpenDisposition::NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
-        false));
-    presenter_->RecordWebsiteSettingsAction(
-        WebsiteSettings::WEBSITE_SETTINGS_SITE_SETTINGS_OPENED);
-  } else {
-    NOTREACHED();
-  }
+  DCHECK_EQ(source, cookie_dialog_link_);
+  // Count how often the Collected Cookies dialog is opened.
+  presenter_->RecordWebsiteSettingsAction(
+      WebsiteSettings::WEBSITE_SETTINGS_COOKIES_DIALOG_OPENED);
+  new CollectedCookiesViews(web_contents());
 }
 
 void WebsiteSettingsPopupView::StyledLabelLinkClicked(views::StyledLabel* label,