Removal of SignedCertificateTimestampStore

Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and SignedCertificateTimestampIDStatus were removed because SCTs are never retrieved from the store (leftover of SCT viewer UI on some platforms). The SCTs are only shown in the DevTools, so they can be sent on IPCs if the DevTools is enabled to reduce memory usage. Numbers of invalid, valid and unknown SCTs were added to SSLStatus to access them quickly. To the ResourceResponseInfo was added a SignedCertificateTimestampAndStatusList attribute, to which a value is only assigned if the DevTools are enabled. I will also have a follow-up CL that displays SCT details in the DevTools.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/88f6c692a10e5da4d1d87dac49d243359048f097
Cr-Commit-Position: refs/heads/master@{#394271}

[dwaxweiler, 2016-05-06 07:04:16]

Description was changed from

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
==========

to

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
==========

[dwaxweiler, 2016-05-06 07:04:17]

daniel.waxweiler@gmail.com changed reviewers:
+ eranm@chromium.org, lgarron@chromium.org, mmenke@chromium.org,
nick@semenkovich.com, palmer@chromium.org

[dwaxweiler, 2016-05-06 07:05:30]

The SignedCertificateTimestampStore has been removed.

[Eran Messeri, 2016-05-06 09:01:49]

lgtm with one question.

https://codereview.chromium.org/1957483003/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:191: const ResourceResponseInfo& info,
Question: Why is the input parameter type change? Seems like you'd like to
expose more of the ResourceResponseInfo to SetSecurityStyleAndDetails but it's
unclear what, as you're not using anything more than the security_info string.

[dwaxweiler, 2016-05-06 12:45:37]

I have reacted to the only comment of Eran's review.

https://codereview.chromium.org/1957483003/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:191: const ResourceResponseInfo& info,
On 2016/05/06 09:01:48, Eran Messeri wrote:
> Question: Why is the input parameter type change? Seems like you'd like to
> expose more of the ResourceResponseInfo to SetSecurityStyleAndDetails but it's
> unclear what, as you're not using anything more than the security_info string.

Acknowledged.

[mmenke, 2016-05-06 14:29:04]

I'm going to go ahead and defer to palmer here - I assume he has some
familiarity with this stuff.

For the record, I verified dwaxweiler has signed the CLA.

[dwaxweiler, 2016-05-06 16:16:02]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-06 16:16:03]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps20001
(title: "reacted to comment of Eran")

[commit-bot: I haz the power, 2016-05-06 16:16:18]

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

[commit-bot: I haz the power, 2016-05-06 16:16:20]

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

[commit-bot: I haz the power, 2016-05-06 16:16:21]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mmenke, 2016-05-06 16:18:44]

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

[commit-bot: I haz the power, 2016-05-06 16:19:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1957483003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1957483003/20001

[commit-bot: I haz the power, 2016-05-06 16:33:18]

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

[commit-bot: I haz the power, 2016-05-06 16:33:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[palmer, 2016-05-06 18:37:59]

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

[palmer, 2016-05-06 18:38:00]

From the perspective of IPC security, LGTM. But, there is a lot going on here
and I don't know the SCT code well enough to know if it all makes sense. For
that I defer to eranm (who has already approved) and estark (who might also have
thoughts).

[dwaxweiler, 2016-05-06 20:09:47]

Description was changed from

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
==========

to

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dwaxweiler, 2016-05-06 20:15:39]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-06 20:15:40]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps40001
(title: "fixed two content_unittests")

[commit-bot: I haz the power, 2016-05-06 20:16:06]

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

[commit-bot: I haz the power, 2016-05-06 20:25:51]

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

[commit-bot: I haz the power, 2016-05-06 20:25:52]

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...)

[dwaxweiler, 2016-05-07 01:47:24]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-07 01:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps60001
(title: "fixed constructor of SSLStatus()")

[commit-bot: I haz the power, 2016-05-07 01:47:47]

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

[commit-bot: I haz the power, 2016-05-07 01:55:04]

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

[commit-bot: I haz the power, 2016-05-07 01:55:05]

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...)

[estark, 2016-05-07 07:12:51]

https://codereview.chromium.org/1957483003/diff/60001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1957483003/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.cc:106:
response->head.signed_certificate_timestamps =
Since this is only required for DevTools, how about setting it only when
info->ShouldReportRawHeaders() is true?

https://codereview.chromium.org/1957483003/diff/60001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/60001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:258: int num_unknown_scts =
ssl_status.num_unknown_scts;
size_t (and the following lines also)

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/r...
content/public/common/resource_response_info.h:151: // validation status.
If you change this to only be set when |report_raw_headers| is true, then
mention it in the comment here. (Similar to line 78)

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
File content/public/common/ssl_status.cc (right):

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
content/public/common/ssl_status.cc:54: break;
This should probably be NOTREACHED()

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
File content/public/common/ssl_status.h (right):

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
content/public/common/ssl_status.h:64: int num_unknown_scts;
Can these be size_t?

https://codereview.chromium.org/1957483003/diff/60001/net/ssl/signed_certific...
File net/ssl/signed_certificate_timestamp_and_status.h (right):

https://codereview.chromium.org/1957483003/diff/60001/net/ssl/signed_certific...
net/ssl/signed_certificate_timestamp_and_status.h:18:
SignedCertificateTimestampAndStatus();
Why is this needed?

[estark, 2016-05-07 07:21:30]

You'll also need approval from a content/ OWNER before you can land this. I
don't think anyone knows this code super well, but maybe creis@ or nasko@ would
be willing to take a look.

I think it would be helpful if you expand the CL description to include the
motivation and your future plans. The motivation is that this information is
never retrieved or displayed in the browser process, and is only shown in
DevTools for developer debugging purposes, so it can be sent on IPCs when
DevTools is enabled rather than hanging around in memory in the browser process
coupled to the renderer lifetime. And you should mention that you plan to have a
follow-up CL that displays the SCT info in DevTools.

Thanks!

[dwaxweiler, 2016-05-07 13:00:53]

Description was changed from

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dwaxweiler, 2016-05-07 13:00:53]

daniel.waxweiler@gmail.com changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[dwaxweiler, 2016-05-07 13:08:27]

Description was changed from

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed.
Numbers of invalid, valid and unknown SCTs were added to SSLStatus.
A SignedCertificateTimestampAndStatusList attribute was added to the
ResourceResponseInfo.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed because SCTs are never retrieved
from the store (leftover of SCT viewer UI on some platforms). The SCTs are only
shown in the DevTools, so they can be sent on IPCs if the DevTools is enabled to
reduce memory usage. Numbers of invalid, valid and unknown SCTs were added to
SSLStatus to access them quickly. To the ResourceResponseInfo was added a
SignedCertificateTimestampAndStatusList attribute, to which a value is only
assigned if the DevTools are enabled. I will also have a follow-up CL that
displays SCT details in the DevTools.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dwaxweiler, 2016-05-07 13:09:40]

I have reacted to the comments of estark and updated the CL's description.

https://codereview.chromium.org/1957483003/diff/60001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1957483003/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.cc:106:
response->head.signed_certificate_timestamps =
On 2016/05/07 07:12:51, estark wrote:
> Since this is only required for DevTools, how about setting it only when
> info->ShouldReportRawHeaders() is true?

Acknowledged.

https://codereview.chromium.org/1957483003/diff/60001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/60001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:258: int num_unknown_scts =
ssl_status.num_unknown_scts;
On 2016/05/07 07:12:51, estark wrote:
> size_t (and the following lines also)

Acknowledged.

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/r...
content/public/common/resource_response_info.h:151: // validation status.
On 2016/05/07 07:12:51, estark wrote:
> If you change this to only be set when |report_raw_headers| is true, then
> mention it in the comment here. (Similar to line 78)

Acknowledged.

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
File content/public/common/ssl_status.cc (right):

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
content/public/common/ssl_status.cc:54: break;
On 2016/05/07 07:12:51, estark wrote:
> This should probably be NOTREACHED()

Acknowledged.

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
File content/public/common/ssl_status.h (right):

https://codereview.chromium.org/1957483003/diff/60001/content/public/common/s...
content/public/common/ssl_status.h:64: int num_unknown_scts;
On 2016/05/07 07:12:51, estark wrote:
> Can these be size_t?
Unfortunately, they cannot because the SSLStatus serialization uses pickle.h,
which does not support size_t types, or am I wrong?

https://codereview.chromium.org/1957483003/diff/60001/net/ssl/signed_certific...
File net/ssl/signed_certificate_timestamp_and_status.h (right):

https://codereview.chromium.org/1957483003/diff/60001/net/ssl/signed_certific...
net/ssl/signed_certificate_timestamp_and_status.h:18:
SignedCertificateTimestampAndStatus();
On 2016/05/07 07:12:51, estark wrote:
> Why is this needed?
IPC requires an empty constructor.

[dwaxweiler, 2016-05-07 14:27:15]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-07 14:27:16]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps80001
(title: "reacted to comments of estark")

[commit-bot: I haz the power, 2016-05-07 14:27:25]

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

[commit-bot: I haz the power, 2016-05-07 14:33:40]

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

[commit-bot: I haz the power, 2016-05-07 14:33:42]

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...)

[estark, 2016-05-09 12:10:40]

lgtm if a //content OWNER is okay with the overall approach (trading IPC
overhead when devtools is enabled for overall simplicity, removing buggy code,
and hopefully reducing memory usage in browser process)

https://codereview.chromium.org/1957483003/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1957483003/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:106: if
(info->ShouldReportRawHeaders()) {
Note: it might make sense to rename this flag or duplicate it as
ShouldSentExtraCertificateInfo() or something rather than repurpose the existing
one. But at this point it's probably best to wait and see what a content OWNER
thinks of this whole thing before making more big changes.

[mmenke, 2016-05-09 17:06:12]

net/ LGTM (Deferring to estark, here)

[dwaxweiler, 2016-05-09 17:54:36]

daniel.waxweiler@gmail.com changed reviewers:
+ pfeldman@chromium.org

[Charlie Reis, 2016-05-09 20:54:21]

creis@chromium.org changed reviewers:
+ dgozman@chromium.org

[Charlie Reis, 2016-05-09 20:54:22]

[+dgozman]

I'd like to hear from dgozman@ or pfeldman@ about the implication for IPC
overhead when DevTools is around, but otherwise this sounds like an improvement
to me from estark's description-- simpler, less memory in common case, less
buggy.

Is it actually more IPCs when DevTools is around, or just larger ones?  I
imagine the latter isn't much of a concern.

Tiny nits below, but I'm deferring almost entirely to eranm@ and estark@ since I
don't know this code.

On 2016/05/07 07:21:30, estark wrote:
> You'll also need approval from a content/ OWNER before you can land this. I
> don't think anyone knows this code super well, but maybe creis@ or nasko@
would
> be willing to take a look.
> 
> I think it would be helpful if you expand the CL description to include the
> motivation and your future plans. The motivation is that this information is
> never retrieved or displayed in the browser process, and is only shown in
> DevTools for developer debugging purposes, so it can be sent on IPCs when
> DevTools is enabled rather than hanging around in memory in the browser
process
> coupled to the renderer lifetime. And you should mention that you plan to have
a
> follow-up CL that displays the SCT info in DevTools.
> 
> Thanks!

https://codereview.chromium.org/1957483003/diff/80001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/common/resource...
content/common/resource_messages.h:96: base::PickleIterator* iter,
nit: Wrong indent.  (See the block above.)

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/s...
File content/public/common/ssl_status.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/s...
content/public/common/ssl_status.h:64: int num_unknown_scts;
Shouldn't we be using an unsigned type (e.g., uint32_t) if they're size_t in
web_url_loader_impl.cc?  I'm not sure what the best practice is here if size_t
isn't supported by pickle.h, but this doesn't seem safe.

[estark, 2016-05-09 20:58:51]

On 2016/05/09 20:54:22, Charlie Reis wrote:
> [+dgozman]
> 
> I'd like to hear from dgozman@ or pfeldman@ about the implication for IPC
> overhead when DevTools is around, but otherwise this sounds like an
improvement
> to me from estark's description-- simpler, less memory in common case, less
> buggy.

Thanks, it would be great to hear from the devtools folks. I've also sent an
email to the speed infra team to ask what they think about this approach, but
haven't heard back yet.

> 
> Is it actually more IPCs when DevTools is around, or just larger ones?  I
> imagine the latter isn't much of a concern.

Just larger ones. It's actually fewer IPC messages overall: in the old world,
DevTools would have to IPC out to the browser to map an SCT ID into SCT details;
now the SCT details are sent on the response IPC that is sent anyway, so
DevTools doesn't have to make a separate IPC.

> 
> Tiny nits below, but I'm deferring almost entirely to eranm@ and estark@ since
I
> don't know this code.
> 
> On 2016/05/07 07:21:30, estark wrote:
> > You'll also need approval from a content/ OWNER before you can land this. I
> > don't think anyone knows this code super well, but maybe creis@ or nasko@
> would
> > be willing to take a look.
> > 
> > I think it would be helpful if you expand the CL description to include the
> > motivation and your future plans. The motivation is that this information is
> > never retrieved or displayed in the browser process, and is only shown in
> > DevTools for developer debugging purposes, so it can be sent on IPCs when
> > DevTools is enabled rather than hanging around in memory in the browser
> process
> > coupled to the renderer lifetime. And you should mention that you plan to
have
> a
> > follow-up CL that displays the SCT info in DevTools.
> > 
> > Thanks!
> 
>
https://codereview.chromium.org/1957483003/diff/80001/content/common/resource...
> File content/common/resource_messages.h (right):
> 
>
https://codereview.chromium.org/1957483003/diff/80001/content/common/resource...
> content/common/resource_messages.h:96: base::PickleIterator* iter,
> nit: Wrong indent.  (See the block above.)
> 
>
https://codereview.chromium.org/1957483003/diff/80001/content/public/common/s...
> File content/public/common/ssl_status.h (right):
> 
>
https://codereview.chromium.org/1957483003/diff/80001/content/public/common/s...
> content/public/common/ssl_status.h:64: int num_unknown_scts;
> Shouldn't we be using an unsigned type (e.g., uint32_t) if they're size_t in
> web_url_loader_impl.cc?  I'm not sure what the best practice is here if size_t
> isn't supported by pickle.h, but this doesn't seem safe.

[nasko, 2016-05-09 22:51:18]

Overall the idea of the patch makes sense to me. I'm personally fine with the
tradeoff of bigger IPCs when DevTools is active for simpler codebase.

There are still issues that need to be addressed, such as the size_t/int across
the IPC boundary.

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
content/public/common/resource_response_info.h:151: // validation status. Only
present if the renderer set report_raw_headers to
nit: s/renderer/renderer process/

[dgozman, 2016-05-09 23:09:39]

I don't think sending extra data with open DevTools is a problem, so please go
ahead.

Should we move it to ResourceDevToolsInfo to have all guarded-by-policy-check
debugging info in the same place?

[dwaxweiler, 2016-05-11 06:40:36]

I have reacted to the latest comments.

https://codereview.chromium.org/1957483003/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1957483003/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:106: if
(info->ShouldReportRawHeaders()) {
On 2016/05/09 12:10:40, estark wrote:
> Note: it might make sense to rename this flag or duplicate it as
> ShouldSentExtraCertificateInfo() or something rather than repurpose the
existing
> one. But at this point it's probably best to wait and see what a content OWNER
> thinks of this whole thing before making more big changes.

Acknowledged.

https://codereview.chromium.org/1957483003/diff/80001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/common/resource...
content/common/resource_messages.h:96: base::PickleIterator* iter,
On 2016/05/09 20:54:22, Charlie Reis wrote:
> nit: Wrong indent.  (See the block above.)

Acknowledged.

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
content/public/common/resource_response_info.h:151: // validation status. Only
present if the renderer set report_raw_headers to
On 2016/05/09 22:51:18, nasko (Out until May 23rd) wrote:
> nit: s/renderer/renderer process/
I have not understood this comment. Could you elaborate on it please?

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/s...
File content/public/common/ssl_status.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/s...
content/public/common/ssl_status.h:64: int num_unknown_scts;
On 2016/05/09 20:54:22, Charlie Reis wrote:
> Shouldn't we be using an unsigned type (e.g., uint32_t) if they're size_t in
> web_url_loader_impl.cc?  I'm not sure what the best practice is here if size_t
> isn't supported by pickle.h, but this doesn't seem safe.

Acknowledged.

https://codereview.chromium.org/1957483003/diff/100001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/100001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1579:
report_raw_headers, report_raw_headers, !is_sync_load,
Passing report_raw_headers for send_extra_certificate_info looks a bit weird
after the addition of the new field in ResourceDispatcherHostImpl. Where can I
get the starting value from?

[nasko, 2016-05-11 16:46:28]

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
content/public/common/resource_response_info.h:151: // validation status. Only
present if the renderer set report_raw_headers to
On 2016/05/11 06:40:35, dwaxweiler wrote:
> On 2016/05/09 22:51:18, nasko (Out until May 23rd) wrote:
> > nit: s/renderer/renderer process/
> I have not understood this comment. Could you elaborate on it please?

s/a/b/ is the standard Unix replacement expression. The comment was a short way
of saying "please replace 'renderer' with 'renderer process'". The word renderer
on its own is very ambiguous, so it is good to be more specific when using it.

[dwaxweiler, 2016-05-11 22:10:02]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-11 22:10:03]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
mmenke@chromium.org, estark@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps120001
(title: "updated a comment")

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

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

[commit-bot: I haz the power, 2016-05-11 22:13:19]

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

[commit-bot: I haz the power, 2016-05-11 22:13:21]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-05-11 22:23:01]

On 2016/05/11 22:13:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   ios-device on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
>   ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
>   mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

You still need a content/ owner's approval as well.  I'm doing another pass.

palmer@: Can you review for IPC again, since things have changed since your
approval?  (There's a lot of custom Pickle code dealing with size_t / u_int32
now, which could use a sanity check.)

[Charlie Reis, 2016-05-11 22:30:23]

Just a few nits remaining, and a bit of confusion about
ShouldSendExtraCertificateInfo.  Happy to approve content/ after that (if
palmer@ is still happy).

https://codereview.chromium.org/1957483003/diff/120001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/1957483003/diff/120001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client.cc:114:
ssl.num_unknown_scts, net::ct::SCT_STATUS_LOG_UNKNOWN);
nit: Wrong indent.

Please run git cl format, which can catch these cases automatically.

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1579:
report_raw_headers, report_raw_headers, !is_sync_load,
Why is this passed in twice?

Maybe it would be clearer to have a local send_extra_cert_info initialized to
report_raw_headers, with a comment saying why they use the same value?  (It's
not clear to me right now.)

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2302: false,
Please document this one like the others.

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:95: bool
ShouldSendExtraCertificateInfo() const;
nit: This doesn't belong in this block if it's not an override from
ResourceRequestInfo.  Looks like we might want a blank line after IsUsingLoFi,
since ShouldReportRawHeaders isn't an override either.

Can you add comments above this to say what it's for, and how it differs from
ShouldReportRawHeaders?  I'm confused by how it's getting used.

https://codereview.chromium.org/1957483003/diff/120001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/120001/content/public/common/...
content/public/common/resource_response_info.h:152: // report_raw_headers to
true.
Is it still report_raw_headers, or ShouldSendExtraCertificateInfo?  (I don't
understand what the latter is for if not.)

[estark, 2016-05-12 00:16:56]

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1579:
report_raw_headers, report_raw_headers, !is_sync_load,
On 2016/05/11 22:30:23, Charlie Reis wrote:
> Why is this passed in twice?
> 
> Maybe it would be clearer to have a local send_extra_cert_info initialized to
> report_raw_headers, with a comment saying why they use the same value?  (It's
> not clear to me right now.)

Daniel: sorry, I wasn't clear; I was suggesting this extra flag as a possibility
for the future, if it was too confusing to use ShouldReportRawHeaders(). I think
that Charlie's right that using ShouldReportRawHeaders() with a comment would
probably be more clear than introducing the second flag.

Charlie: ShouldReportRawHeaders() is set when the network panel is open (which
is what we use to populate requests in the security panel, which is where
Daniel's going to add the SCT details). I thought it might be confusing to
re-use ShouldReportRawHeaders() for the SCTs, since SCTs aren't really raw
headers, but we can probably just clear this up with a comment explaining why
we're using it.

[dwaxweiler, 2016-05-12 07:28:16]

I have fixed the latest issues, such as commenting the use of report_raw_headers
for adding SCT information to requests.

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/80001/content/public/common/r...
content/public/common/resource_response_info.h:151: // validation status. Only
present if the renderer set report_raw_headers to
On 2016/05/11 16:46:28, nasko (Out until May 23rd) wrote:
> On 2016/05/11 06:40:35, dwaxweiler wrote:
> > On 2016/05/09 22:51:18, nasko (Out until May 23rd) wrote:
> > > nit: s/renderer/renderer process/
> > I have not understood this comment. Could you elaborate on it please?
> 
> s/a/b/ is the standard Unix replacement expression. The comment was a short
way
> of saying "please replace 'renderer' with 'renderer process'". The word
renderer
> on its own is very ambiguous, so it is good to be more specific when using it.

Acknowledged.

https://codereview.chromium.org/1957483003/diff/120001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/1957483003/diff/120001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client.cc:114:
ssl.num_unknown_scts, net::ct::SCT_STATUS_LOG_UNKNOWN);
On 2016/05/11 22:30:23, Charlie Reis wrote:
> nit: Wrong indent.
> 
> Please run git cl format, which can catch these cases automatically.

Acknowledged.

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1579:
report_raw_headers, report_raw_headers, !is_sync_load,
On 2016/05/12 00:16:55, estark wrote:
> On 2016/05/11 22:30:23, Charlie Reis wrote:
> > Why is this passed in twice?
> > 
> > Maybe it would be clearer to have a local send_extra_cert_info initialized
to
> > report_raw_headers, with a comment saying why they use the same value? 
(It's
> > not clear to me right now.)
> 
> Daniel: sorry, I wasn't clear; I was suggesting this extra flag as a
possibility
> for the future, if it was too confusing to use ShouldReportRawHeaders(). I
think
> that Charlie's right that using ShouldReportRawHeaders() with a comment would
> probably be more clear than introducing the second flag.
> 
> Charlie: ShouldReportRawHeaders() is set when the network panel is open (which
> is what we use to populate requests in the security panel, which is where
> Daniel's going to add the SCT details). I thought it might be confusing to
> re-use ShouldReportRawHeaders() for the SCTs, since SCTs aren't really raw
> headers, but we can probably just clear this up with a comment explaining why
> we're using it.
Ah, okay, I will add a comment. Thanks for the explanation!

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1957483003/diff/120001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:95: bool
ShouldSendExtraCertificateInfo() const;
On 2016/05/11 22:30:23, Charlie Reis wrote:
> nit: This doesn't belong in this block if it's not an override from
> ResourceRequestInfo.  Looks like we might want a blank line after IsUsingLoFi,
> since ShouldReportRawHeaders isn't an override either.
> 
> Can you add comments above this to say what it's for, and how it differs from
> ShouldReportRawHeaders?  I'm confused by how it's getting used.
Removed code again.

https://codereview.chromium.org/1957483003/diff/120001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1957483003/diff/120001/content/public/common/...
content/public/common/resource_response_info.h:152: // report_raw_headers to
true.
On 2016/05/11 22:30:23, Charlie Reis wrote:
> Is it still report_raw_headers, or ShouldSendExtraCertificateInfo?  (I don't
> understand what the latter is for if not.)

Acknowledged.

[Charlie Reis, 2016-05-12 21:13:09]

Thanks!  content/ LGTM.

@palmer, can you review the IPC stuff again before this lands?

https://codereview.chromium.org/1957483003/diff/140001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1957483003/diff/140001/content/browser/loader...
content/browser/loader/resource_loader.cc:109: // These data is also used to
populate the requests in the security panel.
nit: Either "These data are" or "This data is"

https://codereview.chromium.org/1957483003/diff/140001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/1957483003/diff/140001/content/common/resourc...
content/common/resource_messages.cc:342: if (p.get()) {
nit: No braces needed for one-line body.

[palmer, 2016-05-12 22:05:33]

Yes, I was in the process of re-reviewing this. Still LGTM.

[dwaxweiler, 2016-05-12 22:24:06]

Nice! Let's run the tests again.

https://codereview.chromium.org/1957483003/diff/140001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1957483003/diff/140001/content/browser/loader...
content/browser/loader/resource_loader.cc:109: // These data is also used to
populate the requests in the security panel.
On 2016/05/12 21:13:09, Charlie Reis wrote:
> nit: Either "These data are" or "This data is"

Acknowledged.

https://codereview.chromium.org/1957483003/diff/140001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/1957483003/diff/140001/content/common/resourc...
content/common/resource_messages.cc:342: if (p.get()) {
On 2016/05/12 21:13:09, Charlie Reis wrote:
> nit: No braces needed for one-line body.

Acknowledged.

[dwaxweiler, 2016-05-12 22:24:17]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-12 22:24:18]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
mmenke@chromium.org, estark@chromium.org, palmer@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps160001
(title: "fixed a comment & removed unneeded brackets")

[commit-bot: I haz the power, 2016-05-12 22:24:41]

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

[commit-bot: I haz the power, 2016-05-12 22:27:04]

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

[commit-bot: I haz the power, 2016-05-12 22:27:06]

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

[estark, 2016-05-12 22:27:57]

On 2016/05/12 22:24:06, dwaxweiler wrote:
> Nice! Let's run the tests again.

It looks like you need to rebase ("patch failure" means the bots failed to apply
your patch, which usually means that there was a conflict). `git cl rebase`,
resolve conflicts, and `git cl upload` again should fix this.

> 
>
https://codereview.chromium.org/1957483003/diff/140001/content/browser/loader...
> File content/browser/loader/resource_loader.cc (right):
> 
>
https://codereview.chromium.org/1957483003/diff/140001/content/browser/loader...
> content/browser/loader/resource_loader.cc:109: // These data is also used to
> populate the requests in the security panel.
> On 2016/05/12 21:13:09, Charlie Reis wrote:
> > nit: Either "These data are" or "This data is"
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1957483003/diff/140001/content/common/resourc...
> File content/common/resource_messages.cc (right):
> 
>
https://codereview.chromium.org/1957483003/diff/140001/content/common/resourc...
> content/common/resource_messages.cc:342: if (p.get()) {
> On 2016/05/12 21:13:09, Charlie Reis wrote:
> > nit: No braces needed for one-line body.
> 
> Acknowledged.

[dwaxweiler, 2016-05-12 23:50:03]

The CQ bit was unchecked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-12 23:50:11]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-12 23:50:12]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
mmenke@chromium.org, estark@chromium.org, palmer@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps180001
(title: "rebased")

[commit-bot: I haz the power, 2016-05-12 23:50:34]

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

[commit-bot: I haz the power, 2016-05-13 00:02:32]

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

[commit-bot: I haz the power, 2016-05-13 00:02:34]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[dwaxweiler, 2016-05-13 07:55:43]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-13 07:55:43]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
mmenke@chromium.org, estark@chromium.org, palmer@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps200001
(title: "added missing IPC method")

[commit-bot: I haz the power, 2016-05-13 07:56:00]

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

[commit-bot: I haz the power, 2016-05-13 08:09:07]

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

[commit-bot: I haz the power, 2016-05-13 08:09:09]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dwaxweiler, 2016-05-13 08:15:50]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-13 08:15:51]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
mmenke@chromium.org, estark@chromium.org, palmer@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps220001
(title: ""fixed dereferencing operator"")

[commit-bot: I haz the power, 2016-05-13 08:16:14]

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

[commit-bot: I haz the power, 2016-05-13 08:28:28]

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

[commit-bot: I haz the power, 2016-05-13 08:28:30]

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

[dwaxweiler, 2016-05-13 13:57:17]

On 2016/05/13 08:28:30, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

@palmer, I have added one missing method in IPC. Can you have a look at it
please?

[palmer, 2016-05-17 17:57:40]

> @palmer, I have added one missing method in IPC. Can you have a look at it
> please?

Thank you for being extra careful. Patchset 12 LGTM.

[dwaxweiler, 2016-05-17 21:21:49]

The CQ bit was checked by daniel.waxweiler@gmail.com

[dwaxweiler, 2016-05-17 21:21:49]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
mmenke@chromium.org, estark@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1957483003/#ps240001
(title: "fixed IPC problem")

[commit-bot: I haz the power, 2016-05-17 21:22:22]

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

[commit-bot: I haz the power, 2016-05-17 23:53:48]

Description was changed from

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed because SCTs are never retrieved
from the store (leftover of SCT viewer UI on some platforms). The SCTs are only
shown in the DevTools, so they can be sent on IPCs if the DevTools is enabled to
reduce memory usage. Numbers of invalid, valid and unknown SCTs were added to
SSLStatus to access them quickly. To the ResourceResponseInfo was added a
SignedCertificateTimestampAndStatusList attribute, to which a value is only
assigned if the DevTools are enabled. I will also have a follow-up CL that
displays SCT details in the DevTools.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed because SCTs are never retrieved
from the store (leftover of SCT viewer UI on some platforms). The SCTs are only
shown in the DevTools, so they can be sent on IPCs if the DevTools is enabled to
reduce memory usage. Numbers of invalid, valid and unknown SCTs were added to
SSLStatus to access them quickly. To the ResourceResponseInfo was added a
SignedCertificateTimestampAndStatusList attribute, to which a value is only
assigned if the DevTools are enabled. I will also have a follow-up CL that
displays SCT details in the DevTools.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-17 23:53:50]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-17 23:56:13]

Description was changed from

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed because SCTs are never retrieved
from the store (leftover of SCT viewer UI on some platforms). The SCTs are only
shown in the DevTools, so they can be sent on IPCs if the DevTools is enabled to
reduce memory usage. Numbers of invalid, valid and unknown SCTs were added to
SSLStatus to access them quickly. To the ResourceResponseInfo was added a
SignedCertificateTimestampAndStatusList attribute, to which a value is only
assigned if the DevTools are enabled. I will also have a follow-up CL that
displays SCT details in the DevTools.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Removal of SignedCertificateTimestampStore

SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and
SignedCertificateTimestampIDStatus were removed because SCTs are never retrieved
from the store (leftover of SCT viewer UI on some platforms). The SCTs are only
shown in the DevTools, so they can be sent on IPCs if the DevTools is enabled to
reduce memory usage. Numbers of invalid, valid and unknown SCTs were added to
SSLStatus to access them quickly. To the ResourceResponseInfo was added a
SignedCertificateTimestampAndStatusList attribute, to which a value is only
assigned if the DevTools are enabled. I will also have a follow-up CL that
displays SCT details in the DevTools.

BUG=592561
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/88f6c692a10e5da4d1d87dac49d243359048f097
Cr-Commit-Position: refs/heads/master@{#394271}
==========

[commit-bot: I haz the power, 2016-05-17 23:56:15]

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

[estark, 2016-05-17 23:57:43]

Daniel: nice job, thanks so much for seeing this through! If you aren't too
burnt out from us annoying reviewers and want to continue to add the SCT details
in devtools, that would be awesome. :)