Move SecurityLevel into a class of its own

Move SecurityLevel into a class of its own

This CL moves |GetSecurityLevelForWebContents| and the |SecurityLevel|
enum into a class of their own: |ConnectionSecurityHelper| in
//chrome/browser/ssl (since it is applying Chrome-specific security
policy to determine the security level of a page).

Later, we will use the new |ConnectionSecurityHelper| class to compute a
security level for a page to send to the DevTools Security
panel. Specifically, we need to be able to calculate the security level
for a given page, not just the WebContents for the active tab.

BUG=484392
Committed: https://crrev.com/ca7c0a3374c21ab10c821f4f3a890afc79182acf
Cr-Commit-Position: refs/heads/master@{#329591}

Committed: https://crrev.com/f50c9b4834e151aa6b5538545a503614cb74f5f9
Cr-Commit-Position: refs/heads/master@{#329724}

[estark, 2015-05-07 05:15:12]

estark@chromium.org changed reviewers:
+ felt@chromium.org, pkasting@google.com

[estark, 2015-05-07 05:15:12]

felt, pkasting: could you please take a look? This CL is required for the
DevTools security panel project, as described in
https://docs.google.com/document/d/1kMU5dPi2PwQ7skbgwlfQsenYdTwOi2ya2WYla2uy0...

lgarron: FYI

[estark, 2015-05-07 05:15:52]

estark@chromium.org changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[estark, 2015-05-07 05:16:17]

+pkasting@chromium.org, -pkasting@google.com

[felt, 2015-05-07 20:10:18]

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
While this is being moved anyway:

What do you think about renaming this ConnectionSecurityLevelPolicy, or
something else that makes it clear that this refers specifically to the HTTP[S]
status? "SecurityLevelPolicy" doesn't reflect whether a page has been flagged as
phishing or malware, which is also "Security".

[Peter Kasting, 2015-05-07 21:35:38]

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.cc (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:30: using content::SSLStatus;
Nit: Avoid this unless is significantly improves readability below (IMO it
doesn't).

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:76: if (url.SchemeIs("http") ||
url.SchemeIs("ftp"))
Nit: Should probably be using the scheme constants in src/url/url_constants.h

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:92: if (!!(ssl.content_status &
SSLStatus::DISPLAYED_INSECURE_CONTENT))
Is the !! here really necessary (to prevent warnings)?

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:98: // See http://crbug.com/401365
for details
Nit: Trailing period

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:121: if ((ssl.cert_status &
net::CERT_STATUS_IS_EV) && cert.get())
Nit: Is explicit .get() necessary with scoped_refptr?

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:125: default:
Nit: Blank line above this

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
On 2015/05/07 20:10:18, felt wrote:
> While this is being moved anyway:
> 
> What do you think about renaming this ConnectionSecurityLevelPolicy, or
> something else that makes it clear that this refers specifically to the
HTTP[S]
> status? "SecurityLevelPolicy" doesn't reflect whether a page has been flagged
as
> phishing or malware, which is also "Security".

Another possibility would be to eliminate the wrapping class here and then
rename the enum ConnectionSecurityLevel, leaving it in the global scope.  (I
don't love globals, but I also don't love using classes just to be
pseudo-namespaces.)

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:15: // TODO(wtc): unify
ToolbarModel::SecurityLevel with SecurityStyle.  We
This CL would probably be a good time to either implement this TODO, or decide
that it makes no sense and remove it.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:50: SecurityLevelPolicy();
Use DISALLOW_IMPLICIT_CONSTRUCTORS here.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:29: #include
"chrome/browser/ssl/security_level_policy.h"
Since toolbar_model.h already #includes this, it's not necessary here.  (Comment
also applies in other files.)

Unless, that is, you want to make this an enum class, and forward-declare it in
toolbar_model.h, at which point you will, in fact, want to have the added
#includes in these other .cc files.  Did you want to do that instead?

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.h:37: static
SecurityLevelPolicy::SecurityLevel GetSecurityLevelForWebContents(
I think this function should disappear and callers should use the underlying API
directly.

Since this method is neither giving you information related to the current
toolbar state, nor implemented in terms of underlying toolbar-based APIs, it no
longer belongs here.

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

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:19: // needs to be refined into three
levels: warning, standard, and EV.
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]

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:24: enum SecurityLevel {
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.

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:45: NUM_SECURITY_LEVELS = 6,
Is this necessary?

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:52:
DISALLOW_IMPLICIT_CONSTRUCTORS(SecurityLevelPolicy);
style: INDENT

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

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:19: // needs to be refined into three
levels: warning, standard, and EV.
On 2015/05/08 00:24:24, Ryan Sleevi wrote:
> 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]

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

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

Thanks for the comments, everyone. I'm not ready for another round of review yet
but just wanted to reply to a few of the comments.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
On 2015/05/07 21:35:37, Peter Kasting wrote:
> On 2015/05/07 20:10:18, felt wrote:
> > While this is being moved anyway:
> > 
> > What do you think about renaming this ConnectionSecurityLevelPolicy, or
> > something else that makes it clear that this refers specifically to the
> HTTP[S]
> > status? "SecurityLevelPolicy" doesn't reflect whether a page has been
flagged
> as
> > phishing or malware, which is also "Security".
> 
> Another possibility would be to eliminate the wrapping class here and then
> rename the enum ConnectionSecurityLevel, leaving it in the global scope.  (I
> don't love globals, but I also don't love using classes just to be
> pseudo-namespaces.)

I think I'd like to leave it as a class, if that's alright. My thinking is that
the next step in this project is to add a method here called
|ExplainSecurityLevel|. At that point it might make sense to make it a real
instantiable class. (i.e. you can construct a |SecurityLevelPolicy| object by
passing a |WebContents*| to the constructor, and then you can call
|GetSecurityLevel| and |ExplainSecurityLevel| on the object, and it will
probably make sense to factor out some logic into private methods). Or I could
go ahead and make it instantiable now in anticipation of the next step, if that
would be preferable.

I'm planning on renaming the class to ConnectionSecurityPolicy since
ConnectionSecurityLevelPolicy seems just a tad too long to me.

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:19: // needs to be refined into three
levels: warning, standard, and EV.
On 2015/05/08 00:27:23, felt wrote:
> On 2015/05/08 00:24:24, Ryan Sleevi wrote:
> > 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]
> 
> there is a bug here:
https://code.google.com/p/chromium/issues/detail?id=425728

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)

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:45: NUM_SECURITY_LEVELS = 6,
On 2015/05/08 00:24:24, Ryan Sleevi wrote:
> Is this necessary?

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/...

[Peter Kasting, 2015-05-08 06:38:01]

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
On 2015/05/08 04:54:32, estark wrote:
> On 2015/05/07 21:35:37, Peter Kasting wrote:
> > On 2015/05/07 20:10:18, felt wrote:
> > > While this is being moved anyway:
> > > 
> > > What do you think about renaming this ConnectionSecurityLevelPolicy, or
> > > something else that makes it clear that this refers specifically to the
> > HTTP[S]
> > > status? "SecurityLevelPolicy" doesn't reflect whether a page has been
> flagged
> > as
> > > phishing or malware, which is also "Security".
> > 
> > Another possibility would be to eliminate the wrapping class here and then
> > rename the enum ConnectionSecurityLevel, leaving it in the global scope.  (I
> > don't love globals, but I also don't love using classes just to be
> > pseudo-namespaces.)
> 
> I think I'd like to leave it as a class, if that's alright. My thinking is
that
> the next step in this project is to add a method here called
> |ExplainSecurityLevel|. At that point it might make sense to make it a real
> instantiable class. (i.e. you can construct a |SecurityLevelPolicy| object by
> passing a |WebContents*| to the constructor, and then you can call
> |GetSecurityLevel| and |ExplainSecurityLevel| on the object, and it will
> probably make sense to factor out some logic into private methods). Or I could
> go ahead and make it instantiable now in anticipation of the next step, if
that
> would be preferable.

Leaving as a class is OK given your plans.  I wouldn't make it instantiable yet,
let's wait and do that when there are consumers that want that.

> I'm planning on renaming the class to ConnectionSecurityPolicy since
> ConnectionSecurityLevelPolicy seems just a tad too long to me.

I'm not really sure where the "policy" part of the name comes from anyway.  This
class isn't about setting or enforcing security policies, but simply describing
the existing security level.  If anything maybe the class should be
ConnectionSecurityUtils or something.

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:19: // needs to be refined into three
levels: warning, standard, and EV.
On 2015/05/08 04:54:33, estark wrote:
> On 2015/05/08 00:27:23, felt wrote:
> > On 2015/05/08 00:24:24, Ryan Sleevi wrote:
> > > 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]
> > 
> > there is a bug here:
> https://code.google.com/p/chromium/issues/detail?id=425728
> 
> 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).

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-08 14:16:00]

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
On 2015/05/08 06:38:00, Peter Kasting wrote:
> On 2015/05/08 04:54:32, estark wrote:
> > On 2015/05/07 21:35:37, Peter Kasting wrote:
> > > On 2015/05/07 20:10:18, felt wrote:
> > > > While this is being moved anyway:
> > > > 
> > > > What do you think about renaming this ConnectionSecurityLevelPolicy, or
> > > > something else that makes it clear that this refers specifically to the
> > > HTTP[S]
> > > > status? "SecurityLevelPolicy" doesn't reflect whether a page has been
> > flagged
> > > as
> > > > phishing or malware, which is also "Security".
> > > 
> > > Another possibility would be to eliminate the wrapping class here and then
> > > rename the enum ConnectionSecurityLevel, leaving it in the global scope. 
(I
> > > don't love globals, but I also don't love using classes just to be
> > > pseudo-namespaces.)
> > 
> > I think I'd like to leave it as a class, if that's alright. My thinking is
> that
> > the next step in this project is to add a method here called
> > |ExplainSecurityLevel|. At that point it might make sense to make it a real
> > instantiable class. (i.e. you can construct a |SecurityLevelPolicy| object
by
> > passing a |WebContents*| to the constructor, and then you can call
> > |GetSecurityLevel| and |ExplainSecurityLevel| on the object, and it will
> > probably make sense to factor out some logic into private methods). Or I
could
> > go ahead and make it instantiable now in anticipation of the next step, if
> that
> > would be preferable.
> 
> Leaving as a class is OK given your plans.  I wouldn't make it instantiable
yet,
> let's wait and do that when there are consumers that want that.
> 
> > I'm planning on renaming the class to ConnectionSecurityPolicy since
> > ConnectionSecurityLevelPolicy seems just a tad too long to me.
> 
> I'm not really sure where the "policy" part of the name comes from anyway. 
This
> class isn't about setting or enforcing security policies, but simply
describing
> the existing security level.  If anything maybe the class should be
> ConnectionSecurityUtils or something.

I was thinking of "SHA1 signature in chain => page should be treated as having a
degraded security status" as a policy of sorts, but I'm not too attached to it,
especially if "policy" is likely to be confusing to other people. I'm fine with
ConnectionSecurityUtils, or maybe ConnectionSecurityHelper?

[estark, 2015-05-08 20:25:45]

Patchset #6 (id:100001) has been deleted

[Peter Kasting, 2015-05-08 21:32:53]

On 2015/05/08 14:16:00, estark wrote:
>
https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
> File chrome/browser/ssl/security_level_policy.h (right):
> 
>
https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
> chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
> On 2015/05/08 06:38:00, Peter Kasting wrote:
> > On 2015/05/08 04:54:32, estark wrote:
> > > On 2015/05/07 21:35:37, Peter Kasting wrote:
> > > > On 2015/05/07 20:10:18, felt wrote:
> > > > > While this is being moved anyway:
> > > > > 
> > > > > What do you think about renaming this ConnectionSecurityLevelPolicy,
or
> > > > > something else that makes it clear that this refers specifically to
the
> > > > HTTP[S]
> > > > > status? "SecurityLevelPolicy" doesn't reflect whether a page has been
> > > flagged
> > > > as
> > > > > phishing or malware, which is also "Security".
> > > > 
> > > > Another possibility would be to eliminate the wrapping class here and
then
> > > > rename the enum ConnectionSecurityLevel, leaving it in the global scope.

> (I
> > > > don't love globals, but I also don't love using classes just to be
> > > > pseudo-namespaces.)
> > > 
> > > I think I'd like to leave it as a class, if that's alright. My thinking is
> > that
> > > the next step in this project is to add a method here called
> > > |ExplainSecurityLevel|. At that point it might make sense to make it a
real
> > > instantiable class. (i.e. you can construct a |SecurityLevelPolicy| object
> by
> > > passing a |WebContents*| to the constructor, and then you can call
> > > |GetSecurityLevel| and |ExplainSecurityLevel| on the object, and it will
> > > probably make sense to factor out some logic into private methods). Or I
> could
> > > go ahead and make it instantiable now in anticipation of the next step, if
> > that
> > > would be preferable.
> > 
> > Leaving as a class is OK given your plans.  I wouldn't make it instantiable
> yet,
> > let's wait and do that when there are consumers that want that.
> > 
> > > I'm planning on renaming the class to ConnectionSecurityPolicy since
> > > ConnectionSecurityLevelPolicy seems just a tad too long to me.
> > 
> > I'm not really sure where the "policy" part of the name comes from anyway. 
> This
> > class isn't about setting or enforcing security policies, but simply
> describing
> > the existing security level.  If anything maybe the class should be
> > ConnectionSecurityUtils or something.
> 
> I was thinking of "SHA1 signature in chain => page should be treated as having
a
> degraded security status" as a policy of sorts, but I'm not too attached to
it,
> especially if "policy" is likely to be confusing to other people. I'm fine
with
> ConnectionSecurityUtils, or maybe ConnectionSecurityHelper?

Whichever you think is better.  Don't care.

[estark, 2015-05-08 21:42:50]

Patchset #6 (id:120001) has been deleted

[estark, 2015-05-08 22:05:56]

Patchset #6 (id:140001) has been deleted

[estark, 2015-05-08 22:42:25]

Patchset #6 (id:130025) has been deleted

[estark, 2015-05-09 02:29:09]

Ok, I think this is ready for another look now, please.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.cc (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:30: using content::SSLStatus;
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Nit: Avoid this unless is significantly improves readability below (IMO it
> doesn't).

Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:76: if (url.SchemeIs("http") ||
url.SchemeIs("ftp"))
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Nit: Should probably be using the scheme constants in src/url/url_constants.h

Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:92: if (!!(ssl.content_status &
SSLStatus::DISPLAYED_INSECURE_CONTENT))
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Is the !! here really necessary (to prevent warnings)?

Looks like it isn't. Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:98: // See http://crbug.com/401365
for details
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:121: if ((ssl.cert_status &
net::CERT_STATUS_IS_EV) && cert.get())
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Nit: Is explicit .get() necessary with scoped_refptr?

Looks like it isn't. Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.cc:125: default:
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Nit: Blank line above this

Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:13: class SecurityLevelPolicy {
On 2015/05/08 14:16:00, estark wrote:
> On 2015/05/08 06:38:00, Peter Kasting wrote:
> > On 2015/05/08 04:54:32, estark wrote:
> > > On 2015/05/07 21:35:37, Peter Kasting wrote:
> > > > On 2015/05/07 20:10:18, felt wrote:
> > > > > While this is being moved anyway:
> > > > > 
> > > > > What do you think about renaming this ConnectionSecurityLevelPolicy,
or
> > > > > something else that makes it clear that this refers specifically to
the
> > > > HTTP[S]
> > > > > status? "SecurityLevelPolicy" doesn't reflect whether a page has been
> > > flagged
> > > > as
> > > > > phishing or malware, which is also "Security".
> > > > 
> > > > Another possibility would be to eliminate the wrapping class here and
then
> > > > rename the enum ConnectionSecurityLevel, leaving it in the global scope.

> (I
> > > > don't love globals, but I also don't love using classes just to be
> > > > pseudo-namespaces.)
> > > 
> > > I think I'd like to leave it as a class, if that's alright. My thinking is
> > that
> > > the next step in this project is to add a method here called
> > > |ExplainSecurityLevel|. At that point it might make sense to make it a
real
> > > instantiable class. (i.e. you can construct a |SecurityLevelPolicy| object
> by
> > > passing a |WebContents*| to the constructor, and then you can call
> > > |GetSecurityLevel| and |ExplainSecurityLevel| on the object, and it will
> > > probably make sense to factor out some logic into private methods). Or I
> could
> > > go ahead and make it instantiable now in anticipation of the next step, if
> > that
> > > would be preferable.
> > 
> > Leaving as a class is OK given your plans.  I wouldn't make it instantiable
> yet,
> > let's wait and do that when there are consumers that want that.
> > 
> > > I'm planning on renaming the class to ConnectionSecurityPolicy since
> > > ConnectionSecurityLevelPolicy seems just a tad too long to me.
> > 
> > I'm not really sure where the "policy" part of the name comes from anyway. 
> This
> > class isn't about setting or enforcing security policies, but simply
> describing
> > the existing security level.  If anything maybe the class should be
> > ConnectionSecurityUtils or something.
> 
> I was thinking of "SHA1 signature in chain => page should be treated as having
a
> degraded security status" as a policy of sorts, but I'm not too attached to
it,
> especially if "policy" is likely to be confusing to other people. I'm fine
with
> ConnectionSecurityUtils, or maybe ConnectionSecurityHelper?

Renamed to ConnectionSecurityHelper.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:50: SecurityLevelPolicy();
On 2015/05/07 21:35:37, Peter Kasting wrote:
> Use DISALLOW_IMPLICIT_CONSTRUCTORS here.

Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:29: #include
"chrome/browser/ssl/security_level_policy.h"
On 2015/05/07 21:35:38, Peter Kasting wrote:
> Since toolbar_model.h already #includes this, it's not necessary here. 
(Comment
> also applies in other files.)
> 
> Unless, that is, you want to make this an enum class, and forward-declare it
in
> toolbar_model.h, at which point you will, in fact, want to have the added
> #includes in these other .cc files.  Did you want to do that instead?

Done.

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/1123943002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.h:37: static
SecurityLevelPolicy::SecurityLevel GetSecurityLevelForWebContents(
On 2015/05/07 21:35:38, Peter Kasting wrote:
> I think this function should disappear and callers should use the underlying
API
> directly.
> 
> Since this method is neither giving you information related to the current
> toolbar state, nor implemented in terms of underlying toolbar-based APIs, it
no
> longer belongs here.

Done.

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:19: // needs to be refined into three
levels: warning, standard, and EV.
On 2015/05/08 06:38:00, Peter Kasting wrote:
> On 2015/05/08 04:54:33, estark wrote:
> > On 2015/05/08 00:27:23, felt wrote:
> > > On 2015/05/08 00:24:24, Ryan Sleevi wrote:
> > > > 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]
> > > 
> > > there is a bug here:
> > https://code.google.com/p/chromium/issues/detail?id=425728
> > 
> > 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).
> 
> 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.

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

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:24: enum SecurityLevel {
On 2015/05/08 00:24:23, Ryan Sleevi wrote:
> 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.

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.

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:52:
DISALLOW_IMPLICIT_CONSTRUCTORS(SecurityLevelPolicy);
On 2015/05/08 00:24:24, Ryan Sleevi wrote:
> style: INDENT

Done.

[Peter Kasting, 2015-05-09 04:34:57]

LGTM

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:35: std::string choice =
Nit: There are two ways to write this function in a less repetitive fashion:

(1)

  std::string choice = ...;
  if ((choice != switches::kMarkNonSecureAsNeutral) &&
      (choice != ...) &&
      (choice != ...))
    choice = base::FieldTrialList::FindFullName("MarkNonSecureAs");
  if (choice != switches::kMarkNonSecureAsNeutral)
    return ...;
  ...;

(2) Change this function to take a string and return a SecurityLevel for it,
then change the caller to do:

  SecurityLevel security_level = SecurityLevelForString(
      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
          switches::kMarkNonSecureAs);
  return (security_level == NONE)
      ? SecurityLevelForString(
          base::FieldTrialList::FindFullName("MarkNonSecureAs"))
      : security_level;

Personally I kind of prefer #2.

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.h (right):

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.h:28: // HTTP/no URL/user is
editing
Nit: Eliminate "user is editing" since at this level, that's not a concern. 
(That will come in at the ToolbarModel level.)

[estark, 2015-05-11 01:59:26]

Thanks pkasting.

felt: can you please take a look at //c/b/ssl? After that I'll add OWNERS for
the android code.

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:35: std::string choice =
On 2015/05/09 04:34:56, Peter Kasting wrote:
> Nit: There are two ways to write this function in a less repetitive fashion:
> 
> (1)
> 
>   std::string choice = ...;
>   if ((choice != switches::kMarkNonSecureAsNeutral) &&
>       (choice != ...) &&
>       (choice != ...))
>     choice = base::FieldTrialList::FindFullName("MarkNonSecureAs");
>   if (choice != switches::kMarkNonSecureAsNeutral)
>     return ...;
>   ...;
> 
> (2) Change this function to take a string and return a SecurityLevel for it,
> then change the caller to do:
> 
>   SecurityLevel security_level = SecurityLevelForString(
>       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
>           switches::kMarkNonSecureAs);
>   return (security_level == NONE)
>       ? SecurityLevelForString(
>           base::FieldTrialList::FindFullName("MarkNonSecureAs"))
>       : security_level;
> 
> Personally I kind of prefer #2.

I think #2 doesn't quite match the behavior of the current function: in your #2
a switch value of kMarkNonSecureAsNeutral would get overridden by a field trial
group of kMarkNonSecureAsDubious or AsNonSecure. I did something similar though.

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.h (right):

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.h:28: // HTTP/no URL/user is
editing
On 2015/05/09 04:34:56, Peter Kasting wrote:
> Nit: Eliminate "user is editing" since at this level, that's not a concern. 
> (That will come in at the ToolbarModel level.)

Done.

[Peter Kasting, 2015-05-11 19:15:46]

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:35: std::string choice =
On 2015/05/11 01:59:25, estark wrote:
> On 2015/05/09 04:34:56, Peter Kasting wrote:
> > Nit: There are two ways to write this function in a less repetitive fashion:
> > 
> > (1)
> > 
> >   std::string choice = ...;
> >   if ((choice != switches::kMarkNonSecureAsNeutral) &&
> >       (choice != ...) &&
> >       (choice != ...))
> >     choice = base::FieldTrialList::FindFullName("MarkNonSecureAs");
> >   if (choice != switches::kMarkNonSecureAsNeutral)
> >     return ...;
> >   ...;
> > 
> > (2) Change this function to take a string and return a SecurityLevel for it,
> > then change the caller to do:
> > 
> >   SecurityLevel security_level = SecurityLevelForString(
> >       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
> >           switches::kMarkNonSecureAs);
> >   return (security_level == NONE)
> >       ? SecurityLevelForString(
> >           base::FieldTrialList::FindFullName("MarkNonSecureAs"))
> >       : security_level;
> > 
> > Personally I kind of prefer #2.
> 
> I think #2 doesn't quite match the behavior of the current function: in your
#2
> a switch value of kMarkNonSecureAsNeutral would get overridden by a field
trial
> group of kMarkNonSecureAsDubious or AsNonSecure.

You're right, good catch.

> I did something similar though.

Hmm, the resulting code is now even longer and more complex than the old code :(

I probably would have chosen #1 or else "leave as is" over this.  Doesn't matter
too much, I guess.

[estark, 2015-05-11 19:20:09]

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/210001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:35: std::string choice =
On 2015/05/11 19:15:46, Peter Kasting wrote:
> On 2015/05/11 01:59:25, estark wrote:
> > On 2015/05/09 04:34:56, Peter Kasting wrote:
> > > Nit: There are two ways to write this function in a less repetitive
fashion:
> > > 
> > > (1)
> > > 
> > >   std::string choice = ...;
> > >   if ((choice != switches::kMarkNonSecureAsNeutral) &&
> > >       (choice != ...) &&
> > >       (choice != ...))
> > >     choice = base::FieldTrialList::FindFullName("MarkNonSecureAs");
> > >   if (choice != switches::kMarkNonSecureAsNeutral)
> > >     return ...;
> > >   ...;
> > > 
> > > (2) Change this function to take a string and return a SecurityLevel for
it,
> > > then change the caller to do:
> > > 
> > >   SecurityLevel security_level = SecurityLevelForString(
> > >       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
> > >           switches::kMarkNonSecureAs);
> > >   return (security_level == NONE)
> > >       ? SecurityLevelForString(
> > >           base::FieldTrialList::FindFullName("MarkNonSecureAs"))
> > >       : security_level;
> > > 
> > > Personally I kind of prefer #2.
> > 
> > I think #2 doesn't quite match the behavior of the current function: in your
> #2
> > a switch value of kMarkNonSecureAsNeutral would get overridden by a field
> trial
> > group of kMarkNonSecureAsDubious or AsNonSecure.
> 
> You're right, good catch.
> 
> > I did something similar though.
> 
> Hmm, the resulting code is now even longer and more complex than the old code
:(
> 
> I probably would have chosen #1 or else "leave as is" over this.  Doesn't
matter
> too much, I guess.

ok, I reverted it back to leave it as it was originally

[felt, 2015-05-12 00:01:27]

c/b/ssl lgtm

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:24: enum SecurityLevel {
On 2015/05/09 02:29:08, estark wrote:
> On 2015/05/08 00:24:23, Ryan Sleevi wrote:
> > 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.
> 
> 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.)

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

> 
> 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.

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:76: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kFtpScheme))
for a follow-up (not in this CL):
should this use palmer's new !SchemeIsCrytographic? (or maybe !IsOriginSecure
depending on the meaning we want here...)

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.h (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.h:17: // TODO(wtc): unify this
enum with SecurityStyle.  We
Since this is being left in, can you put a link to the bug where this is being
discussed? For future TODO-finders?

[Ryan Sleevi, 2015-05-12 01:03:28]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-05-12 01:03:29]

LGTM, although a few nits.

I do have an allergic reaction to *Helper classes, much the same way I have an
allergic reaction to files named _util[s] (even though I've been known to
introduce them in the past). That's because it doesn't provide a clear guidance
as to the responsibility of the class, its separation boundaries, and what
belongs in a new class vs should just hang off.

The best alternative I could come up with is ConnectionSecurityModel, but I'm
not strictly sure if that's correct/the best. The idea being is that again,
going back to the model/view/controller split, this is the data model for
whether a given WebContents is "secure", and the UI (whether it be devtools,
omnibox, or website settings) derives and drives the UI based on the model.

However, having a _model class outside of //c/b/ui does feel weird, so I'm OK
with push-back on it. Just thought I'd toss that sort of general unease in the
back of my mind out, to make sure that you (and other reviewers) were attentive
to the risk of this class becoming a bit of a cloaca.

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:76: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kFtpScheme))
On 2015/05/12 00:01:26, felt wrote:
> for a follow-up (not in this CL):
> should this use palmer's new !SchemeIsCrytographic? (or maybe !IsOriginSecure
> depending on the meaning we want here...)

I don't think so. For example, chrome-extension:// schemes (or any new arbitrary
scheme). I mean, blacklist vs whitelist, yeah, I just see using
SchemeIsCryptographic backfiring.

(IsOriginSecure may be sufficient, but I'm less familiar w/ that)

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.h (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.h:52: content::WebContents*
web_contents);
Document? :)

[estark, 2015-05-12 04:37:46]

estark@chromium.org changed reviewers:
+ bauerb@chromium.org

[estark, 2015-05-12 04:37:48]

Thanks felt, rsleevi.

bauerb: can you please take a look at changes in //chrome/android?

rsleevi: ack about the class name. I'll mull it over while waiting for the rest
of the review. I wonder what you think of something like
ConnectionSecurityLevelClassification? (inspired by SSLErrorClassification also
in //c/b/ssl) Its job is pretty much to classify pages into security levels.

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_level_policy.h (right):

https://codereview.chromium.org/1123943002/diff/80001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_level_policy.h:24: enum SecurityLevel {
On 2015/05/12 00:01:26, felt wrote:
> On 2015/05/09 02:29:08, estark wrote:
> > On 2015/05/08 00:24:23, Ryan Sleevi wrote:
> > > 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.
> > 
> > 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.)
> 
> A switch statement sounds much less likely to break by accident. +1 to switch
> statement.
> 
> > 
> > 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.
> 

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

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:76: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kFtpScheme))
On 2015/05/12 01:03:29, Ryan Sleevi wrote:
> On 2015/05/12 00:01:26, felt wrote:
> > for a follow-up (not in this CL):
> > should this use palmer's new !SchemeIsCrytographic? (or maybe
!IsOriginSecure
> > depending on the meaning we want here...)
> 
> I don't think so. For example, chrome-extension:// schemes (or any new
arbitrary
> scheme). I mean, blacklist vs whitelist, yeah, I just see using
> SchemeIsCryptographic backfiring.
> 
> (IsOriginSecure may be sufficient, but I'm less familiar w/ that)

I'll ask Palmer about IsOriginSecure tomorrow.

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.h (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.h:17: // TODO(wtc): unify this
enum with SecurityStyle.  We
On 2015/05/12 00:01:26, felt wrote:
> Since this is being left in, can you put a link to the bug where this is being
> discussed? For future TODO-finders?

Done.

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.h:52: content::WebContents*
web_contents);
On 2015/05/12 01:03:29, Ryan Sleevi wrote:
> Document? :)

Done.

[Bernhard Bauer, 2015-05-12 10:09:41]

Android LGTM with nits:

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1147: * @return The
current {ConnectionSecurityHelperSecurityLevel} for the tab.
You want {@link ...} here.

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:438:
*                                  level of the page.
Align this with the start of the comment (or indent all of the continued lines
eight spaces).

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ssl/ConnectionSecurityHelper.java
(right):

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ssl/ConnectionSecurityHelper.java:12:
public class ConnectionSecurityHelper {
Nit: Add a private constructor to prevent instantiation.

[estark, 2015-05-12 17:07:49]

estark@chromium.org changed reviewers:
+ sky@chromium.org

[estark, 2015-05-12 17:07:50]

Thanks bauerb.

sky@: can you please review chrome/chrome.gyp?

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1147: * @return The
current {ConnectionSecurityHelperSecurityLevel} for the tab.
On 2015/05/12 10:09:41, Bernhard Bauer wrote:
> You want {@link ...} here.

Done.

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:438:
*                                  level of the page.
On 2015/05/12 10:09:41, Bernhard Bauer wrote:
> Align this with the start of the comment (or indent all of the continued lines
> eight spaces).

Done.

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ssl/ConnectionSecurityHelper.java
(right):

https://codereview.chromium.org/1123943002/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ssl/ConnectionSecurityHelper.java:12:
public class ConnectionSecurityHelper {
On 2015/05/12 10:09:41, Bernhard Bauer wrote:
> Nit: Add a private constructor to prevent instantiation.

Done.

[sky, 2015-05-12 19:33:02]

LGTM

[estark, 2015-05-13 04:35:50]

The CQ bit was checked by estark@chromium.org

[estark, 2015-05-13 04:35:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, felt@chromium.org, rsleevi@chromium.org,
bauerb@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1123943002/#ps350001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-13 04:36:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1123943002/350001

[estark, 2015-05-13 04:37:15]

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security_helper.cc (right):

https://codereview.chromium.org/1123943002/diff/270001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security_helper.cc:76: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kFtpScheme))
On 2015/05/12 04:37:48, estark wrote:
> On 2015/05/12 01:03:29, Ryan Sleevi wrote:
> > On 2015/05/12 00:01:26, felt wrote:
> > > for a follow-up (not in this CL):
> > > should this use palmer's new !SchemeIsCrytographic? (or maybe
> !IsOriginSecure
> > > depending on the meaning we want here...)
> > 
> > I don't think so. For example, chrome-extension:// schemes (or any new
> arbitrary
> > scheme). I mean, blacklist vs whitelist, yeah, I just see using
> > SchemeIsCryptographic backfiring.
> > 
> > (IsOriginSecure may be sufficient, but I'm less familiar w/ that)
> 
> I'll ask Palmer about IsOriginSecure tomorrow.

Palmer gave a thumbs-up on |!IsOriginSecure| so I'll do that in a follow-up CL.

[commit-bot: I haz the power, 2015-05-13 04:40:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 04:40:02]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[estark, 2015-05-13 04:45:53]

The CQ bit was checked by estark@chromium.org

[estark, 2015-05-13 04:45:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
felt@chromium.org, pkasting@chromium.org, sky@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1123943002/#ps370001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-13 04:46:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1123943002/370001

[commit-bot: I haz the power, 2015-05-13 05:37:49]

Committed patchset #16 (id:370001)

[commit-bot: I haz the power, 2015-05-13 05:38:36]

Patchset 16 (id:??) landed as
https://crrev.com/ca7c0a3374c21ab10c821f4f3a890afc79182acf
Cr-Commit-Position: refs/heads/master@{#329591}

[estark, 2015-05-13 16:33:39]

A revert of this CL (patchset #16 id:370001) has been created in
https://codereview.chromium.org/1138473003/ by estark@chromium.org.

The reason for reverting is: Downstream build broken by this change because of
forgotten JNI method registration. I need to register the JNI method before
relanding..

[estark, 2015-05-13 20:08:07]

estark@chromium.org changed reviewers:
+ tedchoc@chromium.org

[estark, 2015-05-13 20:08:08]

tedchoc: could you please take a look at the JNI registration for
ConnectionSecurityHelper? I also added a smoke test that calls
|getSecurityLevel| which passes now.

pasko, jdduke: CC'ing you in case you want to take a look also

Thank you!

[Ted C, 2015-05-13 20:19:39]

On 2015/05/13 20:08:08, estark wrote:
> tedchoc: could you please take a look at the JNI registration for
> ConnectionSecurityHelper? I also added a smoke test that calls
> |getSecurityLevel| which passes now.
> 
> pasko, jdduke: CC'ing you in case you want to take a look also
> 
> Thank you!

lgtm

[estark, 2015-05-13 20:22:38]

The CQ bit was checked by estark@chromium.org

[estark, 2015-05-13 20:22:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
felt@chromium.org, pkasting@chromium.org, sky@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1123943002/#ps420001
(title: "register in chrome_jni_registrar")

[commit-bot: I haz the power, 2015-05-13 20:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1123943002/420001

[commit-bot: I haz the power, 2015-05-13 22:02:47]

Committed patchset #19 (id:420001)

[commit-bot: I haz the power, 2015-05-13 22:03:42]

Patchset 19 (id:??) landed as
https://crrev.com/f50c9b4834e151aa6b5538545a503614cb74f5f9
Cr-Commit-Position: refs/heads/master@{#329724}

[sergeyv, 2015-05-14 10:54:25]

A revert of this CL (patchset #19 id:420001) has been created in
https://codereview.chromium.org/1135373003/ by sergeyv@chromium.org.

The reason for reverting is: Android Tests dbg bot is failing:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20(dbg)

Failure example:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
.