Vectorize LocationIconView icons

chrome/browser/ui/toolbar/toolbar_model_impl.cc

Index: chrome/browser/ui/toolbar/toolbar_model_impl.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.cc b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
index 3bdae4af5868f8e61e93fe9b4ed2d9327f65be75..278a655685b93bf78ae0794e992e748c009faef1 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_impl.cc
+++ b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
@@ -33,6 +33,7 @@
 #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;
@@ -126,9 +127,6 @@ SecurityStateModel::SecurityLevel ToolbarModelImpl::GetSecurityLevel(
 }
 
 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.

 }
 
@@ -153,6 +151,33 @@ int ToolbarModelImpl::GetIconForSecurityLevel(
   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();