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

chrome/browser/ui/views/website_settings/website_settings_popup_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/website_settings/website_settings_popup_view.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <vector>
@@ skipping 54 matching lines @@
 namespace {
 
 // NOTE(jdonnelly): This use of this process-wide variable assumes that there's
 // never more than one website settings popup shown and that it's associated
 // with the current window. If this assumption fails in the future, we'll need
 // to return a weak pointer from ShowPopup so callers can associate it with the
 // current window (or other context) and check if the popup they care about is
 // 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 ****************/
 
-// The margins between the popup border and the popup content.
-const int kPopupMarginBottom = 14;
+// Spacing before and after the cookies view.
+const int kCookiesViewVerticalPadding = 6;
 
-// 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;
+// 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

 
-// 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 a permission image and the text.
+const int kPermissionImageSpacing = 6;
 
-const int kSiteDataIconColumnWidth = 20;
+// Spacing between rows in the site settings section
+const int kPermissionsVerticalSpacing = 12;
+
+/**************** Button IDs ****************/
 
 const int BUTTON_RESET_CERTIFICATE_DECISIONS = 1337;
+const int BUTTON_SITE_SETTINGS = 1338;
 
 }  // namespace
 
 // |PopupHeaderView| is the UI element (view) that represents the header of the
 // |WebsiteSettingsPopupView|. The header shows the status of the site's
 // identity check and the name of the site's identity.
 class PopupHeaderView : public views::View {
  public:
   explicit PopupHeaderView(views::ButtonListener* button_listener,
                            views::StyledLabelListener* styled_label_listener);
@@ skipping 64 matching lines @@
     : styled_label_listener_(styled_label_listener),
       summary_label_(nullptr),
       details_label_(nullptr),
       reset_decisions_label_container_(nullptr),
       reset_decisions_label_(nullptr) {
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   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,
                         views::GridLayout::USE_PREF,
                         0,
                         0);
   column_set->AddPaddingColumn(1, 0);
   column_set->AddColumn(views::GridLayout::FILL,
                         views::GridLayout::FILL,
                         1,
@@ skipping 24 matching lines @@
   close_button->SetImage(views::CustomButton::STATE_PRESSED,
                          rb.GetImageNamed(IDR_CLOSE_2_P).ToImageSkia());
   layout->AddView(close_button, 1, 1, views::GridLayout::TRAILING,
                   views::GridLayout::LEADING);
 
   layout->AddPaddingRow(0, kHeaderLabelSpacing);
 
   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);
 
   layout->StartRow(0, label_column_status);
   details_label_ =
       new views::StyledLabel(base::string16(), styled_label_listener);
   layout->AddView(details_label_, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::LEADING);
 
   layout->StartRow(0, label_column_status);
@@ skipping 193 matching lines @@
     gfx::NativeView parent_window,
     Profile* profile,
     content::WebContents* web_contents,
     const GURL& url,
     const security_state::SecurityStateModel::SecurityInfo& security_info)
     : content::WebContentsObserver(web_contents),
       BubbleDialogDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT),
       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);
 
   is_devtools_disabled_ =
       profile->GetPrefs()->GetBoolean(prefs::kDevToolsDisabled);
 
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(
       GetLayoutConstant(LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET), 0));
 
@@ skipping 56 matching lines @@
   is_popup_showing = false;
   presenter_->OnUIClosing();
 }
 
 int WebsiteSettingsPopupView::GetDialogButtons() const {
   return ui::DIALOG_BUTTON_NONE;
 }
 
 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,
                                            int event_flags) {
   // The popup closes automatically when the collected cookies dialog or the
   // certificate viewer opens. So delay handling of the link clicked to avoid
   // a crash in the base class which needs to complete the mouse event handling.
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&WebsiteSettingsPopupView::HandleLinkClickedAsync,
                  weak_factory_.GetWeakPtr(), source));
 }
 
 gfx::Size WebsiteSettingsPopupView::GetPreferredSize() const {
   if (header_ == nullptr && site_settings_view_ == nullptr)
     return views::View::GetPreferredSize();
 
   int height = 0;
   if (header_)
     height += header_->GetPreferredSize().height() + kHeaderMarginBottom;
   if (separator_)
     height += separator_->GetPreferredSize().height();
 
   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);
 }
 
 void WebsiteSettingsPopupView::SetCookieInfo(
     const CookieInfoList& cookie_info_list) {
   // |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);
+    layout = new views::GridLayout(cookies_view_);
+    cookies_view_->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);
+    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(1, 5);
+    layout->AddPaddingRow(0, kCookiesViewVerticalPadding);
 
-    layout->StartRow(1, site_data_content_column);
+    layout->StartRow(1, cookies_view_column);
     WebsiteSettingsUI::PermissionInfo info;
     info.type = CONTENT_SETTINGS_TYPE_COOKIES;
     info.setting = CONTENT_SETTING_ALLOW;
     info.is_incognito =
         Profile::FromBrowserContext(web_contents()->GetBrowserContext())
             ->IsOffTheRecord();
     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->AddPaddingRow(1, 6);
+    layout->AddView(cookie_dialog_link_);
+
+    layout->AddPaddingRow(0, kCookiesViewVerticalPadding);
   }
 
-  layout->Layout(site_data_content_);
+  layout->Layout(cookies_view_);
   SizeToContents();
 }
 
 void WebsiteSettingsPopupView::SetPermissionInfo(
     const PermissionInfoList& permission_info_list,
     const ChosenObjectInfoList& chosen_object_info_list) {
   // When a permission is changed, WebsiteSettings::OnSitePermissionChanged()
   // calls this method with updated permissions. However, PermissionSelectorRow
   // 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();
 }
 
 void WebsiteSettingsPopupView::SetIdentityInfo(
     const IdentityInfo& identity_info) {
   std::unique_ptr<WebsiteSettingsUI::SecurityDescription> security_description =
       identity_info.GetSecurityDescription();
 
   header_->SetSummary(security_description->summary);
@@ skipping 11 matching lines @@
 
   Layout();
   SizeToContents();
 }
 
 void WebsiteSettingsPopupView::SetSelectedTab(TabId tab_id) {
   // TODO(lgarron): Remove this method. (https://crbug.com/571533)
 }
 
 views::View* WebsiteSettingsPopupView::CreateSiteSettingsView() {
-  views::View* pane = new views::View();
-  pane->SetLayoutManager(
-      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1));
+  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 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);
+  // 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 pane;
+  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,
                                                       const gfx::Range& range,
                                                       int event_flags) {
   presenter_->RecordWebsiteSettingsAction(
       WebsiteSettings::WEBSITE_SETTINGS_SECURITY_DETAILS_OPENED);
 
   if (is_devtools_disabled_) {
     DCHECK(certificate_);
     gfx::NativeWindow parent =
         anchor_widget() ? anchor_widget()->GetNativeWindow() : nullptr;
     presenter_->RecordWebsiteSettingsAction(
         WebsiteSettings::WEBSITE_SETTINGS_CERTIFICATE_DIALOG_OPENED);
     ShowCertificateViewer(web_contents(), parent, certificate_.get());
   } else {
     DevToolsWindow::OpenDevToolsWindow(
         web_contents(), DevToolsToggleAction::ShowSecurityPanel());
   }
 }