New origin security indicator icons.

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

 // 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/cocoa/omnibox/omnibox_view_mac.h"
 
 #include <Carbon/Carbon.h>  // kVK_Return
 
 #include "base/mac/foundation_util.h"
 #include "base/memory/ptr_util.h"
@@ skipping 157 matching lines @@
 }
 
 // static
 NSColor* OmniboxViewMac::BaseTextColor(bool in_dark_mode) {
   if (!ui::MaterialDesignController::IsModeMaterial()) {
     return [NSColor darkGrayColor];
   }
   return skia::SkColorToCalibratedNSColor(BaseTextColorSkia(in_dark_mode));
 }
 
+// static
+SkColor OmniboxViewMac::GetSecureTextColorSkia(
+    security_state::SecurityStateModel::SecurityLevel security_level,
+    bool in_dark_mode) {
+  return skia::NSDeviceColorToSkColor(
+      GetSecureTextColor(security_level, in_dark_mode));
+}
+
+// static
+NSColor* OmniboxViewMac::GetSecureTextColor(
+    security_state::SecurityStateModel::SecurityLevel security_level,
+    bool in_dark_mode) {
+  NSColor* color;
+  if (security_level == security_state::SecurityStateModel::EV_SECURE ||
+      security_level == security_state::SecurityStateModel::SECURE) {
+    color = SecureSchemeColor(in_dark_mode);
+  } else if (security_level ==
+             security_state::SecurityStateModel::SECURITY_ERROR) {
+    color = SecurityErrorSchemeColor(in_dark_mode);
+  } else if (security_level ==
+             security_state::SecurityStateModel::SECURITY_WARNING) {
+    color = SecurityWarningSchemeColor(in_dark_mode);
+  } else {
+    NOTREACHED();
+    color = BaseTextColor(in_dark_mode);

[Evan Stade, 2016/05/19 17:58:49]

I'm often told not to handle NOTREACHED

[Peter Kasting, 2016/05/19 18:36:19]

Yeah.  And in general looking at this makes it clear that we could just
early-return to do this all more simply:

{
  if (security_level == security_state::SecurityStateModel::EV_SECURE ||
      security_level == security_state::SecurityStateModel::SECURE)
    return SecureSchemeColor(in_dark_mode);

  if (security_level == security_state::SecurityStateModel::SECURITY_ERROR)
    return SecurityErrorSchemeColor(in_dark_mode);

  DCHECK_EQ(security_state::SecurityStateModel::SECURITY_WARNING,
            security_level);
  return SecurityWarningSchemeColor(in_dark_mode);
}

[palmer, 2016/05/19 18:40:40]

This is borrowed from the pre-existing code (compare with the deleted lines in
this file); and mirrors the Views code. Again, I could go either way, but I'd
prefer not to mess with pre-existing code unless there is a compelling reason.
I've never been advised not to handle NOTREACHED.

[Peter Kasting, 2016/05/19 18:43:21]

Please do fix, here and/or there, as possible.
Not handling NOTREACHED has been part of Chrome style for as long as I can
remember.  See
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
for more.

The existing code was wrong.

[palmer, 2016/05/19 19:55:44]

Fixed here in Cocoa.

The Views code by estade/estark/jcivelli (GetSecureTextColor in
chrome/browser/ui/views/location_bar/location_bar_view.cc) was structured
differently(switch/case, with return at the bottom, sometimes) than the
equivalent Cocoa function; I rewrote it to follow your early-return + DCHECK_EQ
style. See if you like what I did there in the latest patchset.

+  }
+  return color;
+}
+
 OmniboxViewMac::OmniboxViewMac(OmniboxEditController* controller,
                                Profile* profile,
                                CommandUpdater* command_updater,
                                AutocompleteTextField* field)
     : OmniboxView(
           controller,
           base::WrapUnique(new ChromeOmniboxClient(controller, profile))),
       profile_(profile),
       popup_view_(new OmniboxPopupViewMac(this, model(), field)),
       field_(field),
@@ skipping 403 matching lines @@
   // TODO(shess): GTK has this as a member var, figure out why.
   // [Could it be to not change if no change?  If so, I'm guessing
   // AppKit may already handle that.]
   const security_state::SecurityStateModel::SecurityLevel security_level =
       controller()->GetToolbarModel()->GetSecurityLevel(false);
 
   // Emphasize the scheme for security UI display purposes (if necessary).
   if (!model()->user_input_in_progress() && model()->CurrentTextIsURL() &&
       scheme.is_nonempty() &&
       (security_level != security_state::SecurityStateModel::NONE)) {
-    NSColor* color;
-    if (security_level == security_state::SecurityStateModel::EV_SECURE ||
-        security_level == security_state::SecurityStateModel::SECURE) {
-      color = SecureSchemeColor(in_dark_mode);
-    } else if (security_level ==
-               security_state::SecurityStateModel::SECURITY_ERROR) {
-      color = SecurityErrorSchemeColor(in_dark_mode);
+    NSColor* color = GetSecureTextColor(security_level, in_dark_mode);

[Evan Stade, 2016/05/19 17:58:49]

no need for this local var

[palmer, 2016/05/19 18:40:40]

Done.

+    if (security_level == security_state::SecurityStateModel::SECURITY_ERROR) {
       // Add a strikethrough through the scheme.
       [attributedString addAttribute:NSStrikethroughStyleAttributeName
                  value:[NSNumber numberWithInt:NSUnderlineStyleSingle]
                  range:ComponentToNSRange(scheme)];
-    } else if (security_level ==
-               security_state::SecurityStateModel::SECURITY_WARNING) {
-      color = SecurityWarningSchemeColor(in_dark_mode);
-    } else {
-      NOTREACHED();
-      color = BaseTextColor(in_dark_mode);
     }
     [attributedString addAttribute:NSForegroundColorAttributeName
                              value:color
                              range:ComponentToNSRange(scheme)];
   }
 }
 
 void OmniboxViewMac::OnTemporaryTextMaybeChanged(
     const base::string16& display_text,
     bool save_original_selection,
@@ skipping 505 matching lines @@
 
 NSUInteger OmniboxViewMac::GetTextLength() const {
   return [field_ currentEditor] ?  [[[field_ currentEditor] string] length] :
                                    [[field_ stringValue] length];
 }
 
 bool OmniboxViewMac::IsCaretAtEnd() const {
   const NSRange selection = GetSelectedRange();
   return NSMaxRange(selection) == GetTextLength();
 }