Move SecurityLevel into a class of its own

chrome/browser/ssl/security_level_policy.h

+// Copyright 2015 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.
+
+#ifndef CHROME_BROWSER_SSL_SECURITY_LEVEL_POLICY_H_
+#define CHROME_BROWSER_SSL_SECURITY_LEVEL_POLICY_H_
+
+#include "base/macros.h"
+
+namespace content {
+class WebContents;
+}  // namespace content
+
+// This class is responsible for computing the security level of a page.
+class SecurityLevelPolicy {
+ public:
+  // TODO(wtc): unify ToolbarModel::SecurityLevel with SecurityStyle.  We
+  // don't need two sets of security UI levels.  SECURITY_STYLE_AUTHENTICATED
+  // needs to be refined into three levels: warning, standard, and EV.

[Ryan Sleevi, 2015/05/08 00:24:24]

This TODO probably shouldn't be preserved - wtc no longer hacks on Chromium, and
the context is entirely missing (and it's an old comment)

Or you can take it on if it's a cleanup you agree with - filing a bug to track
;) [It does seem sensible, because it's weird to have SecurityLevel and
SecurityStyle]

[felt, 2015/05/08 00:27:23]

there is a bug here: https://code.google.com/p/chromium/issues/detail?id=425728

[estark, 2015/05/08 04:54:33]

I think we should remove the TODO because 1. there's not a concrete action item,
2. there's an open question of whether we can/should actually address it, and if
so how to do it, and 3. the issue is recorded elsewhere (in the bug, and I think
it's one of lgarron's OKRs for this quarter).

I would love if we could address this sooner rather than later; if we are able
to get rid of SecurityLevel and just have one SecurityStyle enum in //content,
that will simplify my job of getting the security state to devtools. The tricky
part, I think, is that we still need a translation layer in //chrome to handle
Chrome-specific policy like SHA1. So GetSecurityLevelForWebContents would still
have to exist, but would just return a SecurityStyle instead of a SecurityLevel,
and it could be a different SecurityStyle than what's in the SSLStatus of the
navigation entry. (Mostly just rambling here, tl;dr is I think we should remove
the TODO but try to get to this very soon)

[Peter Kasting, 2015/05/08 06:38:00]

Hmm, to be pedantic, I don't think (1) or (2) are true ((1) is concretely about
"unify these two objects" and (2) seems to be answered "clearly yes we should do
this").

I don't think removing the TODO is massively harmful, but I think it would be
better to preserve than to leave it.  You're right that there's a bug, and that
we should deal with it, but in the meantime I don't think the TODO is a bad
thing.

I would just replace "ToolbarModel::SecurityLevel" with either "this enum" or
"SecurityLevel".

Part of why I suggested dealing with this now is that I think it would also
inform the question of how this class should be named, instantiated, etc.

[estark, 2015/05/09 02:29:08]

Ok, fair enough. I'll leave it in for now.

+  //
+  // A Java counterpart will be generated for this enum.
+  // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ssl
+  // GENERATED_JAVA_CLASS_NAME_OVERRIDE: SecurityLevelPolicySecurityLevel
+  enum SecurityLevel {

[Ryan Sleevi, 2015/05/08 00:24:23]

Why are you assigning explicit values? Enums will be zero-initialized and
increase.

If it's because this needs to be kept in sync with "something", it would help to
explain it. If it's to accomodate the Java bit specifically, ignore me, but it's
just something that caught my eye.

[estark, 2015/05/09 02:29:08]

Hmm, I'm not sure why they're there. (This was relocated code, not new code.) I
added a comment that this enum needs to line up with the icons list in
ToolbarModelImpl::GetIconForSecurityLevel, but that doesn't mean it needs to
have explicit values. (On a related note, I think it might be better to have a
switch statement in GetIconForSecurityLevel rather than an array indexed by
SecurityLevels. I can go ahead and make that change if someone tells me it's
reasonable.)

The explicit values do not appear to be necessary for the Java counterpart (see
e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/text_i...
which has a Java counterpart but no explicit values), so I'll remove them and
see if anything breaks on the trybots.

[felt, 2015/05/12 00:01:26]

A switch statement sounds much less likely to break by accident. +1 to switch
statement.

[estark, 2015/05/12 04:37:47]

Done. (And removed NUM_SECURITY_LEVELS which no longer serves any purpose.)

+    // HTTP/no URL/user is editing
+    NONE = 0,
+
+    // HTTPS with valid EV cert
+    EV_SECURE = 1,
+
+    // HTTPS (non-EV)
+    SECURE = 2,
+
+    // HTTPS, but unable to check certificate revocation status or with insecure
+    // content on the page
+    SECURITY_WARNING = 3,
+
+    // HTTPS, but the certificate verification chain is anchored on a
+    // certificate that was installed by the system administrator
+    SECURITY_POLICY_WARNING = 4,
+
+    // Attempted HTTPS and failed, page not authenticated
+    SECURITY_ERROR = 5,
+
+    NUM_SECURITY_LEVELS = 6,

[Ryan Sleevi, 2015/05/08 00:24:24]

Is this necessary?

[estark, 2015/05/08 04:54:32]

It's used to sanity check the icons that the security levels get mapped to:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

+  };
+
+  static SecurityLevel GetSecurityLevelForWebContents(
+      content::WebContents* web_contents);
+
+ private:
+DISALLOW_IMPLICIT_CONSTRUCTORS(SecurityLevelPolicy);

[Ryan Sleevi, 2015/05/08 00:24:24]

style: INDENT

[estark, 2015/05/09 02:29:08]

Done.

+};
+
+#endif  // CHROME_BROWSER_SSL_SECURITY_LEVEL_POLICY_H_