Add optional context for certificate errors.

Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
NOPRESUBMIT=true

Committed: https://crrev.com/24da0854378c9640bf9c726a2f067c2e0ba49205
Cr-Commit-Position: refs/heads/master@{#418118}

[eroman, 2016-09-09 20:36:37]

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

[eroman, 2016-09-09 20:36:51]

eroman@chromium.org changed reviewers:
+ mattm@chromium.org

[eroman, 2016-09-09 20:36:52]

https://codereview.chromium.org/2329593002/diff/1/net/cert/internal/cert_erro...
File net/cert/internal/cert_error_id.h (right):

https://codereview.chromium.org/2329593002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_error_id.h:12: // Each "class" of certificate
error/warning has its own unique ID. This is
This is mostly a move, but with these changes:
  * Renamed TYPE --> ID
  * Made it a void pointer so the string literal reality is hidden.

https://codereview.chromium.org/2329593002/diff/1/net/cert/internal/cert_erro...
File net/cert/internal/cert_error_params.h (right):

https://codereview.chromium.org/2329593002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_error_params.h:25: class NET_EXPORT CertErrorParams {
This is a move to its own file, however I also changed the virtual from
ToValue() --> ToDebugString()

https://codereview.chromium.org/2329593002/diff/1/net/data/verify_certificate...
File net/data/verify_certificate_chain_unittest/expired-intermediate.pem
(right):

https://codereview.chromium.org/2329593002/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/expired-intermediate.pem:284:
[Context] Processing Certificate
The files in net/data/verify_certificate_chain_unittest/ were updated
programmatically by rebase-errors.py

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

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-09 20:45:07]

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

[commit-bot: I haz the power, 2016-09-09 20:45:08]

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

[eroman, 2016-09-09 21:18:06]

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

[commit-bot: I haz the power, 2016-09-09 21:18:31]

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-09 21:27:48]

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

[commit-bot: I haz the power, 2016-09-09 21:27:49]

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

[eroman, 2016-09-10 01:12:56]

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

[commit-bot: I haz the power, 2016-09-10 01:13:29]

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

[eroman, 2016-09-10 01:32:04]

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

[commit-bot: I haz the power, 2016-09-10 01:32:45]

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-10 01:40:12]

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

[commit-bot: I haz the power, 2016-09-10 01:40:13]

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

[mattm, 2016-09-12 22:26:40]

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
File net/cert/internal/cert_errors.cc (right):

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
net/cert/internal/cert_errors.cc:70: cur_indentation += "    ";
Was it intentional that this indentation level can vary depending if
node->params block also increases indentation level?

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:425: CertErrorScoperNoParams
error_context(errors, kContextTrustAnchor);
nit, but the naming style mismatch between CertErrorScoperNoParams and
ScopedCertificateContext bugs me a little.

[eroman, 2016-09-12 23:03:59]

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

[eroman, 2016-09-12 23:04:35]

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
File net/cert/internal/cert_errors.cc (right):

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
net/cert/internal/cert_errors.cc:70: cur_indentation += "    ";
On 2016/09/12 22:26:39, mattm wrote:
> Was it intentional that this indentation level can vary depending if
> node->params block also increases indentation level?

Yes; that was purely an aesthetic choice.

I am still playing around with the formatting and error structure (which is why
I added the script to easily regenerate expectations).

But if you have feedback on what looks good I am open to ideas :)

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:425: CertErrorScoperNoParams
error_context(errors, kContextTrustAnchor);
On 2016/09/12 22:26:39, mattm wrote:
> nit, but the naming style mismatch between CertErrorScoperNoParams and
> ScopedCertificateContext bugs me a little.

Done.

[commit-bot: I haz the power, 2016-09-12 23:04:43]

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-12 23:14:06]

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

[commit-bot: I haz the power, 2016-09-12 23:14:07]

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

[mattm, 2016-09-12 23:32:19]

lgtm

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
File net/cert/internal/cert_errors.cc (right):

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
net/cert/internal/cert_errors.cc:70: cur_indentation += "    ";
On 2016/09/12 23:04:35, eroman wrote:
> On 2016/09/12 22:26:39, mattm wrote:
> > Was it intentional that this indentation level can vary depending if
> > node->params block also increases indentation level?
> 
> Yes; that was purely an aesthetic choice.
> 
> I am still playing around with the formatting and error structure (which is
why
> I added the script to easily regenerate expectations).
> 
> But if you have feedback on what looks good I am open to ideas :)

I think it's fine, just wanted to check. Maybe revisit once we get some errors
with more levels of context to see how things look then.

[eroman, 2016-09-12 23:43:37]

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
File net/cert/internal/cert_errors.cc (right):

https://codereview.chromium.org/2329593002/diff/60001/net/cert/internal/cert_...
net/cert/internal/cert_errors.cc:70: cur_indentation += "    ";
On 2016/09/12 23:32:18, mattm wrote:
> On 2016/09/12 23:04:35, eroman wrote:
> > On 2016/09/12 22:26:39, mattm wrote:
> > > Was it intentional that this indentation level can vary depending if
> > > node->params block also increases indentation level?
> > 
> > Yes; that was purely an aesthetic choice.
> > 
> > I am still playing around with the formatting and error structure (which is
> why
> > I added the script to easily regenerate expectations).
> > 
> > But if you have feedback on what looks good I am open to ideas :)
> 
> I think it's fine, just wanted to check. Maybe revisit once we get some errors
> with more levels of context to see how things look then.

Sounds good.

[eroman, 2016-09-12 23:49:09]

Description was changed from

==========
Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
==========

to

==========
Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
NOPRESUBMIT=true
==========

[eroman, 2016-09-12 23:49:38]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-09-12 23:49:53]

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-13 00:24:10]

Description was changed from

==========
Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
NOPRESUBMIT=true
==========

to

==========
Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-09-13 00:24:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-13 00:27:41]

Description was changed from

==========
Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
NOPRESUBMIT=true
==========

to

==========
Add optional context for certificate errors.

This is done by representing certificate errors as a tree of nodes
rather than a flat sequence.

Errors sharing particular contextual parameters are grouped into the
same parent node.

BUG=634443
NOPRESUBMIT=true

Committed: https://crrev.com/24da0854378c9640bf9c726a2f067c2e0ba49205
Cr-Commit-Position: refs/heads/master@{#418118}
==========

[commit-bot: I haz the power, 2016-09-13 00:27:43]

Patchset 5 (id:??) landed as
https://crrev.com/24da0854378c9640bf9c726a2f067c2e0ba49205
Cr-Commit-Position: refs/heads/master@{#418118}