We should use SSLInfo or related types for HTTPS...

We should use SSLInfo or related types for HTTPS
URLs only.

R=eroman,finnur
BUG=53366
TEST=net_unittests --gtest_filter=HTTPSRequestTest.HTTPS*Test

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65864

[wtc, 2010-10-29 22:51:04]

eroman: please review this CL.  You may omit page_info_model.cc
if you have trouble understanding the code.  I will ask finnur
to cover that file later.  Thanks.

[eroman, 2010-11-08 21:09:37]

LGTM.

sorry for the big lag!

[wtc, 2010-11-09 00:24:19]

Finnur: please review this CL.  You may skip http_response_info.cc
as eroman has reviewed it.

The idea of this CL for the three files in chrome/browser
is that we should ignore 'cert_status' and 'security_bits'
unless we have a valid (nonzero) 'cert_id'.

[Finnur, 2010-11-09 11:40:04]

http://codereview.chromium.org/4210004/diff/6002/7001
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/4210004/diff/6002/7001#newcode166
chrome/browser/page_info_model.cc:166: DCHECK_EQ(ssl.security_style(),
SECURITY_STYLE_UNAUTHENTICATED);
Wait... we are not failing safe in this case, are we? If we get to a state where
security_style is != unauthenticated don't we have a big problem somewhere?
Should we say:

icon_id = ssl.security_style() == SECURITY_STYLE_UNAUTHENTICATED ?
ICON_STATE_WARNING_MAJOR : ICON_STATE_ERROR;

WDYT?

Or, alternatively, add (... && ssl.security_style() ==
SECURITY_STYLE_UNAUTHENTICATED) to line 164 since then we fall into the clause
on line 171.

No?

[wtc, 2010-11-10 20:40:56]

Finnur: thanks for the review!

http://codereview.chromium.org/4210004/diff/6002/chrome/browser/page_info_mod...
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/4210004/diff/6002/chrome/browser/page_info_mod...
chrome/browser/page_info_model.cc:166: DCHECK_EQ(ssl.security_style(),
SECURITY_STYLE_UNAUTHENTICATED);
On 2010/11/09 11:40:04, Finnur wrote:
> Wait... we are not failing safe in this case, are we? If we get to a state
where
> security_style is != unauthenticated don't we have a big problem somewhere?

In general I trust our code maintains invariants correctly.
This is why I simply use a DCHECK here to assert an
invariant.

But I implemented your first suggestion as fail-safe/defensive
programming.  Please review Patch Set 3.

> Or, alternatively, add (... && ssl.security_style() ==
> SECURITY_STYLE_UNAUTHENTICATED) to line 164 since then we fall into the clause
> on line 171.

This won't work because the subsequent clauses starting on
line 171 need to be assured that ssl.cert_id() is valid
(nonzero).  That's the idea of this CL: we should access
ssl.security_bits() only for HTTPS (when ssl.cert_id() is
valid).

[Finnur, 2010-11-10 21:04:54]

LGTM