Material Page Info (Views, 2/3): Update security 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 30 matching lines @@
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_list.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/image/image.h"
 #include "ui/resources/grit/ui_resources.h"
+#include "ui/views/border.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/md_text_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/styled_label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_manager.h"
@@ skipping 13 matching lines @@
 
 // Left icon margin.
 const int kIconMarginLeft = 6;
 
 // Margin and padding values for the |PopupHeaderView|.
 const int kHeaderMarginBottom = 10;
 const int kHeaderPaddingBottom = 16;
 const int kHeaderPaddingLeft = 18;
 const int kHeaderPaddingRightForCloseButton = 8;
 const int kHeaderPaddingRightForText = kHeaderPaddingLeft;
-const int kHeaderPaddingTop = 12;
+const int kHeaderPaddingTop = 16;
 
 // Spacing between the site identity label and the site identity status text in
 // the popup header.
 const int kHeaderRowSpacing = 4;
 
 // The max possible width of the popup.
 const int kMaxPopupWidth = 1000;
 
 // The margins between the popup border and the popup content.
-const int kPopupMarginTop = 4;
-const int kPopupMarginLeft = 0;
 const int kPopupMarginBottom = 14;
-const int kPopupMarginRight = 0;
 
 // 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;
 
 // 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;
 
 const int kSiteDataIconColumnWidth = 20;
 
 const int BUTTON_RESET_CERTIFICATE_DECISIONS = 1337;
 
 }  // 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);
   ~PopupHeaderView() override;
 
-  // Sets the name of the site's identity.
-  void SetIdentityName(const base::string16& name);
+  // Sets security summary.

[msw, 2016/08/25 01:30:21]

nit: // Sets the security summary for the current page.
(to be consistent with the comment below)

[lgarron, 2016/08/25 03:26:58]

Done.

+  void SetSummary(const base::string16& text);
+  void SetSummaryColor(SkColor color);
 
-  // Sets the security summary text for the current page.
-  void SetSecuritySummary(const base::string16& security_summary_text,
-                          bool include_details_link);
+  // Sets the security details for the current page.
+  void SetDetails(const base::string16& security_summary_text,

[msw, 2016/08/25 01:30:22]

nit: It's a little confusing that we support SetSummary(text) and
SetDetails(security_summary_text, bool), even though these are two separate
'summary' text blobs. Would it make sense to call the first 'Summary' a 'Title'
or similar? Or rename |security_summary_text| to |security_details_text| or
similar?

[lgarron, 2016/08/25 03:26:58]

I've changed the parameter to security_details_text, which is the right name for
it.

+                  bool include_details_link);
 
   int GetPreferredNameWidth() const;
 
   void AddResetDecisionsButton();
 
  private:
+  // The listener for the styled lables in this view.

[msw, 2016/08/25 01:30:22]

nit: 'labels'

[lgarron, 2016/08/25 03:26:58]

Done.

+  views::StyledLabelListener* styled_label_listener_;
+
   // The label that displays the name of the site's identity.

[msw, 2016/08/25 01:30:22]

nit: update comment.

[lgarron, 2016/08/25 03:26:57]

Done.

-  views::Label* name_;
+  views::Label* summary_;
+
   // The label that displays the status of the identity check for this site.
   // Includes a link to open the DevTools Security panel.
   views::StyledLabel* status_;
 
-  // The button listener attached to the buttons in this view.
-  views::ButtonListener* button_listener_;
-
   // A container for the button for resetting cert decisions. The button is only

[msw, 2016/08/25 01:30:21]

nit: update mentions of button, now it's a "styled label with a link"

[lgarron, 2016/08/25 03:26:57]

Done.

   // shown sometimes, so we use a container to keep track of where to place it
   // (if needed).
-  views::View* reset_decisions_button_container_;
+  views::View* reset_decisions_label_container_;
+  views::StyledLabel* reset_decisions_label_;
 
   DISALLOW_COPY_AND_ASSIGN(PopupHeaderView);
 };
 
 // Website Settings are not supported for internal Chrome pages and extension
 // pages. Instead of the |WebsiteSettingsPopupView|, the
 // |InternalPageInfoPopupView| is displayed.
 class InternalPageInfoPopupView : public views::BubbleDialogDelegateView {
  public:
   // If |anchor_view| is nullptr, or has no Widget, |parent_window| may be
@@ skipping 15 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(InternalPageInfoPopupView);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 // Popup Header
 ////////////////////////////////////////////////////////////////////////////////
 
 PopupHeaderView::PopupHeaderView(
     views::ButtonListener* button_listener,
     views::StyledLabelListener* styled_label_listener)
-    : name_(nullptr),
+    : styled_label_listener_(styled_label_listener),
+      summary_(nullptr),
       status_(nullptr),
-      button_listener_(button_listener),
-      reset_decisions_button_container_(nullptr) {
+      reset_decisions_label_container_(nullptr) {

[msw, 2016/08/25 01:30:21]

nit: also init |reset_decisions_label_| to nullptr

[lgarron, 2016/08/25 03:26:57]

Done.

   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->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,
                         views::GridLayout::USE_PREF,
                         0,
                         0);
   column_set->AddPaddingColumn(0, kHeaderPaddingRightForCloseButton);
 
-  layout->AddPaddingRow(0, kHeaderPaddingTop);
+  // First we add the padding needed for the close button.
+  // In order to move down the summary, we simulate additional padding by giving
+  // it an empty border on top later on.
+  layout->AddPaddingRow(0, kHeaderPaddingRightForCloseButton);
+
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
 
   layout->StartRow(0, label_column);
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  name_ = new views::Label(
-      base::string16(), rb.GetFontList(ui::ResourceBundle::BoldFont));
-  layout->AddView(name_, 1, 1, views::GridLayout::LEADING,
+  const gfx::FontList& font_list = rb.GetFontListWithDelta(1);

[msw, 2016/08/25 01:30:22]

nit: do we really need this to be 1px bigger than the default?

[lgarron, 2016/08/25 03:26:58]

Yep, this is an explicit design design decision in the specs.

[msw, 2016/08/25 03:56:46]

Acknowledged.

+  summary_ = new views::Label(base::string16(), font_list);
+  summary_->SetBorder(views::Border::CreateEmptyBorder(
+      kHeaderPaddingTop - kHeaderPaddingRightForCloseButton, 0, 0, 0));
+  layout->AddView(summary_, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::TRAILING);
   views::ImageButton* close_button = new views::ImageButton(button_listener);

[msw, 2016/08/25 01:30:22]

aside: It'd be nice if this bubble simply used the standard close button (see
BubbleDialogDelegateView::ShouldShowCloseButton), but that might not be
possible, and certainly isn't a blocker for this CL.

[lgarron, 2016/08/25 03:26:58]

Bug filed: https://crbug.com/640851

   close_button->SetImage(views::CustomButton::STATE_NORMAL,
                          rb.GetImageNamed(IDR_CLOSE_2).ToImageSkia());
   close_button->SetImage(views::CustomButton::STATE_HOVERED,
                          rb.GetImageNamed(IDR_CLOSE_2_H).ToImageSkia());
   close_button->SetImage(views::CustomButton::STATE_PRESSED,
                          rb.GetImageNamed(IDR_CLOSE_2_P).ToImageSkia());
-  layout->AddView(close_button, 1, 1, views::GridLayout::TRAILING,
+  layout->AddView(close_button, 1, 2, views::GridLayout::TRAILING,

[msw, 2016/08/25 01:30:22]

Is this now spanning two rows to vertically center the close button over the
summary and the padding row above or below the summary? I'm not sure why we
would need either, especially if we are adding a row above with padding
specifically designed for the close button (kHeaderPaddingRightForCloseButton);
can you explain?

[lgarron, 2016/08/25 03:26:58]

Good catch, this is a vestigial fix.

I was originally planning to have the close button span a spacing row and a row
with the summary, but apparently you can't add a padding row without `clear:
both`ing the existing rows. I ended up using a border on the summary to fix
this, so I've gone ahead and changes this back to 1, 1.

                   views::GridLayout::LEADING);
 
   layout->AddPaddingRow(0, kHeaderRowSpacing);
 
   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->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL,
                                1, views::GridLayout::USE_PREF, 0, 0);
   column_set_status->AddPaddingColumn(0, kHeaderPaddingRightForText);
 
+  layout->AddPaddingRow(0, kHeaderRowSpacing);
+
   layout->StartRow(0, label_column_status);
   status_ = new views::StyledLabel(base::string16(), styled_label_listener);
   layout->AddView(status_,
                   1,
                   1,
                   views::GridLayout::LEADING,
                   views::GridLayout::LEADING);
 
   layout->StartRow(0, label_column_status);
-  reset_decisions_button_container_ = new views::View();
-  reset_decisions_button_container_->SetLayoutManager(
+  reset_decisions_label_container_ = new views::View();
+  reset_decisions_label_container_->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));

[msw, 2016/08/25 01:30:22]

aside: perhaps this could be a FillLayout (but it's not really important here).

[lgarron, 2016/08/25 03:26:58]

Just tried, but that resizes the label to be the full width of the bubble, which
centers the label. :-(
Gonna stick with BoxLayout for now; that will also easily support moving the
button to the right, which we will want for Material design soon.

[msw, 2016/08/25 03:56:46]

Acknowledged.

-  layout->AddView(reset_decisions_button_container_, 1, 1,
+  layout->AddView(reset_decisions_label_container_, 1, 1,
                   views::GridLayout::LEADING, views::GridLayout::LEADING);
 
   layout->AddPaddingRow(1, kHeaderPaddingBottom);
 }
 
 PopupHeaderView::~PopupHeaderView() {}
 
 int PopupHeaderView::GetPreferredNameWidth() const {
-  return name_->GetPreferredSize().width();
+  return summary_->GetPreferredSize().width();
 }
 
-void PopupHeaderView::SetIdentityName(const base::string16& name) {
-  name_->SetText(name);
+void PopupHeaderView::SetSummary(const base::string16& text) {
+  summary_->SetText(text);
 }
 
-void PopupHeaderView::SetSecuritySummary(
-    const base::string16& security_summary_text,
-    bool include_details_link) {
+void PopupHeaderView::SetSummaryColor(SkColor color) {
+  summary_->SetEnabledColor(color);
+}
+
+void PopupHeaderView::SetDetails(const base::string16& security_summary_text,
+                                 bool include_details_link) {
   if (include_details_link) {
     base::string16 details_string =
         l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_DETAILS_LINK);
 
     std::vector<base::string16> subst;
     subst.push_back(security_summary_text);
     subst.push_back(details_string);
 
     std::vector<size_t> offsets;
 
@@ skipping 11 matching lines @@
     status_->AddStyleRange(details_range, link_style);
   } else {
     status_->SetText(security_summary_text);
   }
 
   // Fit the styled label to occupy available width.
   status_->SizeToFit(0);
 }
 
 void PopupHeaderView::AddResetDecisionsButton() {
-  // TODO(estade): this looks pretty crazy as an MD button because the button
-  // text is very long. See crbug.com/512442
-  views::LabelButton* reset_decisions_button =
-      views::MdTextButton::CreateSecondaryUiButton(
-          button_listener_,
-          l10n_util::GetStringUTF16(
-              IDS_PAGEINFO_RESET_INVALID_CERTIFICATE_DECISIONS_BUTTON));
-  reset_decisions_button->set_id(BUTTON_RESET_CERTIFICATE_DECISIONS);
+  std::vector<base::string16> subst;
+  subst.push_back(
+      l10n_util::GetStringUTF16(IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION));

[msw, 2016/08/25 01:30:21]

Please note CL dependencies in the CL description. I was confused by this until
I saw your newer comment about https://codereview.chromium.org/2262223002

[lgarron, 2016/08/25 03:26:57]

Hmm, I thought the convention was only to reference git commits rather than
transient info, but I guess it doesn't hurt. Added.

[msw, 2016/08/25 03:56:46]

This is just to make reviewing easier. I was reviewing this patch set and
couldn't find either IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION nor
WebsiteSettingsUI::SecurityDescription in codesearch, so I wasn't sure if you
forgot to make those changes in this CL, and I'd rather not dig through your
open CLs, git history, etc. just to find prerequisite changes. Your comment
referencing the dependency came just before I was about to send comments asking
about these missing pieces. There's some 'git cl' command for marking dependent
CLs, but i'm not familiar with it, I just typically call them out in
comments/description/message.

[lgarron, 2016/08/25 04:10:14]

Good point. Acknowledged. :-)

+  subst.push_back(l10n_util::GetStringUTF16(
+      IDS_PAGEINFO_RESET_INVALID_CERTIFICATE_DECISIONS_BUTTON));
 
-  reset_decisions_button_container_->AddChildView(reset_decisions_button);
+  std::vector<size_t> offsets;
+
+  base::string16 text = base::ReplaceStringPlaceholders(
+      base::ASCIIToUTF16("$1 $2"), subst, &offsets);

[msw, 2016/08/25 01:30:22]

This is not the right way to combine localized strings; you should structure the
two IDS strings such that one has a placeholder for the other, and then use
GetStringFUTF16, so translations can differ in how they merge the two.

[lgarron, 2016/08/25 03:26:58]

Unfortunately, OSX needs to use separate strings for the label text and its
link.
This pattern avoids having two translations (one with a placeholder for Views,
and one without for Mac).

This was also one of the reasons that led to the first implementation of this
pattern (in the preceding function). After an email thread and some discussion,
we settled on this.

[msw, 2016/08/25 03:56:46]

Acknowledged.

+  reset_decisions_label_ = new views::StyledLabel(text, styled_label_listener_);
+  gfx::Range link_range(offsets[1], text.length());
+
+  views::StyledLabel::RangeStyleInfo link_style =
+      views::StyledLabel::RangeStyleInfo::CreateForLink();
+  if (!ui::MaterialDesignController::IsSecondaryUiMaterial())
+    link_style.font_style |= gfx::Font::FontStyle::UNDERLINE;
+  link_style.disable_line_wrapping = false;
+
+  reset_decisions_label_->AddStyleRange(link_range, link_style);
+  reset_decisions_label_->SizeToFit(0);

[msw, 2016/08/25 01:30:22]

q: Is this needed?

[lgarron, 2016/08/25 03:26:57]

Yes. I think we discussed it on a previous CL. The view is not properly sized if
we don't call this.

[msw, 2016/08/25 03:56:46]

Right... Can you add a similar comment:
// Fit the styled label to occupy available width.

[lgarron, 2016/08/25 04:10:14]

Done.

+  reset_decisions_label_container_->AddChildView(reset_decisions_label_);
 
   // Now that it contains a button, the container needs padding at the top.

[msw, 2016/08/25 01:30:22]

nit: update mentions of button.

-  reset_decisions_button_container_->SetBorder(
+  reset_decisions_label_container_->SetBorder(
       views::Border::CreateEmptyBorder(8, 0, 0, 0));
 
   InvalidateLayout();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // InternalPageInfoPopupView
 ////////////////////////////////////////////////////////////////////////////////
 
 InternalPageInfoPopupView::InternalPageInfoPopupView(
@@ skipping 146 matching lines @@
   layout->StartRow(0, content_column);
   separator_ = new views::Separator(views::Separator::HORIZONTAL);
   layout->AddView(separator_);
 
   layout->AddPaddingRow(1, kHeaderMarginBottom);
   layout->StartRow(1, content_column);
 
   site_settings_view_ = CreateSiteSettingsView();
   layout->AddView(site_settings_view_);
 
-  set_margins(gfx::Insets(kPopupMarginTop, kPopupMarginLeft,
-                          kPopupMarginBottom, kPopupMarginRight));
+  set_margins(gfx::Insets(0, 0, kPopupMarginBottom,
+                          0));  // Each section handles its own padding.

[msw, 2016/08/25 01:30:22]

nit: put the comment before the code and avoid wrapping the statement.

[lgarron, 2016/08/25 03:26:57]

Done.

 
   views::BubbleDialogDelegateView::CreateBubble(this);
 
   presenter_.reset(new WebsiteSettings(
       this, profile, TabSpecificContentSettings::FromWebContents(web_contents),
       web_contents, url, security_info, content::CertStore::GetInstance()));
 }
 
 void WebsiteSettingsPopupView::RenderFrameDeleted(
     content::RenderFrameHost* render_frame_host) {
@@ skipping 147 matching lines @@
   // 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_) {
     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_);
+  layout->set_minimum_size(gfx::Size(300, 0));

[msw, 2016/08/25 01:30:22]

Why is this needed here? If it's needed, use kSiteSettingsViewContentMinWidth.

[lgarron, 2016/08/25 03:26:57]

Removed.
(I think this is a merge artifact.)

   permissions_content_->SetLayoutManager(layout);
 
   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);
@@ skipping 42 matching lines @@
       new views::BoxLayout(views::BoxLayout::kHorizontal,
                            kSiteSettingsViewPaddingLeft, kLinkMarginTop, 0));
   link_section->AddChildView(site_settings_link_);
   site_settings_view_->AddChildView(link_section);
 
   SizeToContents();
 }
 
 void WebsiteSettingsPopupView::SetIdentityInfo(
     const IdentityInfo& identity_info) {
-  base::string16 security_summary_text = identity_info.GetSecuritySummary();
-  header_->SetIdentityName(base::UTF8ToUTF16(identity_info.site_identity));
+  std::unique_ptr<WebsiteSettingsUI::SecurityDescription> security_description =
+      identity_info.GetSecurityDescription();
+
+  header_->SetSummary(security_description->summary);
+  if (security_description->summary_style & WebsiteSettingsUI::STYLE_COLOR) {

[msw, 2016/08/25 01:30:22]

nit: curlies not needed.

[lgarron, 2016/08/25 03:26:57]

*sheds tear and removes curlies*

+    header_->SetSummaryColor(security_description->summary_color);

[msw, 2016/08/25 01:30:22]

It would be nice if we passed around security levels instead of colors, then
translate levels to colors, similar to the omnibox:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_...

[lgarron, 2016/08/25 03:26:58]

I strongly wanted to avoid hard-coding the colors in each platform separately.
There doesn't seem to be a central place to get Material colors, so this pattern
is the best I could find.

[msw, 2016/08/25 03:56:46]

The right way to do is usually defining native theme colors. I highly suggest
looking at which colors are used and how they are picked and modified for themes
and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume
these colors won't look very good in high-contrast inverted color themes, etc.

[lgarron, 2016/08/25 04:10:14]

In this case, the UI surface is not colored by the theme (Page Info always
white/a light background), and the colors for the summary are always the same
fixed green/red Material colors.
Is the current code acceptable for this case?

[msw, 2016/08/25 04:44:57]

Just tested; the bubble has a black background in Win7 "High Contrast Black".

+  }
 
   if (identity_info.cert_id) {
     cert_id_ = identity_info.cert_id;
 
     if (identity_info.show_ssl_decision_revoke_button)
       header_->AddResetDecisionsButton();
   }
 
   bool include_details_link =
       !is_devtools_disabled_ || identity_info.cert_id != 0;
 
-  header_->SetSecuritySummary(security_summary_text, include_details_link);
+  header_->SetDetails(security_description->details, include_details_link);
 
   Layout();
   SizeToContents();
 }
 
 void WebsiteSettingsPopupView::SetSelectedTab(TabId tab_id) {
   // TODO(lgarron): Remove this method. (https://crbug.com/571533)
 }
 
 views::View* WebsiteSettingsPopupView::CreateSiteSettingsView() {
@@ skipping 88 matching lines @@
     gfx::NativeWindow parent =
         anchor_widget() ? anchor_widget()->GetNativeWindow() : nullptr;
     presenter_->RecordWebsiteSettingsAction(
         WebsiteSettings::WEBSITE_SETTINGS_CERTIFICATE_DIALOG_OPENED);
     ShowCertificateViewerByID(web_contents_, parent, cert_id_);
   } else {
     DevToolsWindow::OpenDevToolsWindow(
         web_contents_, DevToolsToggleAction::ShowSecurityPanel());
   }
 }