Add SecurityStateModelClient interface and implementation

Add SecurityStateModelClient interface and implementation

As a step along the way to componentizing SecurityStateModel, this CL
introduces a SecurityStateModelClient interface and a
ChromeSecurityStateModelClient implementation. The client has just a
couple methods for now but will expand such that the client will
eventually provide everything from //content and //chrome that the model
needs.

BUG=515071
Committed: https://crrev.com/13d440ca5df99f9c052c178b4dd53fa14a6a9dc3
Cr-Commit-Position: refs/heads/master@{#361371}

[estark, 2015-11-23 15:46:05]

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

[estark, 2015-11-23 15:46:05]

blundell, PTAL? This is the first in a series of CLs that will componentize
SecurityStateModel, as you suggested in crrev.com/1440303002

[blundell, 2015-11-23 16:06:17]

This is exactly the level of granularity I was looking for, thanks! Only real
question is about GetCertForSSLStatus().

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_security_state_model_delegate.h (right):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_security_state_model_delegate.h:21: //
SecurityStateModelDelegate overrides
nit:

// SecurityStateModelDelegate:

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
File chrome/browser/ssl/security_state_model.cc (left):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model.cc:226:
!GetCertForSSLStatus(visible_ssl_status_)) {
Should this method be going away in this CL?

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
File chrome/browser/ssl/security_state_model.h (right):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model.h:138: Profile* prfoile,
nit: I liked the spelling better before ;).

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model.h:156:
scoped_ptr<SecurityStateModelDelegate> delegate_;
nit: Let's add a comment here explaining how this will shortly be injected with
a TODO(estark).

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
File chrome/browser/ssl/security_state_model_delegate.h (right):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model_delegate.h:14: // Provides information
that a SecurityStateModel needs to determine the
"embedder-specific information"

[estark, 2015-11-23 16:23:40]

Thanks blundell!

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_security_state_model_delegate.h (right):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_security_state_model_delegate.h:21: //
SecurityStateModelDelegate overrides
On 2015/11/23 16:06:17, blundell wrote:
> nit:
> 
> // SecurityStateModelDelegate:

Done.

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
File chrome/browser/ssl/security_state_model.cc (left):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model.cc:226:
!GetCertForSSLStatus(visible_ssl_status_)) {
On 2015/11/23 16:06:17, blundell wrote:
> Should this method be going away in this CL?

Not quite yet, but it will be going away in one of the follow-up CLs. Here are
the gory details: GetCertForSSLStatus() is called from the static
SecurityInfoForRequest() on line 242, so it's still in use. However, because of
a recent, unrelated refactoring, the static SecurityInfoForRequest() method is
actually only needed for SecurityStateModel unit tests right now. Once I fill
out the SecurityStateModelDelegate interface, I'll be able to create a
TestSecurityStateModelDelegate in the unit tests and refactor the tests to use
that instead of relying on SecurityInfoForRequest() -- then I can get rid of
SecurityInfoForRequest() and GetCertForSSLStatus(). Does that make sense?

(In other words, right now the SSM unit tests construct content::SSLStatuses and
pass them in to SecurityInfoForRequest(), and soon I'll be able to instead
construct TestSSMDelegates and inject those into SSMs in the unit tests, as I
did in the original gigantic CL here:
https://codereview.chromium.org/1440303002/diff/280001/components/security_st...)

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
File chrome/browser/ssl/security_state_model.h (right):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model.h:138: Profile* prfoile,
On 2015/11/23 16:06:17, blundell wrote:
> nit: I liked the spelling better before ;).

Doh. Done.

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model.h:156:
scoped_ptr<SecurityStateModelDelegate> delegate_;
On 2015/11/23 16:06:17, blundell wrote:
> nit: Let's add a comment here explaining how this will shortly be injected
with
> a TODO(estark).

Done.

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
File chrome/browser/ssl/security_state_model_delegate.h (right):

https://codereview.chromium.org/1470813002/diff/1/chrome/browser/ssl/security...
chrome/browser/ssl/security_state_model_delegate.h:14: // Provides information
that a SecurityStateModel needs to determine the
On 2015/11/23 16:06:17, blundell wrote:
> "embedder-specific information"

Done.

[blundell, 2015-11-23 16:28:19]

lgtm

[estark, 2015-11-23 16:29:59]

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

[estark, 2015-11-23 16:29:59]

felt: PTAL?

This is the first in a series of steps to componentize SecurityStateModel. This
CL introduces a SSMDelegate interface and a ChromeSSMDelegate implementation
with just a couple small methods on it. In follow-up CLs I'll be extracting
embedder-specific logic with its content/ and chrome/ dependencies from SSM into
ChromeSSMDelegate, and then putting SSM into a component.

[felt, 2015-11-23 22:51:31]

blundell, a question for you brought to mind by this CL:

What's the difference between a Client and a Delegate? Both in practice are used
to abstract the dependencies required in a component. I see examples of both
naming conventions for very similar classes (e.g., BubbleDelegate and
FaviconClient).

[felt, 2015-11-23 23:21:00]

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

https://codereview.chromium.org/1470813002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_model.cc:242:
scoped_refptr<net::X509Certificate> cert = GetCertForSSLStatus(ssl);
Will this also need to be replaced with either RetrieveCert or a new arg?

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

https://codereview.chromium.org/1470813002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_model_delegate.h:26: // MITM certificate.
I think calling it a "MITM certificate" is confusing. It sounds like an invalid
certificate chain. Maybe moving all or some of the comment from the .cc file
would make this less confusing? I'd also recommend renaming it to something like
UsedPolicyInstalledCertificate.

[estark, 2015-11-24 00:07:23]

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

https://codereview.chromium.org/1470813002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_model.cc:242:
scoped_refptr<net::X509Certificate> cert = GetCertForSSLStatus(ssl);
On 2015/11/23 23:21:00, felt wrote:
> Will this also need to be replaced with either RetrieveCert or a new arg?

Oh, I guess I could just add it an arg and then delete GetCertForSSLStatus(). I
went ahead and did that. In a follow-up CL, I'll be able to remove this whole
method.

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

https://codereview.chromium.org/1470813002/diff/40001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_model_delegate.h:26: // MITM certificate.
On 2015/11/23 23:21:00, felt wrote:
> I think calling it a "MITM certificate" is confusing. It sounds like an
invalid
> certificate chain. Maybe moving all or some of the comment from the .cc file
> would make this less confusing? I'd also recommend renaming it to something
like
> UsedPolicyInstalledCertificate.

Done.

[felt, 2015-11-24 00:09:00]

lgtm

[estark, 2015-11-24 00:18:31]

The CQ bit was checked by estark@chromium.org

[estark, 2015-11-24 00:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1470813002/#ps60001
(title: "felt comments")

[commit-bot: I haz the power, 2015-11-24 00:20:36]

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

[commit-bot: I haz the power, 2015-11-24 03:16:54]

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

[commit-bot: I haz the power, 2015-11-24 03:16:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2015-11-24 04:00:33]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-11-24 04:03:55]

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

[commit-bot: I haz the power, 2015-11-24 05:36:31]

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

[commit-bot: I haz the power, 2015-11-24 05:36:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blundell, 2015-11-24 12:02:57]

On 2015/11/23 22:51:31, felt wrote:
> blundell, a question for you brought to mind by this CL:
> 
> What's the difference between a Client and a Delegate? Both in practice are
used
> to abstract the dependencies required in a component. I see examples of both
> naming conventions for very similar classes (e.g., BubbleDelegate and
> FaviconClient).

It's a good question. Actually, the usage of Delegate in this case is
technically incorrect -- sloppiness on my part.

With respect to //components, we should be using Client for interfaces that are
implemented in the embedder (although as you point out there are some interfaces
that don't adhere to this for historical reasons, lack of awareness, or
whatever). So yeah, we should actually change this to Client instead of
Delegate. Can do that at any time that's convenient (or least inconvenient) to
make the change. Thanks for pointing it out!

[estark, 2015-11-24 15:09:43]

On 2015/11/24 12:02:57, blundell wrote:
> On 2015/11/23 22:51:31, felt wrote:
> > blundell, a question for you brought to mind by this CL:
> > 
> > What's the difference between a Client and a Delegate? Both in practice are
> used
> > to abstract the dependencies required in a component. I see examples of both
> > naming conventions for very similar classes (e.g., BubbleDelegate and
> > FaviconClient).
> 
> It's a good question. Actually, the usage of Delegate in this case is
> technically incorrect -- sloppiness on my part.
> 
> With respect to //components, we should be using Client for interfaces that
are
> implemented in the embedder (although as you point out there are some
interfaces
> that don't adhere to this for historical reasons, lack of awareness, or
> whatever). So yeah, we should actually change this to Client instead of
> Delegate. Can do that at any time that's convenient (or least inconvenient) to
> make the change. Thanks for pointing it out!

Ok, thanks for the explanation! I'll go ahead and make that change in this CL
since it's stuck on a sick bot anyway. (Pretty sure those chromeos test failures
aren't because of this CL, as I have another unrelated CL that's seeing the
exact same failures.)

[blundell, 2015-11-24 15:10:58]

On 2015/11/24 15:09:43, estark wrote:
> On 2015/11/24 12:02:57, blundell wrote:
> > On 2015/11/23 22:51:31, felt wrote:
> > > blundell, a question for you brought to mind by this CL:
> > > 
> > > What's the difference between a Client and a Delegate? Both in practice
are
> > used
> > > to abstract the dependencies required in a component. I see examples of
both
> > > naming conventions for very similar classes (e.g., BubbleDelegate and
> > > FaviconClient).
> > 
> > It's a good question. Actually, the usage of Delegate in this case is
> > technically incorrect -- sloppiness on my part.
> > 
> > With respect to //components, we should be using Client for interfaces that
> are
> > implemented in the embedder (although as you point out there are some
> interfaces
> > that don't adhere to this for historical reasons, lack of awareness, or
> > whatever). So yeah, we should actually change this to Client instead of
> > Delegate. Can do that at any time that's convenient (or least inconvenient)
to
> > make the change. Thanks for pointing it out!
> 
> Ok, thanks for the explanation! I'll go ahead and make that change in this CL
> since it's stuck on a sick bot anyway. (Pretty sure those chromeos test
failures
> aren't because of this CL, as I have another unrelated CL that's seeing the
> exact same failures.)

//tools/git/move_source_file.py, //tools/git/mffr.py, and "git cl format" are
super-useful for a task like this.

[estark, 2015-11-24 16:04:39]

Description was changed from

==========
Add SecurityStateModelDelegate interface and implementation

As a step along the way to componentizing SecurityStateModel, this CL
introduces a SecurityStateModelDelegate interface and a
ChromeSecurityStateModelDelegate implementation. The delegate has just a
couple methods for now but will expand such that the delegate will
eventually provide everything from //content and //chrome that the model
needs.

BUG=515071
==========

to

==========
Add SecurityStateModelClient interface and implementation

As a step along the way to componentizing SecurityStateModel, this CL
introduces a SecurityStateModelClient interface and a
ChromeSecurityStateModelClient implementation. The client has just a
couple methods for now but will expand such that the client will
eventually provide everything from //content and //chrome that the model
needs.

BUG=515071
==========

[estark, 2015-11-24 16:07:04]

The CQ bit was checked by estark@chromium.org

[estark, 2015-11-24 16:07:04]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1470813002/#ps100001
(title: "delegate_ -> client_")

[commit-bot: I haz the power, 2015-11-24 16:08:04]

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

[commit-bot: I haz the power, 2015-11-24 17:01:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-24 17:02:09]

Patchset 6 (id:??) landed as
https://crrev.com/13d440ca5df99f9c052c178b4dd53fa14a6a9dc3
Cr-Commit-Position: refs/heads/master@{#361371}