Remove SecurityStateModel memoization

Remove SecurityStateModel memoization

As discussed in the bug, it's hard to imagine that memoizing on
VisibleSecurityState buys us anything other than more complicated code.

BUG=642576
Committed: https://crrev.com/518660431c09f50c55c73b0bee872c2a5fc5b1ae
Cr-Commit-Position: refs/heads/master@{#420444}

[estark, 2016-09-22 04:53:47]

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

[commit-bot: I haz the power, 2016-09-22 04:53:56]

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-09-22 05:36:43]

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

[commit-bot: I haz the power, 2016-09-22 05:36:44]

Dry run: This issue passed the CQ dry run.

[estark, 2016-09-22 15:36:11]

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

[estark, 2016-09-22 15:36:12]

felt, PTAL. Don't be alarmed -- most of this is completely mechanical changes,
with the substantive changes in components/security_state.

Since there's no longer a cached SecurityInfo for which GetSecurityInfo() can
return a const reference, I changed the GetSecurityInfo() signature to take an
output parameter to which it writes the SecurityInfo. (That's why this CL
touches a gazillion files.)

[felt, 2016-09-22 16:29:47]

On 2016/09/22 15:36:12, estark wrote:
> felt, PTAL. Don't be alarmed -- most of this is completely mechanical changes,
> with the substantive changes in components/security_state.
> 
> Since there's no longer a cached SecurityInfo for which GetSecurityInfo() can
> return a const reference, I changed the GetSecurityInfo() signature to take an
> output parameter to which it writes the SecurityInfo. (That's why this CL
> touches a gazillion files.)

I love CLs that start with "Don't be alarmed" ;)

[felt, 2016-09-22 17:19:42]

Are you planning to also remove VisibleSecurityState (as described in the bug),
or have you decided against that?

[estark, 2016-09-22 17:31:24]

On 2016/09/22 17:19:42, felt wrote:
> Are you planning to also remove VisibleSecurityState (as described in the
bug),
> or have you decided against that?

Oh, thanks for reminding me, I meant to mention: I'm waffling on whether I
actually want to remove VisibleSecurityState or not, and would like to do it in
a follow-up if I can re-convince myself that removing it is a good idea.

If we remove it, we have to replace it with SecurityStateModelClient methods...
so instead of GetVisibleSecurityState(), we'd have GetCertStatus(),
GetConnectionStatus(), GetFailsMailwareCheck(), etc. for each field in
VisibleSecurityState. I think that's a bit of an improvement, because I think
it's easy to get VisibleSecurityState confused with SecurityInfo. So... I think
it's worth doing? Do you have an opinion? And if you think it's worth removing
are you okay with me doing that in a follow-up?

[felt, 2016-09-22 17:47:53]

On 2016/09/22 17:31:24, estark wrote:
> On 2016/09/22 17:19:42, felt wrote:
> > Are you planning to also remove VisibleSecurityState (as described in the
> bug),
> > or have you decided against that?
> 
> Oh, thanks for reminding me, I meant to mention: I'm waffling on whether I
> actually want to remove VisibleSecurityState or not, and would like to do it
in
> a follow-up if I can re-convince myself that removing it is a good idea.
> 
> If we remove it, we have to replace it with SecurityStateModelClient
methods...
> so instead of GetVisibleSecurityState(), we'd have GetCertStatus(),
> GetConnectionStatus(), GetFailsMailwareCheck(), etc. for each field in
> VisibleSecurityState. I think that's a bit of an improvement, because I think
> it's easy to get VisibleSecurityState confused with SecurityInfo. So... I
think
> it's worth doing? Do you have an opinion? And if you think it's worth removing
> are you okay with me doing that in a follow-up?

This CL lgtm.

In terms of whether you should also remove VisibleSecurityState-- well... right
now I think it is pretty confusing and annoying to have to copy over all of the
fields between two different structures, and it is not entirely clear to an
outside observer what the difference is or why there is two of them. For that
reason, I'm in favor of removing it, even though it would yield a bunch of
getters.

[estark, 2016-09-22 17:49:22]

On 2016/09/22 17:47:53, felt wrote:
> On 2016/09/22 17:31:24, estark wrote:
> > On 2016/09/22 17:19:42, felt wrote:
> > > Are you planning to also remove VisibleSecurityState (as described in the
> > bug),
> > > or have you decided against that?
> > 
> > Oh, thanks for reminding me, I meant to mention: I'm waffling on whether I
> > actually want to remove VisibleSecurityState or not, and would like to do it
> in
> > a follow-up if I can re-convince myself that removing it is a good idea.
> > 
> > If we remove it, we have to replace it with SecurityStateModelClient
> methods...
> > so instead of GetVisibleSecurityState(), we'd have GetCertStatus(),
> > GetConnectionStatus(), GetFailsMailwareCheck(), etc. for each field in
> > VisibleSecurityState. I think that's a bit of an improvement, because I
think
> > it's easy to get VisibleSecurityState confused with SecurityInfo. So... I
> think
> > it's worth doing? Do you have an opinion? And if you think it's worth
removing
> > are you okay with me doing that in a follow-up?
> 
> This CL lgtm.
> 
> In terms of whether you should also remove VisibleSecurityState-- well...
right
> now I think it is pretty confusing and annoying to have to copy over all of
the
> fields between two different structures, and it is not entirely clear to an
> outside observer what the difference is or why there is two of them. For that
> reason, I'm in favor of removing it, even though it would yield a bunch of
> getters.

Yeah. I think we'll end up with something that is still annoying (having to add
a getter for every field) but less confusing, so that's an improvement at least.

[estark, 2016-09-22 17:51:25]

estark@chromium.org changed reviewers:
+ nparker@chromium.org, sky@chromium.org, tedchoc@chromium.org

[estark, 2016-09-22 17:51:26]

Adding some more OWNERs for review. The changes outside
components/security_state are mechanical.

tedchoc: chrome/browser/android and chrome/browser/ui/android
nparker: chrome/browser/safe_browsing
sky: chrome/browser/ui

Thanks, all.

[Nathan Parker, 2016-09-22 18:09:58]

LGTM for safe_browsing

[Ted C, 2016-09-22 18:14:19]

On 2016/09/22 18:09:58, Nathan Parker wrote:
> LGTM for safe_browsing

chrome/browser/android and chrome/browser/ui/android - lgtm

[sky, 2016-09-22 20:22:16]

LGTM

[estark, 2016-09-22 20:28:08]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-09-22 20:28:42]

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-09-22 20:36:14]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-22 20:37:56]

Description was changed from

==========
Remove SecurityStateModel memoization

As discussed in the bug, it's hard to imagine that memoizing on
VisibleSecurityState buys us anything other than more complicated code.

BUG=642576
==========

to

==========
Remove SecurityStateModel memoization

As discussed in the bug, it's hard to imagine that memoizing on
VisibleSecurityState buys us anything other than more complicated code.

BUG=642576

Committed: https://crrev.com/518660431c09f50c55c73b0bee872c2a5fc5b1ae
Cr-Commit-Position: refs/heads/master@{#420444}
==========

[commit-bot: I haz the power, 2016-09-22 20:37:58]

Patchset 1 (id:??) landed as
https://crrev.com/518660431c09f50c55c73b0bee872c2a5fc5b1ae
Cr-Commit-Position: refs/heads/master@{#420444}