Separate cert loading code from CertLibrary and move to src/chromeos

Separate cert loading code from CertLibrary and move to src/chromeos.

The intention of this CL is to move certificate loading code without chrome/i18n dependencies into src/chromeos. This is so that networking code that needs access to certificates can also be moved to src/chromeos.

This also extracts CertLibrary from CrosLibrary.


BUG=133752
R=gspencer@chromium.org, pneubeck@chromium.org

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

[stevenjb, 2013-05-01 15:23:52]

+hasimoto@ who I believe looked at this code at one point

[Ryan Sleevi, 2013-05-01 18:16:18]

I realize I'm not an OWNER of any of this code, so please take my comments as
advisory that are probably more suitable for follow-up action items.

I realize that you're just trying to move code, not necessarily take ownership,
but I'm hoping that you will be able to direct to the most appropriate owner
(since you are getting the review here from hopefully someone knowledgeable in
this code :p)

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:236:
icu::Locale(g_browser_process->GetApplicationLocale().c_str()), error));
FYI, http://crbug.com/225830

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.h (right):

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:18: class CertLibrary : public
CertLoader::Observer {
nit: Can you provide documentation here about what threading guarantees are
afforded here to this class?

What thread(s) does it live on, and where it is safe to call methods?

For example, it is NOT (chrome) safe to call the NSS functions dealing with the
same slot on multiple threads - that's because each thread will exclusively
lock, and it's possible then for the UI thread to be blocked on the IO thread,
something that should NEVER be done.

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:73: int
GetCertIndexByPkcs11Id(CertType type, const std::string& pkcs11_id) const;
style nit: "pkcs#11" -> "PKCS#11"

You probably want to reference http://crbug.com/236978 here, so it's at least
clear the limitation here.

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/network_login_observer.cc (right):

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/n...
chrome/browser/chromeos/network_login_observer.cc:22: if
(CertLibrary::IsInitialized())
What happens if the CertLibrary is initialized after the NetworkLoginObserver?

Seems like you'd want to unconditionally register instead?

https://codereview.chromium.org/14522013/diff/19001/chromeos/cryptohome/crypt...
File chromeos/cryptohome/cryptohome_library.cc (right):

https://codereview.chromium.org/14522013/diff/19001/chromeos/cryptohome/crypt...
chromeos/cryptohome/cryptohome_library.cc:220: std::string nonce =
salt.substr(0, kNonceSize);
seems like unrelated cleanups from your local tree. Separate CL (and yes, I know
I requested them - they just seem unrelated to this code change)

https://codereview.chromium.org/14522013/diff/19001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.h (right):

https://codereview.chromium.org/14522013/diff/19001/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:26: class CHROMEOS_EXPORT CertLoader : public
net::CertDatabase::Observer,
comment nit: Threading guarantees

https://codereview.chromium.org/14522013/diff/19001/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:46: typedef
ObserverListThreadSafe<CertLoader::Observer> ObserverList;
style: You don't need to make public, do you? Shouldn't this be on line 72?

[stevenjb, 2013-05-01 20:47:59]

Thanks for the feedback Ryan, I appreciate it.

gspencer@, hashimoto@, please take a look when you have a chance, thanks!

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:236:
icu::Locale(g_browser_process->GetApplicationLocale().c_str()), error));
On 2013/05/01 18:16:18, Ryan Sleevi wrote:
> FYI, http://crbug.com/225830
Yep. That's one of several issues preventing this code from moving to
src/chromeos. I'm looking forward to that code moving out of src/chrome.

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.h (right):

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:18: class CertLibrary : public
CertLoader::Observer {
On 2013/05/01 18:16:18, Ryan Sleevi wrote:
> nit: Can you provide documentation here about what threading guarantees are
> afforded here to this class?
> 
> What thread(s) does it live on, and where it is safe to call methods?
> 
> For example, it is NOT (chrome) safe to call the NSS functions dealing with
the
> same slot on multiple threads - that's because each thread will exclusively
> lock, and it's possible then for the UI thread to be blocked on the IO thread,
> something that should NEVER be done.

Added a comment for this class and CertLoader, included notes about threading.

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:73: int
GetCertIndexByPkcs11Id(CertType type, const std::string& pkcs11_id) const;
On 2013/05/01 18:16:18, Ryan Sleevi wrote:
> style nit: "pkcs#11" -> "PKCS#11"
> 
> You probably want to reference http://crbug.com/236978 here, so it's at least
> clear the limitation here.
Done.

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/network_login_observer.cc (right):

https://codereview.chromium.org/14522013/diff/19001/chrome/browser/chromeos/n...
chrome/browser/chromeos/network_login_observer.cc:22: if
(CertLibrary::IsInitialized())
On 2013/05/01 18:16:18, Ryan Sleevi wrote:
> What happens if the CertLibrary is initialized after the NetworkLoginObserver?
> 
> Seems like you'd want to unconditionally register instead?

NetworkLoginObserver is owned by NetworkLibrary which gets instantiated for a
lot of unit tests in a kind of awkward way. CertLibrary only gets initialized by
unit tests that need it.

We're working on cleaning up these dependencies, and this class in particular
will be going away soon. I added a comment.

https://codereview.chromium.org/14522013/diff/19001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.h (right):

https://codereview.chromium.org/14522013/diff/19001/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:26: class CHROMEOS_EXPORT CertLoader : public
net::CertDatabase::Observer,
On 2013/05/01 18:16:18, Ryan Sleevi wrote:
> comment nit: Threading guarantees

Done.

https://codereview.chromium.org/14522013/diff/19001/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:46: typedef
ObserverListThreadSafe<CertLoader::Observer> ObserverList;
On 2013/05/01 18:16:18, Ryan Sleevi wrote:
> style: You don't need to make public, do you? Shouldn't this be on line 72?

Done.

[stevenjb, 2013-05-01 23:32:38]

+pneubeck@

[pneubeck (no reviews), 2013-05-02 14:58:35]

First part of my review. I didn't finish the files that don't have any comments.
This part took already quite a bit of time.

Feel free to drop suggested changes, which I could apply myself in a follow-up
CL.

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:290: class DBusServices
{
The name of this class and its comment don't match the content even more clearly
now.
We should remember to clean that up.

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:383:
CertLibrary::Shutdown();
Shutdown of CertLoader?

Can you use the reverse order for the shutdowns compared to the inits where
possible? I'd find that less surprising. And maybe drop a comment, if there's a
reason to deviate from reverse order.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:86: VLOG(1) << "RequestCertificates: " <<
LoginState::Get()->IsUserLoggedIn();
nit: clarify VLOG to explain the printed value.
E.g.
here:  VLOG << "RequestCertificates"
within the following if:
  VLOG << "Not requesting certificates as no user is logged in."
remove the other VLOG below.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:118: // private methods
nit: not helpful?

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:127: } else if (tpm_token_ready_) {
verbally this doesn't make any sense
.. if it's ready, initialize it ??

better rename the variable to "tpm_token_info_loaded_" and add a comment that
loading is skipped.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:166: void CertLoader::InitializeTPMToken() {
maybe rename to "InitializeNSSForTPM".

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:177: base::PostTaskAndReplyWithResult(
Not that it's really problematic, but I'd guess that the certificate list can
leak during shut-down.

The pattern shown in task_runner_util.h is different, it returns a value (not a
pointer).

I would instead let the second closure own the CertList (like the implementation
of PostTaskAndReplyWithResult):

CertList* result = new CertList();
...GetTaskRunner(true)->PostTaskAndReply(
   FROM_HERE,
   base::Bind(LoadNssCerts, result),
   base::Bind(&CertLoader::UpdateCerts, weak_ptr_..., base::Owned(result)));

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:188: VLOG(1) << "Update Certificates: " <<
cert_list->size();
nit: make vlog self explanatory. "Number of certs: "

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:191: cert_list_ = *cert_list;  // Copy vector
nit: why not swap?

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:205: void
CertLoader::MaybeRetryRequestCertificates() {
Rename to RetryRequestCertificates

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:206: if (!request_task_.is_null())
I don't see the gain of storing the closure request_task_ instead of a simple
boolean, like for the other "local state".
On the other hand, it's rather disturbing.

If I didn't miss something, this conditional is never fulfilled.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:237: StartLoadCertificates();
There is a race:

if LoadNSSCertificates() is currently running while a new cert is added
(removed), this call will be ignored and we miss the new cert (or the removal).

I'd either use a boolean certificate_reloading_required_ to queue another call
to StartLoadCertificates()
and use either the proposed WeakPtrFactory or yet another boolean
certificate_loading_ to check if  LoadNSSCertificates() is currenlty running.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:249: if (LoginState::Get()->IsUserLoggedIn() &&
!certificates_requested_)
the conditional !certificates_requested_ shouldn't be here, but in a more
generally named function like MaybeRequestCertificates which wraps the
RequestCertificates function.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
File chromeos/network/cert_loader.h (right):

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:60: // Call this to start the certificate list
initialization process.
remove the return value. It's confusing and never used.

Please decide if it's safe to call this if !IsUserLoggedIn or not.
If it's safe: remove all conditionals on the calling side.
Otherwise, document it here and log an error if the precondition is violated.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:69: bool certificates_loaded() const { return
certificates_loaded_; }
comment?

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:71: const std::string& tpm_token_name() const {
return tpm_token_name_; }
only valid if IsHaredwareBacked() == true.

This cached name doesn't have to be public. Callers (CertLibrary only) can use
crypto::GetTPMTokenInfo instead or the other way round.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:73: const net::CertificateList& cert_list() const
{ return cert_list_; }
only valid if certificates_loaded() == true

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:76: typedef
ObserverListThreadSafe<CertLoader::Observer> ObserverList;
I don't find a reason why this has to be thread safe.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:129: base::WeakPtrFactory<CertLoader>
weak_ptr_factory_;
There are two entry points to callback sequences:
The loop
RequestCerts -> ... -> ... -> MaybeRetry.. -> [RequestCerts]

and
StartLoadCets -> ... -> UpdateCerts

To ensure that these sequences are not accidentally kicked off multiple times,
I'd suggest to have one WeakPtrFactory for each sequence and:
- In each of the functions that are part of a sequence:
DCHECK(!weak_ptr_factory_.HasWeakPtrs());
- Wrap each callback with a static function that resets() the weak ptr before
running the original callback. So that the DCHECK doesn't trigger.

[stevenjb, 2013-05-03 01:00:31]

I appreciate the detailed review. I would like to avoid a significant refactor
with the CL. I applied the feedback that was straightforward, postponing the
changes that seem risky to me.

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:290: class DBusServices
{
On 2013/05/02 14:58:35, pneubeck wrote:
> The name of this class and its comment don't match the content even more
clearly
> now.
> We should remember to clean that up.
Agreed. I plan to do a cleanup pass soon after 28 branches.

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:383:
CertLibrary::Shutdown();
On 2013/05/02 14:58:35, pneubeck wrote:
> Shutdown of CertLoader?
> 
> Can you use the reverse order for the shutdowns compared to the inits where
> possible? I'd find that less surprising. And maybe drop a comment, if there's
a
> reason to deviate from reverse order.

Done.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:86: VLOG(1) << "RequestCertificates: " <<
LoginState::Get()->IsUserLoggedIn();
On 2013/05/02 14:58:35, pneubeck wrote:
> nit: clarify VLOG to explain the printed value.
> E.g.
> here:  VLOG << "RequestCertificates"
> within the following if:
>   VLOG << "Not requesting certificates as no user is logged in."
> remove the other VLOG below.
I don't feel that would really be more helpful for debugging.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:118: // private methods
On 2013/05/02 14:58:35, pneubeck wrote:
> nit: not helpful?

Done.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:127: } else if (tpm_token_ready_) {
On 2013/05/02 14:58:35, pneubeck wrote:
> verbally this doesn't make any sense
> .. if it's ready, initialize it ??
> 
Um, yes? The TPM needs to be initialized, once it is ready. Added a comment.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:177: base::PostTaskAndReplyWithResult(
On 2013/05/02 14:58:35, pneubeck wrote:
> Not that it's really problematic, but I'd guess that the certificate list can
> leak during shut-down.
> 
> The pattern shown in task_runner_util.h is different, it returns a value (not
a
> pointer).
> 
> I would instead let the second closure own the CertList (like the
implementation
> of PostTaskAndReplyWithResult):
> 
> CertList* result = new CertList();
> ...GetTaskRunner(true)->PostTaskAndReply(
>    FROM_HERE,
>    base::Bind(LoadNssCerts, result),
>    base::Bind(&CertLoader::UpdateCerts, weak_ptr_..., base::Owned(result)));

Not sure I follow right now, but seems like a change that will require a careful
re-factor and testing.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:188: VLOG(1) << "Update Certificates: " <<
cert_list->size();
On 2013/05/02 14:58:35, pneubeck wrote:
> nit: make vlog self explanatory. "Number of certs: "
Again, doesn't seem helpful to me, will leave for the next owner of this code...

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:191: cert_list_ = *cert_list;  // Copy vector
On 2013/05/02 14:58:35, pneubeck wrote:
> nit: why not swap?

Done.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:206: if (!request_task_.is_null())
On 2013/05/02 14:58:35, pneubeck wrote:
> I don't see the gain of storing the closure request_task_ instead of a simple
> boolean, like for the other "local state".
> On the other hand, it's rather disturbing.
> 
> If I didn't miss something, this conditional is never fulfilled.
TODO: Refactor

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:237: StartLoadCertificates();
On 2013/05/02 14:58:35, pneubeck wrote:
> There is a race:
> 
> if LoadNSSCertificates() is currently running while a new cert is added
> (removed), this call will be ignored and we miss the new cert (or the
removal).
> 
> I'd either use a boolean certificate_reloading_required_ to queue another call
> to StartLoadCertificates()
> and use either the proposed WeakPtrFactory or yet another boolean
> certificate_loading_ to check if  LoadNSSCertificates() is currenlty running.
TODO: Refactor

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:249: if (LoginState::Get()->IsUserLoggedIn() &&
!certificates_requested_)
On 2013/05/02 14:58:35, pneubeck wrote:
> the conditional !certificates_requested_ shouldn't be here, but in a more
> generally named function like MaybeRequestCertificates which wraps the
> RequestCertificates function.

TODO:Refactor

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
File chromeos/network/cert_loader.h (right):

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:60: // Call this to start the certificate list
initialization process.
On 2013/05/02 14:58:35, pneubeck wrote:
> remove the return value. It's confusing and never used.
> 
> Please decide if it's safe to call this if !IsUserLoggedIn or not.
> If it's safe: remove all conditionals on the calling side.
> Otherwise, document it here and log an error if the precondition is violated.

Done. It's actually only called within the class now; made private.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:69: bool certificates_loaded() const { return
certificates_loaded_; }
On 2013/05/02 14:58:35, pneubeck wrote:
> comment?
Unnecessary for getters, esp. with obvious names.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:71: const std::string& tpm_token_name() const {
return tpm_token_name_; }
On 2013/05/02 14:58:35, pneubeck wrote:
> only valid if IsHaredwareBacked() == true.
> 
> This cached name doesn't have to be public. Callers (CertLibrary only) can use
> crypto::GetTPMTokenInfo instead or the other way round.

The network code will be using these also (I added slot as well). There may be
some cleanup to be done here, but for now I'd like this all in one place.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:73: const net::CertificateList& cert_list() const
{ return cert_list_; }
On 2013/05/02 14:58:35, pneubeck wrote:
> only valid if certificates_loaded() == true

Done.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:73: const net::CertificateList& cert_list() const
{ return cert_list_; }
On 2013/05/02 14:58:35, pneubeck wrote:
> only valid if certificates_loaded() == true

Done.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:76: typedef
ObserverListThreadSafe<CertLoader::Observer> ObserverList;
On 2013/05/02 14:58:35, pneubeck wrote:
> I don't find a reason why this has to be thread safe.

Done.

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.h:129: base::WeakPtrFactory<CertLoader>
weak_ptr_factory_;
On 2013/05/02 14:58:35, pneubeck wrote:
> There are two entry points to callback sequences:
> The loop
> RequestCerts -> ... -> ... -> MaybeRetry.. -> [RequestCerts]
> 
> and
> StartLoadCets -> ... -> UpdateCerts
> 
> To ensure that these sequences are not accidentally kicked off multiple times,
> I'd suggest to have one WeakPtrFactory for each sequence and:
> - In each of the functions that are part of a sequence:
> DCHECK(!weak_ptr_factory_.HasWeakPtrs());
> - Wrap each callback with a static function that resets() the weak ptr before
> running the original callback. So that the DCHECK doesn't trigger.

Added a TODO

[pneubeck (no reviews), 2013-05-03 09:42:54]

reviewed all files. some comments are not placed in the last patch.


Overall this CL really makes the code clearer. Good move.

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.h (right):

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:60: // Returns true when the
certificate list has been initiailized.
nit: initiailized -> initialized

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:66: // Retruns the number of
certificates available for |type|.
nit: Retruns -> Returns

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:69: // Retreives the certificate
property for |type| at |index|.
nit: Retreives -> Retrieves

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:77: // Same as above but for a
PKCS#11 id. TODO(stevenjb): Replace this with a
nit: empty line above

https://codereview.chromium.org/14522013/diff/19002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:78: // better mechanism for uniquely
idientifying certificates, crbug.com/236978.
nit: idientifying -> identifying

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14522013/diff/19002/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:127: } else if (tpm_token_ready_) {
On 2013/05/03 01:00:31, stevenjb (chromium) wrote:
> On 2013/05/02 14:58:35, pneubeck wrote:
> > verbally this doesn't make any sense
> > .. if it's ready, initialize it ??
> > 
> Um, yes? The TPM needs to be initialized, once it is ready. Added a comment.
> 

"ready" sounds to me more like the initialization is finished. If "ready" is
before initialization, how would you call the state afterwards.
But it's a nit...

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:165: net::X509Certificate* cert =
GetCertificateAt(type, index);
if (!IsHardwareBacked())
  return false;

otherwise the comparison below compares "" with "".

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:236:
collator.reset(icu::Collator::createInstance(
nit: instead of reset(.), pass it to the scoped_ptr constructor above.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:239: collator.reset(NULL);
nit: NULL argument not needed.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.h (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:70: string16
GetCertDisplayStringAt(CertType type, int index) const;
1) It CertLibrary is only meant to be used by the network config views in the
future (network_login_observer doesn't count), then please rename it
appropriately (e.g. CertListModel) and move it to the views. Absolutely no need
to let it be a singleton in this case.

In this case, you can ignore the rest of this comment :-)


2) Otherwise, if CertLibrary is meant to be have any general purpose in the
future then:

This interface doesn't appear flexible to me. It's incompatible to any other
list operations (e.g. of stl).

It adds another (singleton) layer without obvious reason.

I'm fine with that for now, as the main purpose was splitting CertLibrary into
manageable parts. But I don't see why it's a good solution in the mid-term.

Alternative:
All member-functions here can be easily be migrated to static/global functions.
Thus a collection of helper functions that work on net::CertificateList seems
sufficient to me.

Other benefit of this alternative: it's trivial to test.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:93: // Sorted certificate lists
nit: empty line above

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/network_login_observer.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/n...
chrome/browser/chromeos/network_login_observer.cc:20: if
(CertLibrary::IsInitialized())
You could use CertLoader already. I know that it's gonna be removed, but it's a
small change and it separates CertLibrary even more clearly.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:423:
CertLibrary::CERT_TYPE_SERVER_CA), 0);
shouldn't this compare NumCertificates with cert_index ?

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:473: if
(!CertLibrary::Get()->CertificatesLoaded())
Why this restriction? The comment should explain why we don't always listen for
changes.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:934: if
(!CertLibrary::Get()->CertificatesLoaded())
same comment as for vpn config...

[hashimoto, 2013-05-03 16:07:43]

some minor comments

https://codereview.chromium.org/14522013/diff/36001/.gitmodules
File .gitmodules (right):

https://codereview.chromium.org/14522013/diff/36001/.gitmodules#newcode25
.gitmodules:25: [submodule "chrome/test/data/perf/third_party/octane"]
Are these changes intended to be included in this patch?

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:87: static CertLibrary*
g_cert_library = NULL;
nit: How about moving this into the unnamed namespace and remove 'static'?

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_unittest.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_unittest.cc:29: using
::testing::AnyNumber;
nit: Do we need this using and gmock.h include?

https://codereview.chromium.org/14522013/diff/36001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:36: static CertLoader* g_cert_loader = NULL;
nit: How about moving this into the unnamed namespace and remove 'static'?

[stevenjb, 2013-05-03 17:02:32]

PTAL

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:87: static CertLibrary*
g_cert_library = NULL;
On 2013/05/03 16:07:44, hashimoto wrote:
> nit: How about moving this into the unnamed namespace and remove 'static'?
I find global handles to singleton classes useful for debugging.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:165: net::X509Certificate* cert =
GetCertificateAt(type, index);
On 2013/05/03 09:42:54, pneubeck wrote:
> if (!IsHardwareBacked())
>   return false;
> 
> otherwise the comparison below compares "" with "".

Done.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:236:
collator.reset(icu::Collator::createInstance(
On 2013/05/03 09:42:54, pneubeck wrote:
> nit: instead of reset(.), pass it to the scoped_ptr constructor above.

Done.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:239: collator.reset(NULL);
On 2013/05/03 09:42:54, pneubeck wrote:
> nit: NULL argument not needed.

Done.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.h (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:70: string16
GetCertDisplayStringAt(CertType type, int index) const;
I agree that CertLibrary is not a great name and that this code could stand to
be re-factored, but I wanted to limit the amount of real change in this CL.

Ultimately when we rewrite the configuration UI we should abandon this class and
write a more specific, non-singleton layer on top of CertLoader (or CertHandler
or whatever it becomes).

I am concerned about the parsing overhead of OnCertificatesLoaded() if we were
to just turn this into namespaced functions.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:93: // Sorted certificate lists
On 2013/05/03 09:42:54, pneubeck wrote:
> nit: empty line above

Done.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/network_login_observer.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/n...
chrome/browser/chromeos/network_login_observer.cc:20: if
(CertLibrary::IsInitialized())
On 2013/05/03 09:42:54, pneubeck wrote:
> You could use CertLoader already. I know that it's gonna be removed, but it's
a
> small change and it separates CertLibrary even more clearly.

Done.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:423:
CertLibrary::CERT_TYPE_SERVER_CA), 0);
On 2013/05/03 09:42:54, pneubeck wrote:
> shouldn't this compare NumCertificates with cert_index ?
CertLibrary already does these checks; removed this one.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/vpn_config_view.cc:473: if
(!CertLibrary::Get()->CertificatesLoaded())
On 2013/05/03 09:42:54, pneubeck wrote:
> Why this restriction? The comment should explain why we don't always listen
for
> changes.
Changed to always add.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/wifi_config_view.cc:934: if
(!CertLibrary::Get()->CertificatesLoaded())
On 2013/05/03 09:42:54, pneubeck wrote:
> same comment as for vpn config...

Done.

https://codereview.chromium.org/14522013/diff/36001/chromeos/network/cert_loa...
File chromeos/network/cert_loader.cc (right):

https://codereview.chromium.org/14522013/diff/36001/chromeos/network/cert_loa...
chromeos/network/cert_loader.cc:36: static CertLoader* g_cert_loader = NULL;
On 2013/05/03 16:07:44, hashimoto wrote:
> nit: How about moving this into the unnamed namespace and remove 'static'?
Global for debugging.

[stevenjb, 2013-05-03 17:09:29]

https://codereview.chromium.org/14522013/diff/18002/chrome/browser/chromeos/n...
File chrome/browser/chromeos/network_login_observer.cc (right):

https://codereview.chromium.org/14522013/diff/18002/chrome/browser/chromeos/n...
chrome/browser/chromeos/network_login_observer.cc:16: // CertLibrary does not
get initialized in many unit tests even though
s/CertLibrary/CertLoader

[Greg Spencer (Chromium), 2013-05-03 17:24:04]

LGTM. I look forward to getting this one refactored more thoroughly though.

[pneubeck (no reviews), 2013-05-03 17:24:05]

lgtm

The only unknown that I see, is the change from the explicit calls to the login
notification. That _might_ reveal some unexpected behavior.

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.h (right):

https://codereview.chromium.org/14522013/diff/36001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.h:70: string16
GetCertDisplayStringAt(CertType type, int index) const;
On 2013/05/03 17:02:32, stevenjb (chromium) wrote:
> I agree that CertLibrary is not a great name and that this code could stand to
> be re-factored, but I wanted to limit the amount of real change in this CL.
> 
> Ultimately when we rewrite the configuration UI we should abandon this class
and
> write a more specific, non-singleton layer on top of CertLoader (or
CertHandler
> or whatever it becomes).
> 
> I am concerned about the parsing overhead of OnCertificatesLoaded() if we were
> to just turn this into namespaced functions.

Ok.

[commit-bot: I haz the power, 2013-05-03 18:36:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14522013/18002

[commit-bot: I haz the power, 2013-05-03 18:46:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-03 21:55:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14522013/18002

[commit-bot: I haz the power, 2013-05-03 22:00:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14522013/71001

[commit-bot: I haz the power, 2013-05-04 03:57:15]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[stevenjb, 2013-05-06 19:52:02]

Committed patchset #15 manually as r198511 (presubmit successful).