Create a component for SSL error handling

Create a component for SSL error handling

Classes like SSLErrorInfo and SSLErrorClassification are
tightly coupled with the SSL interstitial. Since the interstitial UI is being
componentized, I want to move the SSL error handling classes into a component
too. This CL begins with SSLErrorInfo.

BUG=488673
TBR=stuartmorgan@chromium.org

Committed: https://crrev.com/7f1410648c7767ae8d466ada4385e78176f5db06
Cr-Commit-Position: refs/heads/master@{#349282}

Committed: https://crrev.com/2493b445699a91d3e71e598d9318f32b8974a53f
Cr-Commit-Position: refs/heads/master@{#349486}

[felt, 2015-09-15 02:46:53]

Patchset #1 (id:1) has been deleted

[felt, 2015-09-15 02:47:02]

Patchset #1 (id:20001) has been deleted

[felt, 2015-09-15 08:21:50]

Patchset #4 (id:100001) has been deleted

[felt, 2015-09-15 14:02:38]

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

[felt, 2015-09-15 14:07:26]

Patchset #5 (id:140001) has been deleted

[felt, 2015-09-15 14:42:10]

Patchset #5 (id:160001) has been deleted

[felt, 2015-09-15 14:42:58]

Patchset #4 (id:120001) has been deleted

[felt, 2015-09-15 15:02:17]

Patchset #4 (id:180001) has been deleted

[felt, 2015-09-15 15:02:41]

estark, blundell: PTAL

[felt, 2015-09-15 15:15:11]

On 2015/09/15 15:02:41, felt wrote:
> estark, blundell: PTAL

(btw, I know I'm having trouble with the bots. the issue is that the grd file
has grown too big to fit in the space allocated to it, and we're bumping up
against the next allocated range. I'll get that sorted out -- will involve
tweaking tools/gritsettings/resource_ids with the right magic spell -- hopefully
it shouldn't block your reviews.)

[estark, 2015-09-15 22:37:28]

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/ssl_error_info.h (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:23: class SSLErrorInfo {
Similar to what we did with cert reporting, did you consider making this
ssl_errors::ErrorInfo or something shorter like that?

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:58: static void GetErrorsForCertStatus(
While you're here, you could document the |cert| parameter and delete the
(probably-very-old) TODO on the line above.

[Ryan Sleevi, 2015-09-15 23:24:57]

If they're tightly coupled, should they belong in the same component? Why or why
not?

(My own concerns on the componentization are somewhat archived on chromium-dev,
but my view is that if there's tight coupling, then they probably belong
together, or require decoupling. Decoupling can be good, when there's a use
case, or it can be a bunch of abstractions that make code harder to read,
maintain, find, or debug, so I like to keep close friends together)

[felt, 2015-09-16 00:11:23]

On 2015/09/15 23:24:57, Ryan Sleevi wrote:
> If they're tightly coupled, should they belong in the same component? Why or
why
> not?

Great question. I've been thinking about that.

My original plan was to put them together in a single component. But then it
seemed weird for WebsiteSettings to have a dependency on security_interstitials;
so instead I thought I could put SSLErrorInfo and SSLErrorClassifier in a fairly
simple component for diagnosing errors.

When I say "coupled" I really mean that security_interstitials has dependencies
on SSLErrorInfo and SSLErrorClassifier. They aren't circularly dependent.
(Although it isn't clear to me where SSLErrorHandler should go, since that does
have dependencies on both... but also maybe should just stay in Chrome, I was
planning to ignore that question for now.)

If you don't find it odd to have WebsiteSettings depend on
security_interstitials, then I can combine the ssl_errors and
security_interstitials components into a single one.


> 
> (My own concerns on the componentization are somewhat archived on
chromium-dev,
> but my view is that if there's tight coupling, then they probably belong
> together, or require decoupling. Decoupling can be good, when there's a use
> case, or it can be a bunch of abstractions that make code harder to read,
> maintain, find, or debug, so I like to keep close friends together)

[Ryan Sleevi, 2015-09-16 00:20:57]

On 2015/09/16 00:11:23, felt wrote:
> When I say "coupled" I really mean that security_interstitials has
dependencies
> on SSLErrorInfo and SSLErrorClassifier. They aren't circularly dependent.

Phew :) First order concern, resolved :)

> (Although it isn't clear to me where SSLErrorHandler should go, since that
does
> have dependencies on both... but also maybe should just stay in Chrome, I was
> planning to ignore that question for now.)

It's actually a pretty useful question when thinking about componentization. I
agree, it does sound like it belongs in Chrome if it's pulling in multiple
components - or a refactoring of delegate interfaces if the behaviour of
SSLErrorHandler needs to be usable from components.

I'm a little weird about exposing it as a component, since it's not clear how
much iOS will use this in the future, and perhaps that requires rethinking the
layering of the interstitials, so that we reduce the amount of (unneeded/unused)
code there. But I think that's probably a concern 'for later', but worth
thinking about now.

It sounds like you considered whether one or two, and that was my only other big
concern, so thanks for the explanation (and memorializing it on this CL :P)

> If you don't find it odd to have WebsiteSettings depend on
> security_interstitials, then I can combine the ssl_errors and
> security_interstitials components into a single one.

I agree, strikes me as odd :)

[Ryan Sleevi, 2015-09-16 00:21:07]

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

[Ryan Sleevi, 2015-09-16 00:21:08]

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/ssl_error_info.h (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:59: const
scoped_refptr<net::X509Certificate> cert,
This should either be const-ref (ideal if you don't take ownership) or non-const
by-value (if and only if you're using .Pass() semantics to transfer 'ownership'
of the reference count)

the const-value just means extra refcount churn.

[felt, 2015-09-16 02:54:59]

On 2015/09/16 00:20:57, Ryan Sleevi wrote:
> On 2015/09/16 00:11:23, felt wrote:
> > When I say "coupled" I really mean that security_interstitials has
> dependencies
> > on SSLErrorInfo and SSLErrorClassifier. They aren't circularly dependent.
> 
> Phew :) First order concern, resolved :)
> 
> > (Although it isn't clear to me where SSLErrorHandler should go, since that
> does
> > have dependencies on both... but also maybe should just stay in Chrome, I
was
> > planning to ignore that question for now.)
> 
> It's actually a pretty useful question when thinking about componentization. I
> agree, it does sound like it belongs in Chrome if it's pulling in multiple
> components - or a refactoring of delegate interfaces if the behaviour of
> SSLErrorHandler needs to be usable from components.

I don't think anything would change in my current plans regardless of what
happens with SSLErrorHandler. Either way: security_interstitials still needs to
be layered, and I'm going to avoid layering for the ssl_errors classes.

> 
> I'm a little weird about exposing it as a component, since it's not clear how
> much iOS will use this in the future, and perhaps that requires rethinking the
> layering of the interstitials, so that we reduce the amount of
(unneeded/unused)
> code there. But I think that's probably a concern 'for later', but worth
> thinking about now.

Yeah, I don't know if iOS would ever end up using SSLErrorHandler, which is why
I'm not anxious to move it. But iOS ought to use the rest of the
security_interstitials code.

> 
> It sounds like you considered whether one or two, and that was my only other
big
> concern, so thanks for the explanation (and memorializing it on this CL :P)
> 
> > If you don't find it odd to have WebsiteSettings depend on
> > security_interstitials, then I can combine the ssl_errors and
> > security_interstitials components into a single one.
> 
> I agree, strikes me as odd :)

[blundell, 2015-09-16 10:04:10]

LGTM

The logic on this being a separate component seems sound to me.

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/BUILD.gn (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/BUILD.gn:6: static_library("ssl_errors") {
s/static_library/source_set (they're generally preferred in GN)

[felt, 2015-09-16 14:41:53]

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/BUILD.gn (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/BUILD.gn:6: static_library("ssl_errors") {
On 2015/09/16 10:04:10, blundell wrote:
> s/static_library/source_set (they're generally preferred in GN)

Done.

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/ssl_error_info.h (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:23: class SSLErrorInfo {
On 2015/09/15 22:37:28, estark wrote:
> Similar to what we did with cert reporting, did you consider making this
> ssl_errors::ErrorInfo or something shorter like that?

When we had that last discussion, Colin advocated against dropping the name
prefix if the resulting class name is very generic. I thought "ErrorInfo" might
be too generic so decided against renaming it. WDYT?

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:58: static void GetErrorsForCertStatus(
On 2015/09/15 22:37:28, estark wrote:
> While you're here, you could document the |cert| parameter and delete the
> (probably-very-old) TODO on the line above.

Done.

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:59: const
scoped_refptr<net::X509Certificate> cert,
On 2015/09/16 00:21:08, Ryan Sleevi wrote:
> This should either be const-ref (ideal if you don't take ownership) or
non-const
> by-value (if and only if you're using .Pass() semantics to transfer
'ownership'
> of the reference count)
> 
> the const-value just means extra refcount churn.

Done.

[estark, 2015-09-16 15:01:07]

lgtm

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/ssl_error_info.h (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:23: class SSLErrorInfo {
On 2015/09/16 14:41:53, felt wrote:
> On 2015/09/15 22:37:28, estark wrote:
> > Similar to what we did with cert reporting, did you consider making this
> > ssl_errors::ErrorInfo or something shorter like that?
> 
> When we had that last discussion, Colin advocated against dropping the name
> prefix if the resulting class name is very generic. I thought "ErrorInfo"
might
> be too generic so decided against renaming it. WDYT?

Seems reasonable. I would personally find ssl_errors::Info too generic but
ssl_errors::ErrorInfo descriptive enough, but I think it's just a subjective
preference. I'm fine with leaving it as is.

[blundell, 2015-09-16 15:05:29]

On 2015/09/16 15:01:07, estark wrote:
> lgtm
> 
>
https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
> File components/ssl_errors/ssl_error_info.h (right):
> 
>
https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
> components/ssl_errors/ssl_error_info.h:23: class SSLErrorInfo {
> On 2015/09/16 14:41:53, felt wrote:
> > On 2015/09/15 22:37:28, estark wrote:
> > > Similar to what we did with cert reporting, did you consider making this
> > > ssl_errors::ErrorInfo or something shorter like that?
> > 
> > When we had that last discussion, Colin advocated against dropping the name
> > prefix if the resulting class name is very generic. I thought "ErrorInfo"
> might
> > be too generic so decided against renaming it. WDYT?
> 
> Seems reasonable. I would personally find ssl_errors::Info too generic but
> ssl_errors::ErrorInfo descriptive enough, but I think it's just a subjective
> preference. I'm fine with leaving it as is.

(FWIW, I personally would be fine with either ssl_errors::SSLErrorInfo or
ssl_errors::ErrorInfo; I think they're both clear.)

[felt, 2015-09-16 20:28:08]

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
File components/ssl_errors/ssl_error_info.h (right):

https://codereview.chromium.org/1341923002/diff/200001/components/ssl_errors/...
components/ssl_errors/ssl_error_info.h:23: class SSLErrorInfo {
On 2015/09/16 15:01:07, estark wrote:
> On 2015/09/16 14:41:53, felt wrote:
> > On 2015/09/15 22:37:28, estark wrote:
> > > Similar to what we did with cert reporting, did you consider making this
> > > ssl_errors::ErrorInfo or something shorter like that?
> > 
> > When we had that last discussion, Colin advocated against dropping the name
> > prefix if the resulting class name is very generic. I thought "ErrorInfo"
> might
> > be too generic so decided against renaming it. WDYT?
> 
> Seems reasonable. I would personally find ssl_errors::Info too generic but
> ssl_errors::ErrorInfo descriptive enough, but I think it's just a subjective
> preference. I'm fine with leaving it as is.

since blundell doesn't mind, I've updated it to be ssl_errors::ErrorInfo

[felt, 2015-09-16 20:31:54]

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

[felt, 2015-09-16 20:32:45]

sky, can you please provide an OWNERS review the remaining misc files:
chrome/browser/ui/browser.cc
chrome/browser/BUILD.gn
chrome/browser/DEPS
chrome/common/localized_error.cc

[sky, 2015-09-16 22:26:53]

LGTM

[felt, 2015-09-16 22:30:37]

The CQ bit was checked by felt@chromium.org

[felt, 2015-09-16 22:30:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1341923002/#ps300001
(title: "Fix BUILD.gn after renaming files")

[commit-bot: I haz the power, 2015-09-16 22:31:37]

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

[commit-bot: I haz the power, 2015-09-16 23:14:57]

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

[commit-bot: I haz the power, 2015-09-16 23:14:58]

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

[felt, 2015-09-16 23:22:41]

Hmm... Sleevi, I think I need you to sign off:

Missing L/G/T/M from OWNERS of dependencies added to DEPS:
    '+net',

[Ryan Sleevi, 2015-09-16 23:29:35]

rubber stamp LGTM

[felt, 2015-09-16 23:30:14]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2015-09-16 23:31:24]

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

[commit-bot: I haz the power, 2015-09-16 23:50:37]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2015-09-16 23:51:06]

Patchset 9 (id:??) landed as
https://crrev.com/7f1410648c7767ae8d466ada4385e78176f5db06
Cr-Commit-Position: refs/heads/master@{#349282}

[Justin Donnelly, 2015-09-16 23:58:50]

On 2015/09/16 23:51:06, commit-bot: I haz the power wrote:
> Patchset 9 (id:??) landed as
> https://crrev.com/7f1410648c7767ae8d466ada4385e78176f5db06
> Cr-Commit-Position: refs/heads/master@{#349282}

This change broke
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil....
I'm going to revert.

[Justin Donnelly, 2015-09-17 00:11:12]

On 2015/09/16 23:58:50, Justin Donnelly wrote:
> On 2015/09/16 23:51:06, commit-bot: I haz the power wrote:
> > Patchset 9 (id:??) landed as
> > https://crrev.com/7f1410648c7767ae8d466ada4385e78176f5db06
> > Cr-Commit-Position: refs/heads/master@{#349282}
> 
> This change broke
>
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil....
> I'm going to revert.

Reverting in https://codereview.chromium.org/1351773002

[blundell, 2015-09-17 07:30:16]

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

[blundell, 2015-09-17 07:30:17]

+sdefresne@ Can you see what about this CL would have caused the compile failure
in
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...
I can't :\.

Bonus points: Why was this green in the CQ?

[sdefresne, 2015-09-17 08:51:24]

On 2015/09/17 at 07:30:17, blundell wrote:
> +sdefresne@ Can you see what about this CL would have caused the compile
failure in
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...
I can't :\.

You need to whitelist those new names.

> Bonus points: Why was this green in the CQ?

I don't know why they didn't fail on the try bots however.

[blundell, 2015-09-17 09:04:47]

Thanks!

On 2015/09/17 08:51:24, sdefresne wrote:
> On 2015/09/17 at 07:30:17, blundell wrote:
> > +sdefresne@ Can you see what about this CL would have caused the compile
> failure in
>
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...
> I can't :\.
> 
> You need to whitelist those new names.

(in //build/ios/grit_whitelist.txt)

> 
> > Bonus points: Why was this green in the CQ?
> 
> I don't know why they didn't fail on the try bots however.

We figured this out too: the new files didn't get built on the CQ runs! This
seems like a bug in the analyzer step. I'll file it.

[felt, 2015-09-17 19:16:46]

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

[felt, 2015-09-17 19:17:49]

stuartmorgan: adding you as TBR for the strings that needed to be whitelisted
for iOS

[felt, 2015-09-17 19:18:03]

The CQ bit was checked by felt@chromium.org

[felt, 2015-09-17 19:18:05]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
sky@chromium.org, rsleevi@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1341923002/#ps320001
(title: "Update iOS grit whitelist")

[commit-bot: I haz the power, 2015-09-17 19:20:44]

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

[commit-bot: I haz the power, 2015-09-17 20:34:10]

Committed patchset #10 (id:320001)

[commit-bot: I haz the power, 2015-09-17 20:34:44]

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

[stuartmorgan, 2015-09-18 16:03:02]

whitelist lgtm