Vectorize LocationIconView icons

chrome/browser/ui/toolbar/toolbar_model_impl.cc

 // Copyright 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/toolbar/toolbar_model_impl.h"
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
@@ skipping 15 matching lines @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/ssl_status.h"
 #include "grit/components_scaled_resources.h"
 #include "grit/theme_resources.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/vector_icons_public.h"
 
 using content::NavigationController;
 using content::NavigationEntry;
 using content::WebContents;
 
 ToolbarModelImpl::ToolbarModelImpl(ToolbarModelDelegate* delegate)
     : delegate_(delegate) {
 }
 
 ToolbarModelImpl::~ToolbarModelImpl() {
@@ skipping 73 matching lines @@
   const SecurityStateModel* model =
       SecurityStateModel::FromWebContents(web_contents);
 
   // When editing, assume no security style.
   return (input_in_progress() && !ignore_editing)
              ? SecurityStateModel::NONE
              : model->GetSecurityInfo().security_level;
 }
 
 int ToolbarModelImpl::GetIcon() const {
-  if (WouldPerformSearchTermReplacement(false))
-    return IDR_OMNIBOX_SEARCH_SECURED;
-
   return GetIconForSecurityLevel(GetSecurityLevel(false));

[Peter Kasting, 2015/10/21 19:45:47]

I think GetIconForSecurityLevel() is now only called here, so why don't we just
combine these two functions into one.  Same with the new function you're adding.

[Evan Stade, 2015/10/21 23:20:01]

Done.

 }
 
 int ToolbarModelImpl::GetIconForSecurityLevel(
     SecurityStateModel::SecurityLevel level) const {
   switch (level) {
     case SecurityStateModel::NONE:
       return IDR_LOCATION_BAR_HTTP;
     case SecurityStateModel::EV_SECURE:
     case SecurityStateModel::SECURE:
       return IDR_OMNIBOX_HTTPS_VALID;
     case SecurityStateModel::SECURITY_WARNING:
       // Surface Dubious as Neutral.
       return IDR_LOCATION_BAR_HTTP;
     case SecurityStateModel::SECURITY_POLICY_WARNING:
       return IDR_OMNIBOX_HTTPS_POLICY_WARNING;
     case SecurityStateModel::SECURITY_ERROR:
       return IDR_OMNIBOX_HTTPS_INVALID;
   }
 
   NOTREACHED();
   return IDR_LOCATION_BAR_HTTP;
 }
 
+gfx::VectorIconId ToolbarModelImpl::GetVectorIcon() const {
+  return GetVectorIconForSecurityLevel(GetSecurityLevel(false));
+}
+
+gfx::VectorIconId ToolbarModelImpl::GetVectorIconForSecurityLevel(
+    SecurityStateModel::SecurityLevel level) const {
+#if !defined(OS_ANDROID) && !defined(OS_MACOSX) && !defined(OS_IOS)
+  switch (level) {
+    case SecurityStateModel::NONE:
+      return gfx::VectorIconId::LOCATION_BAR_HTTP;
+    case SecurityStateModel::EV_SECURE:
+    case SecurityStateModel::SECURE:
+      return gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID;
+    case SecurityStateModel::SECURITY_WARNING:
+      // Surface Dubious as Neutral.
+      return gfx::VectorIconId::LOCATION_BAR_HTTP;
+    case SecurityStateModel::SECURITY_POLICY_WARNING:
+      return gfx::VectorIconId::BUSINESS;

[Peter Kasting, 2015/10/21 19:45:47]

It's not at all obvious what this is from the name, and we're not using it for
anything relating to "business", so let's name the icon and enum
LOCATION_BAR_HTTPS_POLICY_WARNING to match the non-vector naming scheme and for
clarity.

(I assume you got this name from the internal icon repository, but I'd rather
optimize for readability here, and if it's necessary for maintainers to know
where the icon comes from, put a comment in the icon file saying "Note: this is
sourced from the internal "business" icon" or some similar explanation.

[Evan Stade, 2015/10/21 23:20:01]

Yes, "business" is the name of this icon, which is straight from the MD icon
depot. I've been consistent in using the MD name for icons of this type. The
other icons here are actually specialized for this one UI surface, but this one
is not, the reason being that this icon should be very rarely visible.

Note that the IDR_OMNIBOX_ name is only an alias, it actually points at
controlled_setting_mandatory.png which at one point was used in many parts of
Chrome (seemingly this is the last usage, though). I anticipate BUSINESS will be
used in more than one place as well (for example, the origin info bubble, in
place of pageinfo_enterprise_managed.png). I guess you could view it as a
drawback that vector icons can't have these kinds of aliases, but I never really
considered them very useful anyway.

[Peter Kasting, 2015/10/22 00:05:24]

FWIW, I'd like to rename all the icons to match their uses in Chrome, not just
this one.  Most maintainers here will be much more familiar with Chrome code and
practices than with the MD icon depot, which basically no one will have ever
looked at.  I like being able to tie icons back to the MD repo -- which is why
I'd be glad to see us put a comment in the icon file indicating its origin --
but the only people who should need those are people who want to update the icon
itself, for which the comment seems like the right combination of "gives the
info to the relevant reader" without "also gives it to everyone else who doesn't
care".

(Actually, putting such comments in the icon files makes sense anyway, because
in most cases today it's not really clear from the icon file whether it's an
unchanged copy of the MD icon, an updated copy tweaked by the designer, wholly
our own invention, etc.  One or two of the icons say this but most don't.)

For icons used in more than one place, like this one might eventually be, it's
OK if we don't have aliases and thus can't name this "omnibox".  But we should
at least pick a name that has some more meaning than "business".  As a reader
this is almost like if you named the bookmark star icon "cabinet".  Not only
does it appear to have nothing to do with the code, it's also a generic word
that seems to say nothing about the icon itself.

+    case SecurityStateModel::SECURITY_ERROR:
+      return gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID;
+  }
+#endif
+
+  NOTREACHED();
+  return gfx::VectorIconId::LOCATION_BAR_HTTP;
+}
+
 base::string16 ToolbarModelImpl::GetEVCertName() const {
   if (GetSecurityLevel(false) != SecurityStateModel::EV_SECURE)
     return base::string16();
 
   // Note: Navigation controller and active entry are guaranteed non-NULL or
   // the security level would be NONE.
   scoped_refptr<net::X509Certificate> cert;
   content::CertStore::GetInstance()->RetrieveCert(
       GetNavigationController()->GetVisibleEntry()->GetSSL().cert_id, &cert);
 
@@ skipping 81 matching lines @@
 
   // Otherwise, extract search terms for HTTPS pages that do not have a security
   // error.
   SecurityStateModel::SecurityLevel security_level =
       GetSecurityLevel(ignore_editing);
   return ((security_level == SecurityStateModel::NONE) ||
           (security_level == SecurityStateModel::SECURITY_ERROR))
              ? base::string16()
              : search_terms;
 }