Material Page Info (Views, 2/3): Update security 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 18e209d8ccf0de77a6f357f7ef7e5a06cd133df9..4886cf75a115bb7548df7fb8954a36c7598df6a8 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
@@ -48,6 +48,7 @@
 #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"
@@ -81,7 +82,7 @@ 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.
@@ -91,10 +92,7 @@ const int kHeaderRowSpacing = 4;
 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;
@@ -124,31 +122,34 @@ class PopupHeaderView : public views::View {
                            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);
 };
@@ -184,10 +185,10 @@ class InternalPageInfoPopupView : public views::BubbleDialogDelegateView {
 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);
 
@@ -209,13 +210,19 @@ PopupHeaderView::PopupHeaderView(
                         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);
 
-  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,
+
+  layout->StartRow(0, label_column);
+  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,
@@ -224,7 +231,7 @@ PopupHeaderView::PopupHeaderView(
                          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);
@@ -237,6 +244,8 @@ PopupHeaderView::PopupHeaderView(
                                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_,
@@ -246,10 +255,10 @@ PopupHeaderView::PopupHeaderView(
                   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);
@@ -258,16 +267,19 @@ PopupHeaderView::PopupHeaderView(
 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);
@@ -299,19 +311,31 @@ void PopupHeaderView::SetSecuritySummary(
 }
 
 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));
+
+  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());
 
-  reset_decisions_button_container_->AddChildView(reset_decisions_button);
+  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();
@@ -478,8 +502,8 @@ WebsiteSettingsPopupView::WebsiteSettingsPopupView(
   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);
 
@@ -647,6 +671,7 @@ void WebsiteSettingsPopupView::SetPermissionInfo(
 
   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 =
@@ -709,8 +734,13 @@ void WebsiteSettingsPopupView::SetPermissionInfo(
 
 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;
@@ -722,7 +752,7 @@ void WebsiteSettingsPopupView::SetIdentityInfo(
   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();