Show HTTP-Bad security chip UI on Mac and Views

chrome/browser/ui/views/location_bar/location_bar_view.cc

Index: chrome/browser/ui/views/location_bar/location_bar_view.cc
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
index 38c7c2eed4e6543dd38a0f97badb21c5cea3d7a3..b9bdfd863614dd54fb11fee0a9f0e4419af7fccf 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -825,7 +825,8 @@ void LocationBarView::RefreshLocationIcon() {
   security_state::SecurityStateModel::SecurityLevel security_level =
       GetToolbarModel()->GetSecurityLevel(false);
   SkColor icon_color =
-      (security_level == security_state::SecurityStateModel::NONE)
+      (security_level == security_state::SecurityStateModel::NONE ||
+       security_level == security_state::SecurityStateModel::HTTP_SHOW_WARNING)

[Peter Kasting, 2016/10/17 20:56:39]

Hmm, should SHOW_WARNING really use the text color, and not a security text
color (which would presumably be red)?

[felt, 2016/10/17 22:56:58]

That's correct. It intentionally is set to show as the text color and not as
red, as per specs. It's meant to be a mild warning.

           ? color_utils::DeriveDefaultIconColor(GetColor(TEXT))
           : GetSecureTextColor(security_level);
   location_icon_view_->SetImage(gfx::CreateVectorIcon(
@@ -1018,7 +1019,8 @@ bool LocationBarView::ShouldShowSecurityChip() const {
   return ((level == SecurityLevel::SECURE ||
            level == SecurityLevel::EV_SECURE) &&
           should_show_secure_state_) ||
-         level == SecurityLevel::DANGEROUS;
+         level == SecurityLevel::DANGEROUS ||
+         level == SecurityLevel::HTTP_SHOW_WARNING;

[Peter Kasting, 2016/10/17 20:56:39]

Nit: Shorter, and seems more readable:

  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
  const SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
  if (level == SecurityLevel::SECURE || level == SecurityLevel::EV_SECURE)
    return should_show_secure_state_;
  return level == SecurityLevel::DANGEROUS ||
         level == SecurityLevel::HTTP_SHOW_WARNING;

[felt, 2016/10/17 22:56:58]

Done.

 }
 
 bool LocationBarView::ShouldAnimateSecurityChip() const {
@@ -1027,7 +1029,8 @@ bool LocationBarView::ShouldAnimateSecurityChip() const {
   bool is_secure_level =
       level == SecurityLevel::EV_SECURE || level == SecurityLevel::SECURE;
   return ShouldShowSecurityChip() &&
-         ((level == SecurityLevel::DANGEROUS &&
+         (((level == SecurityLevel::DANGEROUS ||
+            level == SecurityLevel::HTTP_SHOW_WARNING) &&
            should_animate_nonsecure_state_) ||
           (is_secure_level && should_animate_secure_state_));

[Peter Kasting, 2016/10/17 20:56:39]

Nit: Feels more readable to me, and is parallel to my suggestion above:

  if (!ShouldShowSecurityChip())
    return false;

  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
  const SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
  if (level == SecurityLevel::EV_SECURE || level == SecurityLevel::SECURE)
    return should_animate_secure_state_;
  return should_animate_nonsecure_state_ &&
         (level == SecurityLevel::DANGEROUS ||
          level == SecurityLevel::HTTP_SHOW_WARNING);

You might not need the first conditional at all, depending on how callers call
these two functions.  (If "animate when showing" is orthogonal to "showing to
begin with", you can get away with not having it.)

[felt, 2016/10/17 22:56:58]

Done.

 }