Move SecurityStyleChanged logic and tests to chrome/browser/ssl

Move SecurityStyleChanged logic and tests to chrome/browser/ssl

Browser::GetSecurityStyle() does a whole bunch of stuff to compute
explanations about the SSL status of the page. This CL moves this logic
into a method of ChromeSecurityStateModelClient, in chrome/browser/ssl,
and its tests as well. The code fits better there since it expresses a
lot of Chrome-specific SSL logic.

BUG=604339
Committed: https://crrev.com/09dbe8e1141646f0e6168179daded505daf34c92
Cr-Commit-Position: refs/heads/master@{#395197}

[estark, 2016-05-17 20:38:48]

estark@chromium.org changed reviewers:
+ msw@chromium.org, palmer@chromium.org

[estark, 2016-05-17 20:38:49]

msw: can you please review chrome/browser/ui?
palmer: can you please review chrome/browser/ssl?

This shuffles some code from Browser to ChromeSecurityStateModelClient, where I
think it makes a lot more sense.

[palmer, 2016-05-17 22:37:29]

lgtm

[msw, 2016-05-18 17:20:24]

lgtm with nits and qs.

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:92:
content::WebContents* web_contents,
nit: remove |web_contents| if it isn't used (use |web_contents_| if needed?).

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:94: const
SecurityStateModel::SecurityInfo& security_info = GetSecurityInfo();
optional nit: If this is the only instance data needed, perhaps that could be
passed into a static (class or namespace) function?

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:43:
#include "net/cert/x509_certificate.h"
nit: remove (duplicate of line above)

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:666:
SetUpMockCertVerifierForHttpsServer(0, net::OK);
q: Is this necessary? It wasn't part of the old test, afaict. (ditto elsewhere)

[estark, 2016-05-20 22:12:09]

Thanks!

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:92:
content::WebContents* web_contents,
On 2016/05/18 17:20:24, msw wrote:
> nit: remove |web_contents| if it isn't used (use |web_contents_| if needed?).

Done.

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client.cc:94: const
SecurityStateModel::SecurityInfo& security_info = GetSecurityInfo();
On 2016/05/18 17:20:24, msw wrote:
> optional nit: If this is the only instance data needed, perhaps that could be
> passed into a static (class or namespace) function?

Done.

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:43:
#include "net/cert/x509_certificate.h"
On 2016/05/18 17:20:24, msw wrote:
> nit: remove (duplicate of line above)

Done.

https://codereview.chromium.org/1986953002/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:666:
SetUpMockCertVerifierForHttpsServer(0, net::OK);
On 2016/05/18 17:20:24, msw wrote:
> q: Is this necessary? It wasn't part of the old test, afaict. (ditto
elsewhere)

Ehh, I did it to be consistent with the rest of the tests in this file, but I
guess most of the other tests actually have a reason to use a mock CertVerifier,
and I don't really have a reason here. Removed.

[estark, 2016-05-20 22:17:07]

The CQ bit was checked by estark@chromium.org

[estark, 2016-05-20 22:17:08]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1986953002/#ps60001
(title: "minor cleanup")

[commit-bot: I haz the power, 2016-05-20 22:17:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986953002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986953002/60001

[commit-bot: I haz the power, 2016-05-20 23:17:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 23:19:00]

Description was changed from

==========
Move SecurityStyleChanged logic and tests to chrome/browser/ssl

Browser::GetSecurityStyle() does a whole bunch of stuff to compute
explanations about the SSL status of the page. This CL moves this logic
into a method of ChromeSecurityStateModelClient, in chrome/browser/ssl,
and its tests as well. The code fits better there since it expresses a
lot of Chrome-specific SSL logic.

BUG=604339
==========

to

==========
Move SecurityStyleChanged logic and tests to chrome/browser/ssl

Browser::GetSecurityStyle() does a whole bunch of stuff to compute
explanations about the SSL status of the page. This CL moves this logic
into a method of ChromeSecurityStateModelClient, in chrome/browser/ssl,
and its tests as well. The code fits better there since it expresses a
lot of Chrome-specific SSL logic.

BUG=604339

Committed: https://crrev.com/09dbe8e1141646f0e6168179daded505daf34c92
Cr-Commit-Position: refs/heads/master@{#395197}
==========

[commit-bot: I haz the power, 2016-05-20 23:19:01]

Patchset 4 (id:??) landed as
https://crrev.com/09dbe8e1141646f0e6168179daded505daf34c92
Cr-Commit-Position: refs/heads/master@{#395197}