Show HTTP-Bad security chip UI on Mac and Views

chrome/browser/ui/views/location_bar/location_bar_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/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/command_line.h"
@@ skipping 807 matching lines @@
 
 void LocationBarView::RefreshLocationIcon() {
   // |omnibox_view_| may not be ready yet if Init() has not been called. The
   // icon will be set soon by OnChanged().
   if (!omnibox_view_)
     return;
 
   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(
       omnibox_view_->GetVectorIcon(), kIconWidth, icon_color));
 }
 
 bool LocationBarView::RefreshContentSettingViews() {
   bool visibility_changed = false;
   for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
        i != content_setting_views_.end(); ++i) {
@@ skipping 172 matching lines @@
           security_state::SecurityStateModel::EV_SECURE) &&
          should_show_secure_state_;
 }
 
 bool LocationBarView::ShouldShowSecurityChip() const {
   using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
   SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
   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 {
   using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
   SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
   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.

 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private LocationBar implementation:
 
 void LocationBarView::ShowFirstRunBubble() {
   // Wait until search engines have loaded to show the first run bubble.
   TemplateURLService* url_service =
       TemplateURLServiceFactory::GetForProfile(profile());
   if (!url_service->loaded()) {
@@ skipping 297 matching lines @@
 // LocationBarView, private TemplateURLServiceObserver implementation:
 
 void LocationBarView::OnTemplateURLServiceChanged() {
   template_url_service_->RemoveObserver(this);
   template_url_service_ = nullptr;
   // If the browser is no longer active, let's not show the info bubble, as this
   // would make the browser the active window again.
   if (omnibox_view_ && omnibox_view_->GetWidget()->IsActive())
     ShowFirstRunBubble();
 }