Add an SignedCertificateTimetampStore, making SignedCertificateTimestamp be refcounted to aid.

Add an SignedCertificateTimetampStore, making SignedCertificateTimestamp be refcounted to aid.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236797

[alcutter, 2013-11-18 14:56:37]

Building on the previous CL - this one adds a new
SignedCertificateTimestampStore.

[wtc, 2013-11-19 01:57:40]

Patch set 2 LGTM.

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
File content/browser/sct_store_impl.cc (right):

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.cc:22: : store_() {}

It is not necessary to initialize the store_ member like this if you are using
its default constructor. "store_()" can be omitted.

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
File content/browser/sct_store_impl.h (right):

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.h:15: class SignedCertificateTimestampStoreImpl

The Style Guide recommends that these two files be named
signed_certificate_timestamp_store_impl.{h,c} to match the class name.

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.h:19: static SignedCertificateTimestampStoreImpl*
GetInstance();

It seems that we don't need a GetInstance() method in the Impl class.

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.h:31: virtual
~SignedCertificateTimestampStoreImpl();

Both of these can probably be private because I don't think this class will have
subclasses.

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
File content/public/browser/sct_store.h (right):

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
content/public/browser/sct_store.h:24: // track of the SCTs associated to loaded
resources.  It can be accessed from

Nit: associated to => associated with ?

Also fix line 34.

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
content/public/browser/sct_store.h:28: class SignedCertificateTimestampStore {

Our Style Guide recommends that the file name matches the class name. So these
files should be named signed_certificate_timestamp_store.{h,cc}.

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
content/public/browser/sct_store.h:44: virtual bool
RetrieveSignedCertificateTimestamp(int sct_id,

Nit: if you think these two method names are long, we can shorten them to
"Store" and "Retrieve".

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
net/cert/signed_certificate_timestamp.h:74: : public
base::RefCountedThreadSafe<SignedCertificateTimestamp> {

We need to make SignedCertificateTimestamp ref-counted because the
RendererDataMemoizingStore template requires a ref-counted type, right?

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
net/cert/signed_certificate_timestamp.h:77: // Compares signature bytes
(timestamps could potentially not be unique).

Are you sure we don't need to compare every member of the struct?

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.cc#...
net/test/ct_test_util.cc:145: sct->swap(ret);

I think the "swap" idiom is only used by functions that may fail. In those
functions, we only swap the output argument when we are sure the function will
succeed.

Since these two functions return void, I think we should just do:
  *sct = new SignedCertificateTimestamp();
  sct->log_id = HexToBytes(kTestKeyId);
  sct->timestamp = ...

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.h#n...
net/test/ct_test_util.h:13: class SignedCertificateTimestamp;

Is this correct? SignedCertificateTimestamp is defined in the "ct" namespace and
is a struct rather than a class.

[alcutter, 2013-11-19 15:51:33]

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
File content/browser/sct_store_impl.cc (right):

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.cc:22: : store_() {}
On 2013/11/19 01:57:40, wtc wrote:
> 
> It is not necessary to initialize the store_ member like this if you are using
> its default constructor. "store_()" can be omitted.

Good call, done.

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
File content/browser/sct_store_impl.h (right):

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.h:15: class SignedCertificateTimestampStoreImpl
On 2013/11/19 01:57:40, wtc wrote:
> 
> The Style Guide recommends that these two files be named
> signed_certificate_timestamp_store_impl.{h,c} to match the class name.

Done.

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.h:19: static SignedCertificateTimestampStoreImpl*
GetInstance();
On 2013/11/19 01:57:40, wtc wrote:
> 
> It seems that we don't need a GetInstance() method in the Impl class.

Looks to be a requirement of the singleton template that the type T passed in to
Singleton<T> must have a GetInstance() method.

The GetInstance in the public SignedCertificateTimestamp interface just
delegates to this one.

This is the same pattern as the [pre-existing] cert_store code uses.

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
File content/public/browser/sct_store.h (right):

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
content/public/browser/sct_store.h:24: // track of the SCTs associated to loaded
resources.  It can be accessed from
On 2013/11/19 01:57:40, wtc wrote:
> 
> Nit: associated to => associated with ?
> 
> Also fix line 34.

Done.

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
content/public/browser/sct_store.h:44: virtual bool
RetrieveSignedCertificateTimestamp(int sct_id,
On 2013/11/19 01:57:40, wtc wrote:
> 
> Nit: if you think these two method names are long, we can shorten them to
> "Store" and "Retrieve".

Done.

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
net/cert/signed_certificate_timestamp.h:74: : public
base::RefCountedThreadSafe<SignedCertificateTimestamp> {
On 2013/11/19 01:57:40, wtc wrote:
> 
> We need to make SignedCertificateTimestamp ref-counted because the
> RendererDataMemoizingStore template requires a ref-counted type, right?

Yes, that's right.

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
net/cert/signed_certificate_timestamp.h:77: // Compares signature bytes
(timestamps could potentially not be unique).
On 2013/11/19 01:57:40, wtc wrote:
> 
> Are you sure we don't need to compare every member of the struct?

I don't think so: the sig bytes are vanishingly unlikely to match unless we're
comparing two instances of the same SCT.

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.cc#...
net/test/ct_test_util.cc:145: sct->swap(ret);
On 2013/11/19 01:57:40, wtc wrote:
> 
> I think the "swap" idiom is only used by functions that may fail. In those
> functions, we only swap the output argument when we are sure the function will
> succeed.
> 
> Since these two functions return void, I think we should just do:
>   *sct = new SignedCertificateTimestamp();
>   sct->log_id = HexToBytes(kTestKeyId);
>   sct->timestamp = ...

no problem, done.

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/72333007/diff/30001/net/test/ct_test_util.h#n...
net/test/ct_test_util.h:13: class SignedCertificateTimestamp;
On 2013/11/19 01:57:40, wtc wrote:
> 
> Is this correct? SignedCertificateTimestamp is defined in the "ct" namespace
and
> is a struct rather than a class.

Good catch - that certainly wasn't correct.

[agl, 2013-11-19 16:12:40]

LGTM

https://codereview.chromium.org/72333007/diff/140001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/72333007/diff/140001/net/test/ct_test_util.cc...
net/test/ct_test_util.cc:135: CHECK(sct != NULL);
nit: it would also be fine to have the argument be called sct_ref and then

SignedCertificateTimestamp *const sct(sct_ref.get());

In order to avoid all the (*sct) stuff.

(Up to you. No need to change.)

[agl, 2013-11-19 17:45:11]

+sky for content/browser OWNERS

[alcutter, 2013-11-19 18:03:47]

https://codereview.chromium.org/72333007/diff/140001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/72333007/diff/140001/net/test/ct_test_util.cc...
net/test/ct_test_util.cc:135: CHECK(sct != NULL);
On 2013/11/19 16:12:40, agl wrote:
> nit: it would also be fine to have the argument be called sct_ref and then
> 
> SignedCertificateTimestamp *const sct(sct_ref.get());
> 
> In order to avoid all the (*sct) stuff.
> 
> (Up to you. No need to change.)

"My god, it's full of stars."

Good idea, done.

[sky, 2013-11-19 20:24:20]

I'm not familiar with this code at all. You're going to need an owner for
content/public

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
content/content_browser.gypi:748:
'browser/signed_certificate_timestamp_store_impl.cc',
nit: sort

[wtc, 2013-11-19 21:41:44]

Patch set 5 LGTM.

sky: I have a question for you below. Please search for "Scott".

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
net/cert/signed_certificate_timestamp.h:77: // Compares signature bytes
(timestamps could potentially not be unique).

On 2013/11/19 15:51:34, alcutter wrote:
>
> I don't think so: the sig bytes are vanishingly unlikely to match unless we're
> comparing two instances of the same SCT.

That implies we should compare the signature bytes first. But that doesn't
necessarily imply we should not compare the other struct members.

For example, if two SignedCertificateTimestamp objects have the same signature
bytes but different timestamps, do you want RendererDataMemoizingStore to
consider them as the same?

The LessThan operator is used by
    typedef std::map<T*, int, typename T::LessThan> ReverseItemMap;
in RendererDataMemoizingStore. I am not familiar with how std::map uses the
LessThan operator, so it's possible that the operator doesn't need to be exact.

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
File content/browser/signed_certificate_timestamp_store_impl.cc (right):

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
content/browser/signed_certificate_timestamp_store_impl.cc:11:
SignedCertificateTimestampStore::GetInstance() {

The style guide recommends NOT indenting this line:

  * If you break after the return type of a function definition, do not indent.

Also fix line 17.

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
File content/browser/signed_certificate_timestamp_store_impl.h (right):

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
content/browser/signed_certificate_timestamp_store_impl.h:8: #include
"base/memory/singleton.h"

Nit: based on the comments in "base/memory/singleton.h", we should be able to
get away with just a forward declaration in this .h file, and only move this
#include to the .cc file.

The forward declaration we need here is:

template <typename T> struct DefaultSingletonTraits;

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
content/content_browser.gypi:748:
'browser/signed_certificate_timestamp_store_impl.cc',

On 2013/11/19 20:24:21, sky wrote:
> nit: sort

Scott: could you clarify? This file seems to be listed in the right order
("service_worker" should be before "signed_certificate_timestamp_store_impl").

https://codereview.chromium.org/72333007/diff/220001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/72333007/diff/220001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.h:77: // Compares signature bytes
(timestamps could potentially not be unique).

Nit: This comment (line 77) should be moved to the .cc file because it describes
the implementation.

https://codereview.chromium.org/72333007/diff/220001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/72333007/diff/220001/net/test/ct_test_util.h#...
net/test/ct_test_util.h:8: #include <string>

Nit: add a blank line after this line.

[sky, 2013-11-19 22:41:46]

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
content/content_browser.gypi:748:
'browser/signed_certificate_timestamp_store_impl.cc',
On 2013/11/19 21:41:44, wtc wrote:
> 
> On 2013/11/19 20:24:21, sky wrote:
> > nit: sort
> 
> Scott: could you clarify? This file seems to be listed in the right order
> ("service_worker" should be before "signed_certificate_timestamp_store_impl").


Looks like all the browser/service_worker files are in the wrong place.

[wtc, 2013-11-19 23:02:38]

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
content/content_browser.gypi:748:
'browser/signed_certificate_timestamp_store_impl.cc',

On 2013/11/19 22:41:47, sky wrote:
>
> Looks like all the browser/service_worker files are in the wrong place.

I got it now. You were referring to
browser/signed_certificate_timestamp_store_impl.h being before browser/net/*.

[alcutter, 2013-11-20 12:53:28]

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
File content/browser/sct_store_impl.h (right):

https://codereview.chromium.org/72333007/diff/30001/content/browser/sct_store...
content/browser/sct_store_impl.h:31: virtual
~SignedCertificateTimestampStoreImpl();
On 2013/11/19 01:57:40, wtc wrote:
> 
> Both of these can probably be private because I don't think this class will
have
> subclasses.

(did this in a previous patch)

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
File content/public/browser/sct_store.h (right):

https://codereview.chromium.org/72333007/diff/30001/content/public/browser/sc...
content/public/browser/sct_store.h:28: class SignedCertificateTimestampStore {
On 2013/11/19 01:57:40, wtc wrote:
> 
> Our Style Guide recommends that the file name matches the class name. So these
> files should be named signed_certificate_timestamp_store.{h,cc}.

(did this in a previous patch, forgot to reply to it.)

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/72333007/diff/30001/net/cert/signed_certifica...
net/cert/signed_certificate_timestamp.h:77: // Compares signature bytes
(timestamps could potentially not be unique).
On 2013/11/19 21:41:44, wtc wrote:
> 
> On 2013/11/19 15:51:34, alcutter wrote:
> >
> > I don't think so: the sig bytes are vanishingly unlikely to match unless
we're
> > comparing two instances of the same SCT.
> 
> That implies we should compare the signature bytes first. But that doesn't
> necessarily imply we should not compare the other struct members.
> 
> For example, if two SignedCertificateTimestamp objects have the same signature
> bytes but different timestamps, do you want RendererDataMemoizingStore to
> consider them as the same?

Technically you're right, and I'm more than happy to add the code to compare
other members for completeness, though in practise I'd be very surprised if
there were ever two SCTs with the same signature bytes (it's effectively
indistinguishable from a 256-bit random number).

> 
> The LessThan operator is used by
>     typedef std::map<T*, int, typename T::LessThan> ReverseItemMap;
> in RendererDataMemoizingStore. I am not familiar with how std::map uses the
> LessThan operator, so it's possible that the operator doesn't need to be
exact.

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
content/content_browser.gypi:748:
'browser/signed_certificate_timestamp_store_impl.cc',
On 2013/11/19 23:02:39, wtc wrote:
> 
> On 2013/11/19 22:41:47, sky wrote:
> >
> > Looks like all the browser/service_worker files are in the wrong place.
> 
> I got it now. You were referring to
> browser/signed_certificate_timestamp_store_impl.h being before browser/net/*.
> 

Moved my additions as well as the existing service_worker lines down into
alphabetical order.

[alcutter, 2013-11-20 14:57:53]

On 2013/11/19 20:24:20, sky wrote:
> I'm not familiar with this code at all. You're going to need an owner for
> content/public
> 
>
https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
> File content/content_browser.gypi (right):
> 
>
https://codereview.chromium.org/72333007/diff/220001/content/content_browser....
> content/content_browser.gypi:748:
> 'browser/signed_certificate_timestamp_store_impl.cc',
> nit: sort

+avi for content/public

[sky, 2013-11-20 21:45:12]

Since this went through eng review John should be more familiar with this then
I. So,
-sky
+john (sorry)

[wtc, 2013-11-21 03:13:46]

Patch set 7 LGTM. Thanks.

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
File content/browser/signed_certificate_timestamp_store_impl.cc (right):

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
content/browser/signed_certificate_timestamp_store_impl.cc:11:
SignedCertificateTimestampStore::GetInstance() {

On 2013/11/19 21:41:44, wtc wrote:
> 
> The style guide recommends NOT indenting this line:
> 
>   * If you break after the return type of a function definition, do not
indent.
> 
> Also fix line 17.

Please fix this, unless clang format recommends formatting it this way.

https://codereview.chromium.org/72333007/diff/220001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/72333007/diff/220001/net/test/ct_test_util.h#...
net/test/ct_test_util.h:8: #include <string>

On 2013/11/19 21:41:44, wtc wrote:
> 
> Nit: add a blank line after this line.

Please fix this.

https://codereview.chromium.org/72333007/diff/590001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.cc (right):

https://codereview.chromium.org/72333007/diff/590001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:32: return false;

Thanks for implementing this.

This is more complicated than I expected. Perhaps we should code it this way:

  if (lhs.get() == rhs.get())
    return false;
  if (lhs->signature.signature_data != rhs->signature.signature_data)
    return lhs->signature.signature_data < rhs->signature.signature_data;
  if (lhs->log_id != rhs->log_id)
    return lhs->log_id < rhs->log_id;
  if (lhs->timestamp != rhs->timestamp)
    return lhs->timestamp < rhs->timestamp;
  ...
  return lhs->version < rhs->version;

[alcutter, 2013-11-21 10:48:19]

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
File content/browser/signed_certificate_timestamp_store_impl.cc (right):

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
content/browser/signed_certificate_timestamp_store_impl.cc:11:
SignedCertificateTimestampStore::GetInstance() {
On 2013/11/21 03:13:46, wtc wrote:
> 
> On 2013/11/19 21:41:44, wtc wrote:
> > 
> > The style guide recommends NOT indenting this line:
> > 
> >   * If you break after the return type of a function definition, do not
> indent.
> > 
> > Also fix line 17.
> 
> Please fix this, unless clang format recommends formatting it this way.

Sorry, I think I fixed this and then somehow undid that while messing with git
during the last patch.

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
File content/browser/signed_certificate_timestamp_store_impl.h (right):

https://codereview.chromium.org/72333007/diff/220001/content/browser/signed_c...
content/browser/signed_certificate_timestamp_store_impl.h:8: #include
"base/memory/singleton.h"
On 2013/11/19 21:41:44, wtc wrote:
> 
> Nit: based on the comments in "base/memory/singleton.h", we should be able to
> get away with just a forward declaration in this .h file, and only move this
> #include to the .cc file.
> 
> The forward declaration we need here is:
> 
> template <typename T> struct DefaultSingletonTraits;
> 

Done.

https://codereview.chromium.org/72333007/diff/220001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/72333007/diff/220001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.h:77: // Compares signature bytes
(timestamps could potentially not be unique).
On 2013/11/19 21:41:44, wtc wrote:
> 
> Nit: This comment (line 77) should be moved to the .cc file because it
describes
> the implementation.

Comment removed since it's not applicable anymore.

https://codereview.chromium.org/72333007/diff/220001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/72333007/diff/220001/net/test/ct_test_util.h#...
net/test/ct_test_util.h:8: #include <string>
On 2013/11/21 03:13:46, wtc wrote:
> 
> On 2013/11/19 21:41:44, wtc wrote:
> > 
> > Nit: add a blank line after this line.
> 
> Please fix this.

Done.

[alcutter, 2013-11-21 10:56:39]

https://codereview.chromium.org/72333007/diff/590001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.cc (right):

https://codereview.chromium.org/72333007/diff/590001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:32: return false;
On 2013/11/21 03:13:47, wtc wrote:
> 
> Thanks for implementing this.
> 
> This is more complicated than I expected. Perhaps we should code it this way:
> 
>   if (lhs.get() == rhs.get())
>     return false;
>   if (lhs->signature.signature_data != rhs->signature.signature_data)
>     return lhs->signature.signature_data < rhs->signature.signature_data;
>   if (lhs->log_id != rhs->log_id)
>     return lhs->log_id < rhs->log_id;
>   if (lhs->timestamp != rhs->timestamp)
>     return lhs->timestamp < rhs->timestamp;
>   ...
>   return lhs->version < rhs->version;

That's a much more pleasing way of doing it, thanks.
Done.

[jam, 2013-11-21 19:47:44]

https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
File content/public/browser/signed_certificate_timestamp_store.h (right):

https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
content/public/browser/signed_certificate_timestamp_store.h:28: class
SignedCertificateTimestampStore {
why is this in content/public, is there code in src/chrome that will use this? I
can't tell since there's no chrome changes in this cl

[Eran M. (Google), 2013-11-21 19:59:32]

On 2013/11/21 19:47:44, jam wrote:
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> File content/public/browser/signed_certificate_timestamp_store.h (right):
> 
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> content/public/browser/signed_certificate_timestamp_store.h:28: class
> SignedCertificateTimestampStore {
> why is this in content/public, is there code in src/chrome that will use this?
I
> can't tell since there's no chrome changes in this cl

There will be a UI component that will display the Signed Certificate
Timestamps, similar to the Certificate Viewer, so the renderer has to know the
IDs of the SCTs in the same way it needs to know the ID of the certificate.

[alcutter, 2013-11-21 20:04:43]

On 2013/11/21 19:47:44, jam wrote:
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> File content/public/browser/signed_certificate_timestamp_store.h (right):
> 
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> content/public/browser/signed_certificate_timestamp_store.h:28: class
> SignedCertificateTimestampStore {
> why is this in content/public, is there code in src/chrome that will use this?
I
> can't tell since there's no chrome changes in this cl

Yes, there are other pending changes (by Eran) which will use this for showing
the SignedCertificateTimestamps this class holds in the certificate viewer in
chrome.
This class will be used similarly to the one in
content/public/browser/cert_viewer.h

[jam, 2013-11-21 20:06:44]

On 2013/11/21 19:59:32, eranm wrote:
> On 2013/11/21 19:47:44, jam wrote:
> >
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> > File content/public/browser/signed_certificate_timestamp_store.h (right):
> > 
> >
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> > content/public/browser/signed_certificate_timestamp_store.h:28: class
> > SignedCertificateTimestampStore {
> > why is this in content/public, is there code in src/chrome that will use
this?
> I
> > can't tell since there's no chrome changes in this cl
> 
> There will be a UI component that will display the Signed Certificate
> Timestamps, similar to the Certificate Viewer, so the renderer has to know the
> IDs of the SCTs in the same way it needs to know the ID of the certificate.

Do you have the code in chrome already written? i.e. I still don't understand
how we know for sure that this is the public interface that will be needed? is
it because you based it on cert_store.h?

Also another question: why is there two different classes for this and
CertStore? i.e. why isn't that one class used for signed certificates as well?

[wtc, 2013-11-21 20:26:39]

Patch set 8 LGTM.

[alcutter, 2013-11-22 11:01:26]

On 2013/11/21 20:06:44, jam wrote:
> On 2013/11/21 19:59:32, eranm wrote:
> > On 2013/11/21 19:47:44, jam wrote:
> > >
> >
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> > > File content/public/browser/signed_certificate_timestamp_store.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> > > content/public/browser/signed_certificate_timestamp_store.h:28: class
> > > SignedCertificateTimestampStore {
> > > why is this in content/public, is there code in src/chrome that will use
> this?
> > I
> > > can't tell since there's no chrome changes in this cl
> > 
> > There will be a UI component that will display the Signed Certificate
> > Timestamps, similar to the Certificate Viewer, so the renderer has to know
the
> > IDs of the SCTs in the same way it needs to know the ID of the certificate.
> 
> Do you have the code in chrome already written? i.e. I still don't understand
> how we know for sure that this is the public interface that will be needed? is
> it because you based it on cert_store.h?

there's a rough outline of the overall code spread across a few CLs:
https://codereview.chromium.org/27026002/
https://codereview.chromium.org/71633002/
https://codereview.chromium.org/76443006/

As you mentioned, the interface is based on cert_store.h and we know that works
well for passing across certificates to be shown in the UI, so we are confident
that a similar approach works for SCTs too.

Eran can give you more detail on that if needed.

> 
> Also another question: why is there two different classes for this and
> CertStore? i.e. why isn't that one class used for signed certificates as well?

Originally that was how it was implemented, but since these are
SignedCertificateTimestamps and not Certificates it seemed rather like we were
shoe-horning extra functionality into that class which shouldn't really be
there; the SCTs cannot be directly associated with the Certificates held in that
class due to a subtle potential issue (whereby a given Certificate is received
multiple times from one or more servers, but is accompanied by differing sets of
SCTs each time - we'd then be validating/displaying SCT info from the union of
the sets rather than per connection), so we end up having to have two logically
distinct stores in the same class.

[jam, 2013-11-22 16:19:34]

On 2013/11/22 11:01:26, alcutter wrote:
> On 2013/11/21 20:06:44, jam wrote:
> > On 2013/11/21 19:59:32, eranm wrote:
> > > On 2013/11/21 19:47:44, jam wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> > > > File content/public/browser/signed_certificate_timestamp_store.h
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/72333007/diff/840001/content/public/browser/s...
> > > > content/public/browser/signed_certificate_timestamp_store.h:28: class
> > > > SignedCertificateTimestampStore {
> > > > why is this in content/public, is there code in src/chrome that will use
> > this?
> > > I
> > > > can't tell since there's no chrome changes in this cl
> > > 
> > > There will be a UI component that will display the Signed Certificate
> > > Timestamps, similar to the Certificate Viewer, so the renderer has to know
> the
> > > IDs of the SCTs in the same way it needs to know the ID of the
certificate.
> > 
> > Do you have the code in chrome already written? i.e. I still don't
understand
> > how we know for sure that this is the public interface that will be needed?
is
> > it because you based it on cert_store.h?
> 
> there's a rough outline of the overall code spread across a few CLs:
> https://codereview.chromium.org/27026002/
> https://codereview.chromium.org/71633002/
> https://codereview.chromium.org/76443006/
> 
> As you mentioned, the interface is based on cert_store.h and we know that
works
> well for passing across certificates to be shown in the UI, so we are
confident
> that a similar approach works for SCTs too.
> 
> Eran can give you more detail on that if needed.
> 
> > 
> > Also another question: why is there two different classes for this and
> > CertStore? i.e. why isn't that one class used for signed certificates as
well?
> 
> Originally that was how it was implemented, but since these are
> SignedCertificateTimestamps and not Certificates it seemed rather like we were
> shoe-horning extra functionality into that class which shouldn't really be
> there; the SCTs cannot be directly associated with the Certificates held in
that
> class due to a subtle potential issue (whereby a given Certificate is received
> multiple times from one or more servers, but is accompanied by differing sets
of
> SCTs each time - we'd then be validating/displaying SCT info from the union of
> the sets rather than per connection), so we end up having to have two
logically
> distinct stores in the same class.

I see, thanks for the background. lgtm

[commit-bot: I haz the power, 2013-11-22 16:23:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alcutter@google.com/72333007/840001

[commit-bot: I haz the power, 2013-11-22 18:46:40]

Change committed as 236797