Refactor SecurityStateModel/Clients for simplicity and reusability.

Refactor SecurityStateModel/Clients for simplicity and reusability.

Primary purpose for this patch is to make content-specific parts of
ChromeSecurityStateModelClient reusable for use in other embedders,
e.g. in headless chrome to support DevTool's Security domain.

The patch replaces the SecurityStateModel class (and its Client
interface) with helper methods, and moves content-specific logic
from CSSMC into new helper methods in the security_state component.

BUG=609394
Committed: https://crrev.com/cac56b6adc739e3ba3d5a20a45c0da87fec9d02a
Cr-Commit-Position: refs/heads/master@{#432474}

[Eric Seckler, 2016-10-25 10:11:56]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2016-10-25 10:12:12]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 10:12:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 10:34:58]

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

[commit-bot: I haz the power, 2016-10-25 10:34:59]

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

[Eric Seckler, 2016-10-25 10:44:46]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 10:44:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 11:05:54]

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

[commit-bot: I haz the power, 2016-10-25 11:05:55]

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

[Eric Seckler, 2016-10-25 11:23:16]

Patchset #2 (id:40001) has been deleted

[Eric Seckler, 2016-10-25 11:23:27]

Patchset #1 (id:20001) has been deleted

[Eric Seckler, 2016-10-25 11:23:48]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 11:24:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-10-25 11:58:06]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 11:58:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-10-25 12:42:54]

Patchset #1 (id:60001) has been deleted

[Eric Seckler, 2016-10-25 12:55:49]

Description was changed from

==========
Make content-specific parts of ChromeSecurityStateModelClient reusable.

Extracts parts of CSSMC into the security_state component, to make it
accessible to other embedders of content/. This is necessary to support
DevTool's Security domain in headless chrome. (We will add this support
in a follow-up patch.)

BUG=609394
==========

to

==========
Make content-specific parts of ChromeSecurityStateModelClient reusable.

Extracts parts of CSSMC into the security_state component, to make it
accessible to other embedders of content/. This is necessary to support
DevTool's Security domain in headless chrome. (We will add this support
in a follow-up patch.)

The patch introduces WebContentsSecurityStateModelClient as a new
superclass of CSSMC. WCSSMC resides in
/components/security_state/content. Sources previously under
/components/security_state (shared between iOS/content) are moved to
/components/security_state/core. UI strings now used by WCSSMC are
moved into .grd files in /components/.

BUG=609394
==========

[Eric Seckler, 2016-10-25 12:57:48]

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

[Eric Seckler, 2016-10-25 12:57:49]

Hi Emily,

We're looking to make the DevTools security domain compatible with headless
chrome (https://cs.chromium.org/chromium/src/headless/README.md). Headless is an
embedder of /content/ rather than /chrome/. As a result, it doesn't currently
have access to the logic in ChromeSecurityStateModelClient that computes the
security state for DevTools' Security.securityStateChanged event (see linked
bug).

This patch moves things around, so we can access this logic from headless.

Thanks for taking a look!
Eric

[commit-bot: I haz the power, 2016-10-25 13:59:08]

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

[commit-bot: I haz the power, 2016-10-25 13:59:08]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-10-25 16:24:01]

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

[blundell, 2016-10-25 16:24:01]

https://codereview.chromium.org/2448943002/diff/80001/components/security_sta...
File
components/security_state/content/web_contents_security_state_model_client.h
(right):

https://codereview.chromium.org/2448943002/diff/80001/components/security_sta...
components/security_state/content/web_contents_security_state_model_client.h:25:
class WebContentsSecurityStateModelClient : public SecurityStateModelClient {
drive-by nit: The Client naming signifies an interface that is implemented by
the embedder, so we shouldn't put a partial implementation of a Client interface
within a component itself. I suggest just making this class a helper class that
can be shared between the Chrome client and the new Headless client.

[Eric Seckler, 2016-10-25 16:43:59]

https://codereview.chromium.org/2448943002/diff/80001/components/security_sta...
File
components/security_state/content/web_contents_security_state_model_client.h
(right):

https://codereview.chromium.org/2448943002/diff/80001/components/security_sta...
components/security_state/content/web_contents_security_state_model_client.h:25:
class WebContentsSecurityStateModelClient : public SecurityStateModelClient {
On 2016/10/25 16:24:01, blundell wrote:
> drive-by nit: The Client naming signifies an interface that is implemented by
> the embedder, so we shouldn't put a partial implementation of a Client
interface
> within a component itself. I suggest just making this class a helper class
that
> can be shared between the Chrome client and the new Headless client.

security_state/content/ basically is the content implementation of the
component, though (as compared to the iOS one). Thus, I'd argue that
implementing the components' client interface should be OK? It sounds like it
should from
https://www.chromium.org/developers/design-documents/layered-components-design .
But since this is my first exposure to components, I might be wrong :)

[blundell, 2016-10-25 17:03:58]

On 2016/10/25 16:43:59, Eric Seckler wrote:
>
https://codereview.chromium.org/2448943002/diff/80001/components/security_sta...
> File
> components/security_state/content/web_contents_security_state_model_client.h
> (right):
> 
>
https://codereview.chromium.org/2448943002/diff/80001/components/security_sta...
>
components/security_state/content/web_contents_security_state_model_client.h:25:
> class WebContentsSecurityStateModelClient : public SecurityStateModelClient {
> On 2016/10/25 16:24:01, blundell wrote:
> > drive-by nit: The Client naming signifies an interface that is implemented
by
> > the embedder, so we shouldn't put a partial implementation of a Client
> interface
> > within a component itself. I suggest just making this class a helper class
> that
> > can be shared between the Chrome client and the new Headless client.
> 
> security_state/content/ basically is the content implementation of the
> component, though (as compared to the iOS one). Thus, I'd argue that
> implementing the components' client interface should be OK? It sounds like it
> should from
> https://www.chromium.org/developers/design-documents/layered-components-design
.
> But since this is my first exposure to components, I might be wrong :)

The embedder is at the level of the end product (e.g., Headless or Chrome). The
code within the component is not at the embedder level but rather for the use of
embedders; e.g., //components/security_state/content is available for use by all
embedders who use the Content API. It's a subtle distinction I agree, but it's
useful to avoid diluting the meaning of these terms.

[estark, 2016-10-25 22:49:53]

I haven't reviewed too carefully yet; I think I agree with blundell's point. To
me it seems strange to have a fairly broad client interface that's implemented
almost entirely within the component itself.

Here's a proposal that seems more natural to me:

- WebContentsSecurityStateModel (in components/security_state/content) has a
SecurityStateModel. WebContentsSSM::GetSecurityInfo() calls
SSM::GetSecurityInfo() and passes it whatever info it needs.

- WebContentsSSM has a SecurityStateModelClient, with the following interface:
  - UsedPolicyInstalledCertificate()
  - GetMalwareStatus()

- ChromeSSMClient has a WebContentsSSM (instead of the SSM that it has now).

[Eric Seckler, 2016-10-26 10:29:18]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 10:29:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 10:33:21]

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

[commit-bot: I haz the power, 2016-10-26 10:33:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2016-10-26 12:05:37]

Patchset #2 (id:100001) has been deleted

[Eric Seckler, 2016-10-26 12:05:55]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 12:06:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-10-26 12:18:32]

On 2016/10/25 22:49:53, estark wrote:
> I haven't reviewed too carefully yet; I think I agree with blundell's point.
To
> me it seems strange to have a fairly broad client interface that's implemented
> almost entirely within the component itself.
> 
> Here's a proposal that seems more natural to me:
> 
> - WebContentsSecurityStateModel (in components/security_state/content) has a
> SecurityStateModel. WebContentsSSM::GetSecurityInfo() calls
> SSM::GetSecurityInfo() and passes it whatever info it needs.
> 
> - WebContentsSSM has a SecurityStateModelClient, with the following interface:
>   - UsedPolicyInstalledCertificate()
>   - GetMalwareStatus()
> 
> - ChromeSSMClient has a WebContentsSSM (instead of the SSM that it has now).

Thanks! PTAL, I've updated the patch - hope I'm understanding this correctly :)

I'm wondering whether it's worth detangling the ChromeSecurityStateModelClient
browser tests and moving WebContentsSecurityStateModel-related parts into to
security_state/content/. WDYT?

[commit-bot: I haz the power, 2016-10-26 12:52:12]

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

[commit-bot: I haz the power, 2016-10-26 12:52:13]

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

[Eric Seckler, 2016-10-26 14:19:44]

Patchset #2 (id:120001) has been deleted

[Eric Seckler, 2016-10-26 14:19:58]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 14:20:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 15:43:05]

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

[commit-bot: I haz the power, 2016-10-26 15:43:06]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-10-26 18:30:47]

https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
File components/security_state/content/web_contents_security_state_model.cc
(right):

https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
components/security_state/content/web_contents_security_state_model.cc:140:
class WebContentsSecurityStateModel::Client : public SecurityStateModelClient {
This is still implementing the core Client interface within the component. I
would do the following:

WebContentsSecurityStateModel (which I would personally rename to something like
WebContentsSecurityStateModelHelper since it has-a SecurityStateModel rather
than is-a) doesn't implement SecurityStateModelClient at all. Instead it has
parallel interfaces for the functionality that you want to share between the
Chrome and Headless impls.

ChromeSecurityStateModelClient has a WebContentsSecurityStateModelHelper
instance and simply delegates its impls of ChromeSecurityStateModelClient
methods to that interface as necessary. For the functionality that's
embedder-specific it would naturally still be completely contained in
ChromeSecurityStateModelClient (rather than needing to call out to
model_->client from within the component as you're doing here).

This is a little different from what Emily has suggested, but it avoids needing
two separate Client interfaces and seems like the most natural structure to me.

[estark, 2016-10-26 19:13:01]

On 2016/10/26 18:30:47, blundell wrote:
>
https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
> File components/security_state/content/web_contents_security_state_model.cc
> (right):
> 
>
https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
> components/security_state/content/web_contents_security_state_model.cc:140:
> class WebContentsSecurityStateModel::Client : public SecurityStateModelClient
{
> This is still implementing the core Client interface within the component. I
> would do the following:
> 
> WebContentsSecurityStateModel (which I would personally rename to something
like
> WebContentsSecurityStateModelHelper since it has-a SecurityStateModel rather
> than is-a) doesn't implement SecurityStateModelClient at all. Instead it has
> parallel interfaces for the functionality that you want to share between the
> Chrome and Headless impls.
> 
> ChromeSecurityStateModelClient has a WebContentsSecurityStateModelHelper
> instance and simply delegates its impls of ChromeSecurityStateModelClient
> methods to that interface as necessary. For the functionality that's
> embedder-specific it would naturally still be completely contained in
> ChromeSecurityStateModelClient (rather than needing to call out to
> model_->client from within the component as you're doing here).
> 
> This is a little different from what Emily has suggested, but it avoids
needing
> two separate Client interfaces and seems like the most natural structure to
me.

FWIW I wasn't suggesting two Client interfaces, but rather that
ChromeSSMClient::GetSecurityInfo() calls WebContentsSSM::GetSecurityInfo(),
which calls SSM:GetSecurityInfo(client->GetVisibleSecurityState(),
client->UsedPolicyInstalledCertificate(), client->GetMalwareStatus()), so that
SSM doesn't need a client at all.

This is all rather confused by the fact that I don't think we actually need a
client interface at all. ChromeSSLClient::GetSecurityInfo() could simply pass
GetMalwareStatus() and UsedPolicyInstalledCertificate() directly to
WebContentsSSM::GetSecurityInfo(), which could pass those values on to
SSM::GetSecurityInfo(), such that SSM doesn't need to call on a client at all.

[Eric Seckler, 2016-10-26 19:40:13]

On 2016/10/26 19:13:01, estark wrote:
> On 2016/10/26 18:30:47, blundell wrote:
> >
>
https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
> > File components/security_state/content/web_contents_security_state_model.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
> > components/security_state/content/web_contents_security_state_model.cc:140:
> > class WebContentsSecurityStateModel::Client : public
SecurityStateModelClient
> {
> > This is still implementing the core Client interface within the component. I
> > would do the following:
> > 
> > WebContentsSecurityStateModel (which I would personally rename to something
> like
> > WebContentsSecurityStateModelHelper since it has-a SecurityStateModel rather
> > than is-a) doesn't implement SecurityStateModelClient at all. Instead it has
> > parallel interfaces for the functionality that you want to share between the
> > Chrome and Headless impls.
> > 
> > ChromeSecurityStateModelClient has a WebContentsSecurityStateModelHelper
> > instance and simply delegates its impls of ChromeSecurityStateModelClient
> > methods to that interface as necessary. For the functionality that's
> > embedder-specific it would naturally still be completely contained in
> > ChromeSecurityStateModelClient (rather than needing to call out to
> > model_->client from within the component as you're doing here).
> > 
> > This is a little different from what Emily has suggested, but it avoids
> needing
> > two separate Client interfaces and seems like the most natural structure to
> me.
> 
> FWIW I wasn't suggesting two Client interfaces, but rather that
> ChromeSSMClient::GetSecurityInfo() calls WebContentsSSM::GetSecurityInfo(),
> which calls SSM:GetSecurityInfo(client->GetVisibleSecurityState(),
> client->UsedPolicyInstalledCertificate(), client->GetMalwareStatus()), so that
> SSM doesn't need a client at all.
> 
> This is all rather confused by the fact that I don't think we actually need a
> client interface at all. ChromeSSLClient::GetSecurityInfo() could simply pass
> GetMalwareStatus() and UsedPolicyInstalledCertificate() directly to
> WebContentsSSM::GetSecurityInfo(), which could pass those values on to
> SSM::GetSecurityInfo(), such that SSM doesn't need to call on a client at all.

OK, that makes more sense now. Thanks for the thorough explanation :)

Happy to remove the clients - it might require some changes to the tests,
though.
This means we'd also replace IOSChromeSecurityStateModelClient by a
IOSChromeSecurityStateModel and remove the SecurityStateModelClient completely,
right?

i.e. we'd have in chrome (and analogous in headless):

ChromeSecurityStateModel::GetSecurityInfo(result) ->
WebContentsSecurityStateModel::GetSecurityInfo(result,
used_policy_installed_certificate, malware_status) ->
SecurityStateModel::GetSecurityInfo(result, visible_state,
used_policy_installed_certificate)

and on iOS:

IOSChromeSecurityStateModel::GetSecurityInfo(result) ->
SecurityStateModel::GetSecurityInfo(result, visible_state,
used_policy_installed_certificate)

How would we make what is currently
SecurityStateModelClient::IsOriginSecure(url) available to SSM?

[Eric Seckler, 2016-10-27 15:12:17]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 15:12:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-10-27 15:14:41]

On 2016/10/26 19:40:13, Eric Seckler wrote:
> On 2016/10/26 19:13:01, estark wrote:
> > On 2016/10/26 18:30:47, blundell wrote:
> > >
> >
>
https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
> > > File
components/security_state/content/web_contents_security_state_model.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2448943002/diff/140001/components/security_st...
> > >
components/security_state/content/web_contents_security_state_model.cc:140:
> > > class WebContentsSecurityStateModel::Client : public
> SecurityStateModelClient
> > {
> > > This is still implementing the core Client interface within the component.
I
> > > would do the following:
> > > 
> > > WebContentsSecurityStateModel (which I would personally rename to
something
> > like
> > > WebContentsSecurityStateModelHelper since it has-a SecurityStateModel
rather
> > > than is-a) doesn't implement SecurityStateModelClient at all. Instead it
has
> > > parallel interfaces for the functionality that you want to share between
the
> > > Chrome and Headless impls.
> > > 
> > > ChromeSecurityStateModelClient has a WebContentsSecurityStateModelHelper
> > > instance and simply delegates its impls of ChromeSecurityStateModelClient
> > > methods to that interface as necessary. For the functionality that's
> > > embedder-specific it would naturally still be completely contained in
> > > ChromeSecurityStateModelClient (rather than needing to call out to
> > > model_->client from within the component as you're doing here).
> > > 
> > > This is a little different from what Emily has suggested, but it avoids
> > needing
> > > two separate Client interfaces and seems like the most natural structure
to
> > me.
> > 
> > FWIW I wasn't suggesting two Client interfaces, but rather that
> > ChromeSSMClient::GetSecurityInfo() calls WebContentsSSM::GetSecurityInfo(),
> > which calls SSM:GetSecurityInfo(client->GetVisibleSecurityState(),
> > client->UsedPolicyInstalledCertificate(), client->GetMalwareStatus()), so
that
> > SSM doesn't need a client at all.
> > 
> > This is all rather confused by the fact that I don't think we actually need
a
> > client interface at all. ChromeSSLClient::GetSecurityInfo() could simply
pass
> > GetMalwareStatus() and UsedPolicyInstalledCertificate() directly to
> > WebContentsSSM::GetSecurityInfo(), which could pass those values on to
> > SSM::GetSecurityInfo(), such that SSM doesn't need to call on a client at
all.
> 
> OK, that makes more sense now. Thanks for the thorough explanation :)
> 
> Happy to remove the clients - it might require some changes to the tests,
> though.
> This means we'd also replace IOSChromeSecurityStateModelClient by a
> IOSChromeSecurityStateModel and remove the SecurityStateModelClient
completely,
> right?
> 
> i.e. we'd have in chrome (and analogous in headless):
> 
> ChromeSecurityStateModel::GetSecurityInfo(result) ->
> WebContentsSecurityStateModel::GetSecurityInfo(result,
> used_policy_installed_certificate, malware_status) ->
> SecurityStateModel::GetSecurityInfo(result, visible_state,
> used_policy_installed_certificate)
> 
> and on iOS:
> 
> IOSChromeSecurityStateModel::GetSecurityInfo(result) ->
> SecurityStateModel::GetSecurityInfo(result, visible_state,
> used_policy_installed_certificate)
> 
> How would we make what is currently
> SecurityStateModelClient::IsOriginSecure(url) available to SSM?

Uploaded a quick draft of what this might look like. Is this what you were
thinking of, Emily?

Thanks again!

[commit-bot: I haz the power, 2016-10-27 15:16:12]

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

[commit-bot: I haz the power, 2016-10-27 15:16:13]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2016-10-28 00:28:13]

Sorry for the delay. This approach looks reasonable to me. Before I review in
more detail I'm curious what blundell thinks?

Two high-level comments:

1. I'm not sure about the names; as blundell pointed out earlier in the thread,
having a class named WebContentsSecurityStateModel would usually imply that it's
the WebContents implementation of a SecurityStateModel interface, which isn't
the case here. (And same confusion for ChromeSecurityStateModel.) Maybe
ChromeSecurityStateModel should have a WebContentsSecurityStateHelper which
should call a static security_state::GetSecurityInfo method? (Or, it might make
sense to leave it as SecurityStateModel::GetSecurityInfo for now and change it
to a standalone method in a follow-up CL since this is so large already.)

2. It would probably make sense to move the ChromeSSMClient tests into
compontents/security_state/content since that's where the logic that they are
testing lives. However, that could be a follow-up CL too.

[blundell, 2016-10-28 21:59:47]

Hey,

This looks good, thanks! Here's how I suggest resolving the naming issue:

- WebContentsSecurityStateModel is essentially stateless. I think we should have
it be a collection of utility functions (security_state_content_utils.*), with
the one method that needs a WebContents just taking it as an input.

- ChromeSecurityStateModel is basically a tab helper
(https://chromium.googlesource.com/chromium/src/+show/master/docs/tab_helpers.md).
I suggest we rename it SecurityStateTabHelper. It would directly own a
security_state::SecurityStateModel and would make use of the functions in
security_state_content_utils.* as necessary in calling into its
security_state::SecurityStateModel instance.

How does that sound?

https://codereview.chromium.org/2448943002/diff/160001/components/security_st...
File components/security_state/content/web_contents_security_state_model.cc
(right):

https://codereview.chromium.org/2448943002/diff/160001/components/security_st...
components/security_state/content/web_contents_security_state_model.cc:141: bool
IsOriginSecure(const GURL& url) {
Let's make this dependence explicit by having SecurityStateModel take in a
base::Callback that tells whether the origin is secure.

[Eric Seckler, 2016-11-01 10:58:56]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 10:59:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 11:03:14]

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

[commit-bot: I haz the power, 2016-11-01 11:03:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2016-11-01 11:22:44]

Patchset #4 (id:180001) has been deleted

[Eric Seckler, 2016-11-01 11:22:56]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 11:23:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 11:27:06]

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

[commit-bot: I haz the power, 2016-11-01 11:27:10]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Eric Seckler, 2016-11-01 11:46:54]

Patchset #4 (id:200001) has been deleted

[Eric Seckler, 2016-11-01 12:00:15]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 12:00:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-01 12:08:30]

Patchset #4 (id:220001) has been deleted

[Eric Seckler, 2016-11-01 12:10:18]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 12:10:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 12:30:46]

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

[commit-bot: I haz the power, 2016-11-01 12:30:47]

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

[Eric Seckler, 2016-11-01 13:59:50]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 14:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 14:23:19]

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

[commit-bot: I haz the power, 2016-11-01 14:23:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Eric Seckler, 2016-11-01 15:33:09]

Patchset #4 (id:240001) has been deleted

[Eric Seckler, 2016-11-01 16:21:06]

Patchset #4 (id:260001) has been deleted

[Eric Seckler, 2016-11-01 16:21:36]

Patchset #4 (id:260001) has been deleted

[Eric Seckler, 2016-11-01 16:21:47]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 16:22:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-01 16:46:36]

Thanks for the suggestions, all addressed.

However, I'm observing an issue with the added localized strings that only
surfaces in the ios_chrome_unit_tests on the ios simulator builders. It
manifests as error in ssl_errors::ErrorInfo::CreateError, see e.g. logs here:

https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu...

It seems that for a given resource ID the test retrieves the "old" message that
had this ID before my change (IDS_SUPERVISED_USER_BLOCK_HEADER_DEFAULT), rather
than the message that now has this ID
(IDS_CERT_ERROR_AUTHORITY_INVALID_DETAILS).

Do you have an intuition what might be going wrong here? I'm not really familiar
with Chromium's resource management, maybe I'm adding the strings incorrectly or
we're missing some dependency on the resources for the ios_chrome_unit_tests
target? The weird thing is that it worked fine for patch set 2.

https://codereview.chromium.org/2448943002/diff/160001/components/security_st...
File components/security_state/content/web_contents_security_state_model.cc
(right):

https://codereview.chromium.org/2448943002/diff/160001/components/security_st...
components/security_state/content/web_contents_security_state_model.cc:141: bool
IsOriginSecure(const GURL& url) {
On 2016/10/28 21:59:46, blundell wrote:
> Let's make this dependence explicit by having SecurityStateModel take in a
> base::Callback that tells whether the origin is secure.

Done.

[commit-bot: I haz the power, 2016-11-01 16:47:47]

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

[commit-bot: I haz the power, 2016-11-01 16:47:48]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[estark, 2016-11-01 16:58:55]

(haven't reviewed yet)

On 2016/11/01 16:46:36, Eric Seckler wrote:
> Thanks for the suggestions, all addressed.
> 
> However, I'm observing an issue with the added localized strings that only
> surfaces in the ios_chrome_unit_tests on the ios simulator builders. It
> manifests as error in ssl_errors::ErrorInfo::CreateError, see e.g. logs here:
> 
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu...

There's an ongoing issue with ios_chrome_unittests, see
https://crbug.com/660343. I think it's some weird infra problem...

> 
> It seems that for a given resource ID the test retrieves the "old" message
that
> had this ID before my change (IDS_SUPERVISED_USER_BLOCK_HEADER_DEFAULT),
rather
> than the message that now has this ID
> (IDS_CERT_ERROR_AUTHORITY_INVALID_DETAILS).
> 
> Do you have an intuition what might be going wrong here? I'm not really
familiar
> with Chromium's resource management, maybe I'm adding the strings incorrectly
or
> we're missing some dependency on the resources for the ios_chrome_unit_tests
> target? The weird thing is that it worked fine for patch set 2.
> 
>
https://codereview.chromium.org/2448943002/diff/160001/components/security_st...
> File components/security_state/content/web_contents_security_state_model.cc
> (right):
> 
>
https://codereview.chromium.org/2448943002/diff/160001/components/security_st...
> components/security_state/content/web_contents_security_state_model.cc:141:
bool
> IsOriginSecure(const GURL& url) {
> On 2016/10/28 21:59:46, blundell wrote:
> > Let's make this dependence explicit by having SecurityStateModel take in a
> > base::Callback that tells whether the origin is secure.
> 
> Done.

[Eric Seckler, 2016-11-01 17:00:30]

On 2016/10/28 00:28:13, estark wrote:
> 2. It would probably make sense to move the ChromeSSMClient tests into
> compontents/security_state/content since that's where the logic that they are
> testing lives. However, that could be a follow-up CL too.

I moved one of the browser tests into the component, because it is only based on
VisibleSecurityState logic that's in the content_utils now. I'd suggest to
consider moving part of the remaining ones separately. Some of them intend to
test logic confined to content_utils, but need to be detangled from the tab
helper.

[Eric Seckler, 2016-11-01 17:07:10]

On 2016/11/01 16:58:55, estark wrote:
> There's an ongoing issue with ios_chrome_unittests, see
> https://crbug.com/660343. I think it's some weird infra problem...

Aaah, thanks for the quick shout! :)

[blundell, 2016-11-01 23:27:44]

This looks good from my POV. Thanks! I'll be OOO the rest of the week, so
consider this my lg.

[blundell, 2016-11-01 23:29:01]

(Note that I didn't review the content of the refactoring in detail but just the
form :).

https://codereview.chromium.org/2448943002/diff/280001/components/security_st...
File components/security_state/content/content_utils.h (right):

https://codereview.chromium.org/2448943002/diff/280001/components/security_st...
components/security_state/content/content_utils.h:18: namespace
security_state_content_utils {
nit: this should just be the namespace of the component, i.e., security_state.

[estark, 2016-11-02 05:22:03]

Sorry I didn't quite get to this today; I did a quick pass over the non-ios
parts and I'll take a closer look tomorrow morning.

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper.h:22: // Uses a WebContents to
provide a SecurityStateModel with the
Can you update this comment, please?

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper.h:53: // True if a console has been
logged about an omnibox warning that
Sorry, I know this typo was here before, but while you're here, there's a word
missing ("console" => "console message"), would you mind fixing it?

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.h:13: #include
"components/security_state/core/security_state_model.h"
Looks like you could just delete this and forward-declare SecurityInfo instead.

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
chrome/browser/ui/browser_dialogs.h:15: #include
"components/security_state/core/security_state_model.h"
same here

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:19: #include
"components/security_state/core/security_state_model.h"
and here

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.h:16: #include
"components/security_state/core/security_state_model.h"
and here

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:23: #include
"components/security_state/core/security_state_model.h"
Ok I'm going to stop saying "and here", you get the idea :P

[Eric Seckler, 2016-11-02 09:56:53]

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper.h:22: // Uses a WebContents to
provide a SecurityStateModel with the
On 2016/11/02 05:22:02, estark wrote:
> Can you update this comment, please?

Done. Let me know if you had something else in mind, though :)

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper.h:53: // True if a console has been
logged about an omnibox warning that
On 2016/11/02 05:22:02, estark wrote:
> Sorry, I know this typo was here before, but while you're here, there's a word
> missing ("console" => "console message"), would you mind fixing it?

Done.

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.h:13: #include
"components/security_state/core/security_state_model.h"
On 2016/11/02 05:22:02, estark wrote:
> Looks like you could just delete this and forward-declare SecurityInfo
instead.

I think that only works if we move SecurityInfo out of SecurityStateModel (it's
a nested class at the moment). WDYT?

[estark, 2016-11-03 04:38:46]

LGTM. Just a couple more notes and nits inline. Also could you please update the
CL description to describe the new approach?

Thanks for doing this and for getting rid of the unnecessary client interface
along the way!

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.h:13: #include
"components/security_state/core/security_state_model.h"
On 2016/11/02 09:56:53, Eric Seckler wrote:
> On 2016/11/02 05:22:02, estark wrote:
> > Looks like you could just delete this and forward-declare SecurityInfo
> instead.
> 
> I think that only works if we move SecurityInfo out of SecurityStateModel
(it's
> a nested class at the moment). WDYT?

Oh, that's right -- fine to leave as is. However, I left a comment in
security_state_model.h; I do think we might want to remove SecurityStateModel
the class (not necessarily in this CL) and replace it with a couple constants,
structs, and a GetSecurityInfo() utility function. If we do that later, we'll
have to come back and replace all
security_state::SecurityStateModel::SecurityInfo references with
security_state::SecurityInfo. So, if you want to do that now, it might save some
time later, but not necessary to do now IMO.

https://codereview.chromium.org/2448943002/diff/300001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper.h (right):

https://codereview.chromium.org/2448943002/diff/300001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper.h:51:
std::unique_ptr<security_state::SecurityStateModel> model_;
nit: could probably just be a SecurityStateModel, not a
unique_ptr<SecurityStateModel>

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/content/content_utils.cc:29: namespace
security_state_content_utils {
Per blundell's comment, these should be in the same security_state namespace as
SecurityStateModel.

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
File components/security_state/core/security_state_model.h (right):

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/core/security_state_model.h:28: class
SecurityStateModel {
I'll just note, there's no reason AFAICT for this to be a class anymore; it
doesn't have any state and is essentially just the utility function
GetSecurityState.

We can clean that up in a follow-up CL though (I can do it if you'd prefer).

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/core/security_state_model.h:207: SecurityInfo* result,
optional nit: I think putting the output parameter |result| at the end of the
argument list would be more conventional

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
File components/security_state/core/security_state_model_unittest.cc (right):

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/core/security_state_model_unittest.cc:101:
UsedPolicyInstalledCertificate(),
nit:
`false /* used policy installed certificate */`
instead of a helper method

https://codereview.chromium.org/2448943002/diff/300001/ios/chrome/browser/ssl...
File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right):

https://codereview.chromium.org/2448943002/diff/300001/ios/chrome/browser/ssl...
ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:31:
UsedPolicyInstalledCertificate(),
nit:
`false /* used policy installed certificate */`
instead of a helper function

[Eric Seckler, 2016-11-03 13:11:09]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 13:11:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-03 13:19:27]

Description was changed from

==========
Make content-specific parts of ChromeSecurityStateModelClient reusable.

Extracts parts of CSSMC into the security_state component, to make it
accessible to other embedders of content/. This is necessary to support
DevTool's Security domain in headless chrome. (We will add this support
in a follow-up patch.)

The patch introduces WebContentsSecurityStateModelClient as a new
superclass of CSSMC. WCSSMC resides in
/components/security_state/content. Sources previously under
/components/security_state (shared between iOS/content) are moved to
/components/security_state/core. UI strings now used by WCSSMC are
moved into .grd files in /components/.

BUG=609394
==========

to

==========
Refactor SecurityStateModel/Clients for simplicity and reusability.

Primary purpose for this patch is to make content-specific parts of
ChromeSecurityStateModelClient reusable for use in other embedders,
e.g. in headless chrome to support DevTool's Security domain.

The patch replaces the SecurityStateModel class (and its Client
interface) with helper methods, and moves content-specific logic
from CSSMC into new helper methods in the security_state component.

BUG=609394
==========

[commit-bot: I haz the power, 2016-11-03 13:25:39]

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

[commit-bot: I haz the power, 2016-11-03 13:25:43]

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

[Eric Seckler, 2016-11-03 13:40:30]

Patchset #6 (id:320001) has been deleted

[Eric Seckler, 2016-11-03 14:02:40]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 14:03:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 14:22:43]

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

[commit-bot: I haz the power, 2016-11-03 14:22:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Eric Seckler, 2016-11-03 14:37:32]

Remove SecurityStateModel.

[Eric Seckler, 2016-11-03 14:37:52]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 14:38:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 14:58:33]

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

[commit-bot: I haz the power, 2016-11-03 14:58:35]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Eric Seckler, 2016-11-03 15:15:16]

Patchset #6 (id:340001) has been deleted

[Eric Seckler, 2016-11-03 15:24:03]

Patchset #6 (id:360001) has been deleted

[Eric Seckler, 2016-11-03 15:26:09]

Remove SecurityStateModel.

[Eric Seckler, 2016-11-03 15:27:51]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 15:28:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-03 17:01:06]

Thanks, Emily! PTAL, removing SecurityStateModel changed quite a few places :)

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands.h (right):

https://codereview.chromium.org/2448943002/diff/280001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.h:13: #include
"components/security_state/core/security_state_model.h"
On 2016/11/03 04:38:45, estark wrote:
> On 2016/11/02 09:56:53, Eric Seckler wrote:
> > On 2016/11/02 05:22:02, estark wrote:
> > > Looks like you could just delete this and forward-declare SecurityInfo
> > instead.
> > 
> > I think that only works if we move SecurityInfo out of SecurityStateModel
> (it's
> > a nested class at the moment). WDYT?
> 
> Oh, that's right -- fine to leave as is. However, I left a comment in
> security_state_model.h; I do think we might want to remove SecurityStateModel
> the class (not necessarily in this CL) and replace it with a couple constants,
> structs, and a GetSecurityInfo() utility function. If we do that later, we'll
> have to come back and replace all
> security_state::SecurityStateModel::SecurityInfo references with
> security_state::SecurityInfo. So, if you want to do that now, it might save
some
> time later, but not necessary to do now IMO.

You're right, makes sense to do it in one review. SecurityStateModel is no more,
includes replaced by forward-decls. Hope I got them all :)

https://codereview.chromium.org/2448943002/diff/280001/components/security_st...
File components/security_state/content/content_utils.h (right):

https://codereview.chromium.org/2448943002/diff/280001/components/security_st...
components/security_state/content/content_utils.h:18: namespace
security_state_content_utils {
On 2016/11/01 23:29:01, blundell (OOO until Nov 7) wrote:
> nit: this should just be the namespace of the component, i.e., security_state.

Done.

https://codereview.chromium.org/2448943002/diff/300001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper.h (right):

https://codereview.chromium.org/2448943002/diff/300001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper.h:51:
std::unique_ptr<security_state::SecurityStateModel> model_;
On 2016/11/03 04:38:45, estark wrote:
> nit: could probably just be a SecurityStateModel, not a
> unique_ptr<SecurityStateModel>

no longer necessary :)

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/content/content_utils.cc:29: namespace
security_state_content_utils {
On 2016/11/03 04:38:45, estark wrote:
> Per blundell's comment, these should be in the same security_state namespace
as
> SecurityStateModel.

Done.

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
File components/security_state/core/security_state_model.h (right):

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/core/security_state_model.h:28: class
SecurityStateModel {
On 2016/11/03 04:38:45, estark wrote:
> I'll just note, there's no reason AFAICT for this to be a class anymore; it
> doesn't have any state and is essentially just the utility function
> GetSecurityState.
> 
> We can clean that up in a follow-up CL though (I can do it if you'd prefer).

Class is gone :)

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/core/security_state_model.h:207: SecurityInfo* result,
On 2016/11/03 04:38:45, estark wrote:
> optional nit: I think putting the output parameter |result| at the end of the
> argument list would be more conventional

Done.

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
File components/security_state/core/security_state_model_unittest.cc (right):

https://codereview.chromium.org/2448943002/diff/300001/components/security_st...
components/security_state/core/security_state_model_unittest.cc:101:
UsedPolicyInstalledCertificate(),
On 2016/11/03 04:38:45, estark wrote:
> nit:
> `false /* used policy installed certificate */`
> instead of a helper method

Done.

https://codereview.chromium.org/2448943002/diff/300001/ios/chrome/browser/ssl...
File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right):

https://codereview.chromium.org/2448943002/diff/300001/ios/chrome/browser/ssl...
ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:31:
UsedPolicyInstalledCertificate(),
On 2016/11/03 04:38:45, estark wrote:
> nit:
> `false /* used policy installed certificate */`
> instead of a helper function

Done.

[Eric Seckler, 2016-11-03 17:37:36]

eseckler@chromium.org changed reviewers:
+ jochen@chromium.org, rohitrao@chromium.org, sadrul@chromium.org

[Eric Seckler, 2016-11-03 17:37:38]

Adding reviewers for OWNERS approval:

+jochen@ for
    /chrome/ [except browser/ssl/],
    /components/ [except security_state] since blundell is OOO,
    /components/security_state/content/DEPS.

+rohitrao for
    /ios/chrome/browser/ui.

+sadrul for
    /components/security_state/content/DEPS (ui/base dependency).

Thanks everyone!

[commit-bot: I haz the power, 2016-11-03 18:10:33]

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

[commit-bot: I haz the power, 2016-11-03 18:10:35]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jochen (gone - plz use gerrit), 2016-11-04 09:18:32]

https://codereview.chromium.org/2448943002/diff/380001/components/security_st...
File components/security_state/content/DEPS (right):

https://codereview.chromium.org/2448943002/diff/380001/components/security_st...
components/security_state/content/DEPS:6: "+content/shell",
you shouldn't depend on content/shell

https://codereview.chromium.org/2448943002/diff/380001/components/security_st...
components/security_state/content/DEPS:10:
"+third_party/WebKit/public/platform",
browser things should list the webkit headers individually they need to include.
We can only include POD / enum headers from the browser

[Eric Seckler, 2016-11-04 10:47:01]

https://codereview.chromium.org/2448943002/diff/380001/components/security_st...
File components/security_state/content/DEPS (right):

https://codereview.chromium.org/2448943002/diff/380001/components/security_st...
components/security_state/content/DEPS:6: "+content/shell",
On 2016/11/04 09:18:32, jochen wrote:
> you shouldn't depend on content/shell

We're using content/shell/browser/shell.h in the browsertest. Is there an
alternative?

https://codereview.chromium.org/2448943002/diff/380001/components/security_st...
components/security_state/content/DEPS:10:
"+third_party/WebKit/public/platform",
On 2016/11/04 09:18:32, jochen wrote:
> browser things should list the webkit headers individually they need to
include.
> We can only include POD / enum headers from the browser

Done.

[rohitrao (ping after 24h), 2016-11-04 14:07:01]

lgtm

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
File ios/chrome/browser/ssl/ios_chrome_security_state_model_client.h (left):

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
ios/chrome/browser/ssl/ios_chrome_security_state_model_client.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
This file is marked as an edit instead of a delete.  This happens occasionally,
and I don't know how to fix it other than to close this issue and create a new
one.

I don't know if the CQ will properly delete this file, or if it will delete all
the content but leave an empty file behind.

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right):

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:23:
IOSSecurityStateTabHelper::IOSSecurityStateTabHelper(web::WebState* web_state)
Where does the desktop code create and attach its version of this TabHelper? 
I'll need to do something similar on iOS.

[Eric Seckler, 2016-11-04 14:17:56]

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
File ios/chrome/browser/ssl/ios_chrome_security_state_model_client.h (left):

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
ios/chrome/browser/ssl/ios_chrome_security_state_model_client.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/11/04 14:07:01, rohitrao wrote:
> This file is marked as an edit instead of a delete.  This happens
occasionally,
> and I don't know how to fix it other than to close this issue and create a new
> one.
> 
> I don't know if the CQ will properly delete this file, or if it will delete
all
> the content but leave an empty file behind.

Thanks for noticing! Shall we keep an eye out for it and delete it in a
follow-up if necessary?

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right):

https://codereview.chromium.org/2448943002/diff/400001/ios/chrome/browser/ssl...
ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:23:
IOSSecurityStateTabHelper::IOSSecurityStateTabHelper(web::WebState* web_state)
On 2016/11/04 14:07:01, rohitrao wrote:
> Where does the desktop code create and attach its version of this TabHelper? 
> I'll need to do something similar on iOS.

The iOS version is created by calling the static method CreateForWebState. On
desktop, the equivalent happens here:

https://cs.chromium.org/chromium/src/chrome/browser/ui/tab_helpers.cc?l=187&dr=C

[sadrul, 2016-11-04 15:45:02]

> +sadrul for
>     /components/security_state/content/DEPS (ui/base dependency).

lgtm

[estark, 2016-11-04 17:16:53]

still lgtm!

[Eric Seckler, 2016-11-08 10:01:17]

Jochen, I see you're traveling this/next week. Will you have a chance to take
another peek at this, or should I redistribute reviews? Thanks! :)

[Eric Seckler, 2016-11-10 16:35:39]

eseckler@chromium.org changed reviewers:
+ agl@chromium.org, kinuko@chromium.org, sky@chromium.org
- jochen@chromium.org

[Eric Seckler, 2016-11-10 16:35:41]

Updating reviewers for OWNER approval:

-jochen b/c traveling.

+sky for
    //chrome/ [except browser/ssl/].

+blundell for
    //components/ [except security_state],
    //components/security_state/content/DEPS (components dep)

+kinuko for
    //components/security_state/content/DEPS (content and webkit deps)

+agl for
    //components/security_state/content/DEPS (third_party/boringssl dep)

Thanks again!

[sky, 2016-11-10 18:00:35]

LGTM

[kinuko, 2016-11-11 05:45:11]

https://codereview.chromium.org/2448943002/diff/400001/components/security_st...
File components/security_state/content/DEPS (right):

https://codereview.chromium.org/2448943002/diff/400001/components/security_st...
components/security_state/content/DEPS:7: "+content/shell",
I think you can use specific_include_rules to add DEPS only for test-files

[Eric Seckler, 2016-11-11 14:44:17]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 14:44:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-11 14:45:21]

https://codereview.chromium.org/2448943002/diff/400001/components/security_st...
File components/security_state/content/DEPS (right):

https://codereview.chromium.org/2448943002/diff/400001/components/security_st...
components/security_state/content/DEPS:7: "+content/shell",
On 2016/11/11 05:45:11, kinuko wrote:
> I think you can use specific_include_rules to add DEPS only for test-files

Done.

[commit-bot: I haz the power, 2016-11-11 15:56:39]

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

[commit-bot: I haz the power, 2016-11-11 15:56:41]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-11-11 16:06:43]

lgtm

[blundell, 2016-11-14 11:45:21]

//components lgtm

[Eric Seckler, 2016-11-14 12:02:03]

eseckler@chromium.org changed reviewers:
+ davidben@chromium.org
- agl@chromium.org

[Eric Seckler, 2016-11-14 12:02:05]

On 2016/11/10 16:35:41, Eric Seckler wrote:
> +agl for
>     //components/security_state/content/DEPS (third_party/boringssl dep)

+davidben instead

[Eric Seckler, 2016-11-14 12:05:50]

eseckler@chromium.org changed reviewers:
+ agl@chromium.org
- davidben@chromium.org

[Eric Seckler, 2016-11-14 12:05:52]

On 2016/11/14 12:02:05, Eric Seckler wrote:
> On 2016/11/10 16:35:41, Eric Seckler wrote:
> > +agl for
> >     //components/security_state/content/DEPS (third_party/boringssl dep)
> 
> +davidben instead

Ah, didn't realize he's OOO. back to agl@ ... sorry about that :)

[Eric Seckler, 2016-11-14 14:32:12]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 14:32:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 15:21:41]

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

[commit-bot: I haz the power, 2016-11-14 15:21:43]

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

[Eric Seckler, 2016-11-14 15:37:49]

Patchset #9 (id:440001) has been deleted

[Eric Seckler, 2016-11-14 15:38:05]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 15:38:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 17:11:12]

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

[commit-bot: I haz the power, 2016-11-14 17:11:15]

Dry run: This issue passed the CQ dry run.

[agl, 2016-11-15 18:47:01]

lgtm

[Eric Seckler, 2016-11-15 19:34:24]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 19:35:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 19:50:29]

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

[commit-bot: I haz the power, 2016-11-15 19:50:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Eric Seckler, 2016-11-16 08:55:32]

Patchset #10 (id:480001) has been deleted

[Eric Seckler, 2016-11-16 08:55:52]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 08:56:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-16 10:00:07]

The CQ bit was unchecked by eseckler@chromium.org

[Eric Seckler, 2016-11-16 10:00:08]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-11-16 10:00:10]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org, estark@chromium.org, rohitrao@chromium.org,
kinuko@chromium.org, blundell@chromium.org, agl@chromium.org
Link to the patchset: https://codereview.chromium.org/2448943002/#ps500001
(title: "sync.")

[commit-bot: I haz the power, 2016-11-16 10:00:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 11:19:07]

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

[commit-bot: I haz the power, 2016-11-16 11:19:10]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Eric Seckler, 2016-11-16 11:21:16]

Patchset #11 (id:520001) has been deleted

[Eric Seckler, 2016-11-16 11:21:31]

Patchset #10 (id:500001) has been deleted

[Eric Seckler, 2016-11-16 11:25:46]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-11-16 11:25:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org, kinuko@chromium.org, estark@chromium.org,
rohitrao@chromium.org, agl@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2448943002/#ps540001
(title: "sync.")

[commit-bot: I haz the power, 2016-11-16 11:26:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 13:50:12]

Description was changed from

==========
Refactor SecurityStateModel/Clients for simplicity and reusability.

Primary purpose for this patch is to make content-specific parts of
ChromeSecurityStateModelClient reusable for use in other embedders,
e.g. in headless chrome to support DevTool's Security domain.

The patch replaces the SecurityStateModel class (and its Client
interface) with helper methods, and moves content-specific logic
from CSSMC into new helper methods in the security_state component.

BUG=609394
==========

to

==========
Refactor SecurityStateModel/Clients for simplicity and reusability.

Primary purpose for this patch is to make content-specific parts of
ChromeSecurityStateModelClient reusable for use in other embedders,
e.g. in headless chrome to support DevTool's Security domain.

The patch replaces the SecurityStateModel class (and its Client
interface) with helper methods, and moves content-specific logic
from CSSMC into new helper methods in the security_state component.

BUG=609394
==========

[commit-bot: I haz the power, 2016-11-16 13:50:18]

Committed patchset #10 (id:540001)

[commit-bot: I haz the power, 2016-11-16 13:56:01]

Description was changed from

==========
Refactor SecurityStateModel/Clients for simplicity and reusability.

Primary purpose for this patch is to make content-specific parts of
ChromeSecurityStateModelClient reusable for use in other embedders,
e.g. in headless chrome to support DevTool's Security domain.

The patch replaces the SecurityStateModel class (and its Client
interface) with helper methods, and moves content-specific logic
from CSSMC into new helper methods in the security_state component.

BUG=609394
==========

to

==========
Refactor SecurityStateModel/Clients for simplicity and reusability.

Primary purpose for this patch is to make content-specific parts of
ChromeSecurityStateModelClient reusable for use in other embedders,
e.g. in headless chrome to support DevTool's Security domain.

The patch replaces the SecurityStateModel class (and its Client
interface) with helper methods, and moves content-specific logic
from CSSMC into new helper methods in the security_state component.

BUG=609394

Committed: https://crrev.com/cac56b6adc739e3ba3d5a20a45c0da87fec9d02a
Cr-Commit-Position: refs/heads/master@{#432474}
==========

[commit-bot: I haz the power, 2016-11-16 13:56:04]

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